Re: CVE-2023-52596: sysctl: Fix out of bounds access for empty sysctl registers

[Joel Granados]

On Mon, Mar 11, 2024 at 02:57:50PM -0700, Luis Chamberlain wrote:
> On Mon, Mar 11, 2024 at 09:11:27AM +0100, Michal Hocko wrote:
> > On Wed 06-03-24 06:45:54, Greg KH wrote:
> > > Description
> > > ===========
> > > 
> > > In the Linux kernel, the following vulnerability has been resolved:
> > > 
> > > sysctl: Fix out of bounds access for empty sysctl registers
> > > 
> > > When registering tables to the sysctl subsystem there is a check to see
> > > if header is a permanently empty directory (used for mounts). This check
> > > evaluates the first element of the ctl_table. This results in an out of
> > > bounds evaluation when registering empty directories.
> > > 
> > > The function register_sysctl_mount_point now passes a ctl_table of size
> > > 1 instead of size 0. It now relies solely on the type to identify
> > > a permanently empty register.
> > > 
> > > Make sure that the ctl_table has at least one element before testing for
> > > permanent emptiness.
> > 
> > While this makes the code more robust and more future proof I do not think
> > this is fixing any real issue not to mention anything with security
> > implications. AFAIU there is no actual code that can generate empty
> > sysctl directories unless the kernel is heavily misconfigured.
> > 
> > Luis, Joel, what do you think?
> 
> As per review with Joel:
> 
> The out-of-bounds issue cannot be triggered in older kernels unless
> you had external modules with empty sysctls. That is because although
> support for empty sysctls was added on v6.6 none of the sysctls that
> were trimmed for the superfluous entry over the different kernel
> releases so far has had the chance to be empty.
> 
> The 0-day reported crash was for a future out of tree patch which was
> never merged yet:
> 
> https://github.com/Joelgranados/linux/commit/423f75083acd947804c8d5c31ad1e1b5fcb3c020                                                                                                       
> 
> Let's examine this commit to see why it triggers a crash to understand
> how the out of bounds issue can be triggered.
> 
> Looking for suspects of sysctls which may end up empty in that patch we
> have a couple. kernel/sched/fair.c sched_fair_sysctls can end up empty when
> you don't have CONFIG_CFS_BANDWIDTH or CONFIG_NUMA_BALANCING. But the kernel
> config for the 0-day test was:                                                                                                                                                                         
> https://download.01.org/0day-ci/archive/20231120/202311201431.57aae8f3-oliver.sang@xxxxxxxxx/config-6.6.0-rc2-00030-g423f75083acd                                                           
> 
> It has CONFIG_CFS_BANDWIDTH=y but CONFIG_NUMA_BALANCING is not set so
> this was not the culprit, but that configuration could have been a
> cause if it was possible.
> 
> In the same future-not-upstream commit kernel/sched/core.c sched_core_sysctls
> can be empty if you don't have the following:                                                                                                                     
> 
> CONFIG_SCHEDSTATS       --> not set on 0-day kernel
> CONFIG_UCLAMP_TASK      --> not set on 0-day kernel                                                                                                                                         
> CONFIG_NUMA_BALANCING   --> not set on 0-day kernel
> 
> So that was the cause, and an example real valid config which can trigger
> a crash. But that patch was never upstream.
> 
> Now, we can look for existing removal of sysctl sentinels with:
> 
> git log -p --grep "superfluous sentinel element"
> 
> And of these, at first glance, only locks_sysctls seems like it *could*
> possibly end up in a situation where random config would create an empty
> sysctl, but exanding on the code we see that's not possible because
> leases-enable sysctl entry is always built if you have sysctls enabled:
> 
> static struct ctl_table llocks_sysctlsocks_sysctls[] = {
>         {
>                 .procname       = "leases-enable",
>                 .data           = &leases_enable,
>                 .maxlen         = sizeof(int),                                   
>                 .mode           = 0644,
>                 .proc_handler   = proc_dointvec,
>         },
> #ifdef CONFIG_MMU
>         {
>                 .procname       = "lease-break-time",
>                 .data           = &lease_break_time,                             
>                 .maxlen         = sizeof(int),
>                 .mode           = 0644,
>                 .proc_handler   = proc_dointvec,
>         },
> #endif /* CONFIG_MMU */
> };
> 
> The out of bounds fix commit should have just had the tag:
> 
> Fixes 9edbfe92a0a13 ("sysctl: Add size to register_sysctl")
> 
> Backporting this is fine, but wouldn't fix an issue unless an external
It is not only fine, I think it is necessary to avoid some other future
backported patch actually introducing an empty sysctl.

> module had empty sysctls. And exploiting this is not possible unless
> you purposely build an external module which could end up with empty
> sysctls.
This is exactly my conclusion.
Very nice summary Luis. Thx for putting it together

> 
> HTH,
> 
>   Luis


-- 

Joel Granados

Attachment: signature.asc
Description: PGP signature