Received-SPF: pass (google.com: domain of linux-kernel+bounces-100166-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 12 Mar 2024 12:20:05 +0100
From: Joel Granados <j.granados@samsung.com>
To: Luis Chamberlain <mcgrof@kernel.org>
CC: Michal Hocko <mhocko@suse.com>, <cve@kernel.org>,
	<linux-kernel@vger.kernel.org>, Greg Kroah-Hartman
	<gregkh@linuxfoundation.org>
Subject: Re: CVE-2023-52596: sysctl: Fix out of bounds access for empty
 sysctl registers
Message-ID: <20240312112005.u6t3k7fe5pom24yw@joelS2.panther.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg="pgp-sha512";
	protocol="application/pgp-signature"; boundary="xi7cclg6zd773sge"
Content-Disposition: inline
In-Reply-To: <Ze9-Xmn8v4P_wppv@bombadil.infradead.org>
CMS-TYPE: 201P
References: <2024030645-CVE-2023-52596-b98e@gregkh>
	<Ze68r7_aTn6Vjbpj@tiehlicka>
	<CGME20240311215755eucas1p112332a9a874672135c14edb722130ad4@eucas1p1.samsung.com>
	<Ze9-Xmn8v4P_wppv@bombadil.infradead.org>

--xi7cclg6zd773sge
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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
> > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> > >=20
> > > In the Linux kernel, the following vulnerability has been resolved:
> > >=20
> > > sysctl: Fix out of bounds access for empty sysctl registers
> > >=20
> > > When registering tables to the sysctl subsystem there is a check to s=
ee
> > > if header is a permanently empty directory (used for mounts). This ch=
eck
> > > evaluates the first element of the ctl_table. This results in an out =
of
> > > bounds evaluation when registering empty directories.
> > >=20
> > > The function register_sysctl_mount_point now passes a ctl_table of si=
ze
> > > 1 instead of size 0. It now relies solely on the type to identify
> > > a permanently empty register.
> > >=20
> > > Make sure that the ctl_table has at least one element before testing =
for
> > > permanent emptiness.
> >=20
> > While this makes the code more robust and more future proof I do not th=
ink
> > 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.
> >=20
> > Luis, Joel, what do you think?
>=20
> As per review with Joel:
>=20
> 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.
>=20
> The 0-day reported crash was for a future out of tree patch which was
> never merged yet:
>=20
> https://github.com/Joelgranados/linux/commit/423f75083acd947804c8d5c31ad1=
e1b5fcb3c020                                                               =
                                       =20
>=20
> Let's examine this commit to see why it triggers a crash to understand
> how the out of bounds issue can be triggered.
>=20
> 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 wh=
en
> you don't have CONFIG_CFS_BANDWIDTH or CONFIG_NUMA_BALANCING. But the ker=
nel
> config for the 0-day test was:                                           =
                                                                           =
                                                  =20
> https://download.01.org/0day-ci/archive/20231120/202311201431.57aae8f3-ol=
iver.sang@intel.com/config-6.6.0-rc2-00030-g423f75083acd                   =
                                       =20
>=20
> It has CONFIG_CFS_BANDWIDTH=3Dy 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.
>=20
> In the same future-not-upstream commit kernel/sched/core.c sched_core_sys=
ctls
> can be empty if you don't have the following:                            =
                                                                           =
             =20
>=20
> CONFIG_SCHEDSTATS       --> not set on 0-day kernel
> CONFIG_UCLAMP_TASK      --> not set on 0-day kernel                      =
                                                                           =
                                       =20
> CONFIG_NUMA_BALANCING   --> not set on 0-day kernel
>=20
> So that was the cause, and an example real valid config which can trigger
> a crash. But that patch was never upstream.
>=20
> Now, we can look for existing removal of sysctl sentinels with:
>=20
> git log -p --grep "superfluous sentinel element"
>=20
> 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:
>=20
> static struct ctl_table llocks_sysctlsocks_sysctls[] =3D {
>         {
>                 .procname       =3D "leases-enable",
>                 .data           =3D &leases_enable,
>                 .maxlen         =3D sizeof(int),                         =
         =20
>                 .mode           =3D 0644,
>                 .proc_handler   =3D proc_dointvec,
>         },
> #ifdef CONFIG_MMU
>         {
>                 .procname       =3D "lease-break-time",
>                 .data           =3D &lease_break_time,                   =
         =20
>                 .maxlen         =3D sizeof(int),
>                 .mode           =3D 0644,
>                 .proc_handler   =3D proc_dointvec,
>         },
> #endif /* CONFIG_MMU */
> };
>=20
> The out of bounds fix commit should have just had the tag:
>=20
> Fixes 9edbfe92a0a13 ("sysctl: Add size to register_sysctl")
>=20
> 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

>=20
> HTH,
>=20
>   Luis


--=20

Joel Granados

--xi7cclg6zd773sge
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQGzBAABCgAdFiEErkcJVyXmMSXOyyeQupfNUreWQU8FAmXwOmQACgkQupfNUreW
QU+Cfgv/RlgmVAWF6PFmsVJYBKzD8AEAUxMsRWCHlA6QlS8pgCoYGs98klDUxxz4
7hdClzTTQfPBlP+Ym9Zz96CarGidjQyjwlyUHkACTxZrxmAs+WZXmX2DXRpBrDVl
iJ+hJfMMKw9HRGcAa+IK4d2StMsxljMxGjK28zTJeNrDLuXFOzpqYIostV2Fn5+F
8TDTXlBC6WdP16hni/9r8lxdwjhbsA6OctlamDsYiQOBkdbnnle/fOiRd1hXHxE3
9fkH7flAaC4UTOieI0lLgBj97NG6vi1iZ4aLuRcUCYNwK9Vf6vUDjcOt1hif2jlG
fKljzov0p6sltfl7vJoWHE5PhT/x5jZK/N4AF1o+3/UoE++foNPiYD8qLpNwISNJ
2t4EB0raC/ELQeVgyhnikur3lTOKLLto9nI9CyZsgXyo6QykSxJYQLpzD8ZQ6lI4
hQLg3+LGZKAC/Vu2GG2wX/hYHLKMiOCUahTvZgNnDa1tqQA+79ag/Z4xT2jXbm3m
DzF+R2/8
=AfvB
-----END PGP SIGNATURE-----

--xi7cclg6zd773sge--