Message-ID: <20240312091730.GU86322@google.com>
Date: Tue, 12 Mar 2024 09:17:30 +0000
From: Lee Jones <lee@...nel.org>
To: Luis Chamberlain <mcgrof@...nel.org>
Cc: Michal Hocko <mhocko@...e.com>, cve@...nel.org,
	linux-kernel@...r.kernel.org,
	Joel Granados <j.granados@...sung.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: CVE-2023-52596: sysctl: Fix out of bounds access for empty
 sysctl registers

On Mon, 11 Mar 2024, 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@intel.com/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
> module had empty sysctls. And exploiting this is not possible unless
> you purposely build an external module which could end up with empty
> sysctls.

Thanks for the amazing explanation Luis.

If I'm reading this correctly, an issue does exist, but an attacker
would have to lay some foundations before it could be triggered.  Sounds
like loading of a malicious or naive module would be enough.

We know from conducting postmortems on previous exploits that successful
attacks often consist of leveraging a chain of smaller, seemingly
implausible or innocuous looking bugs rather than in isolation.

With that in mind, it is still my belief that this could be used by an
attacker in such a chain.  Unless I have this totally wrong or any of
the maintainers have strong feelings to the contrary, I would like to
keep the CVE number associated with this fix.

-- 
Lee Jones [李琼斯]

Powered by blists - more mailing lists