Received-SPF: pass (google.com: domain of linux-kernel+bounces-123538-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 28 Mar 2024 17:53:41 +0100
From: Joel Granados <j.granados@samsung.com>
To: <wenyang.linux@foxmail.com>
CC: Luis Chamberlain <mcgrof@kernel.org>, Kees Cook <keescook@chromium.org>,
	Christian Brauner <brauner@kernel.org>, "Eric W . Biederman"
	<ebiederm@xmission.com>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] sysctl: move the extra1/2 boundary check of u8 to
 sysctl_check_table_array
Message-ID: <20240328165341.3ov54gwjymghtp2g@joelS2.panther.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg="pgp-sha512";
	protocol="application/pgp-signature"; boundary="o5jgyvl67e7knwjj"
Content-Disposition: inline
In-Reply-To: <tencent_8599CCEFED19AF4A8A5A358862B226FD8A05@qq.com>
CMS-TYPE: 201P
References: <CGME20240302152805eucas1p22ac49edeee0be22bcf5562daeba11716@eucas1p2.samsung.com>
	<tencent_8599CCEFED19AF4A8A5A358862B226FD8A05@qq.com>

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

On Sat, Mar 02, 2024 at 11:27:45PM +0800, wenyang.linux@foxmail.com wrote:
> From: Wen Yang <wenyang.linux@foxmail.com>
>=20

<--- snip --->

> diff --git a/kernel/sysctl-test.c b/kernel/sysctl-test.c
> index 6ef887c19c48..84e759a8328f 100644
> --- a/kernel/sysctl-test.c
> +++ b/kernel/sysctl-test.c
> @@ -367,6 +367,35 @@ static void sysctl_test_api_dointvec_write_single_gr=
eater_int_max(
>  	KUNIT_EXPECT_EQ(test, 0, *((int *)table.data));
>  }
> =20
> +/*
> + * Test that registering an invalid extra value is not allowed.
> + */
> +static void sysctl_test_register_sysctl_sz_invalid_extra_value(
> +		struct kunit *test)
> +{
> +	unsigned char data =3D 0;
> +	struct ctl_table table[] =3D {
I'm pretty sure that this is going to clash with the constification that
Thomas is working on. Please re-work this patch knowing that these
ctl_tables are going to have to change to const at some point.

> +		{
> +			.procname	=3D "foo",
> +			.data		=3D &data,
> +			.maxlen		=3D sizeof(u8),
> +			.mode		=3D 0644,
> +			.proc_handler	=3D proc_dou8vec_minmax,
> +			.extra1		=3D SYSCTL_ZERO,
> +			.extra2		=3D SYSCTL_ONE_THOUSAND,
> +		},
> +		{}
Don't use the sentinel here. We are removing them and all new sysctl
tables (even the test ones) should be created without them

> +	};
> +	unsigned int size =3D ARRAY_SIZE(table);
You do not need size here. When you use register_sysctl, the size will
be automatically calculated for you.

> +
> +	KUNIT_EXPECT_NULL(test, register_sysctl_sz("foo", table, size));
> +	table[0].extra1 =3D SYSCTL_ONE_THOUSAND;
> +	KUNIT_EXPECT_NULL(test, register_sysctl_sz("foo", table, size));
> +	table[0].extra1 =3D SYSCTL_ONE_HUNDRED;
> +	table[0].extra2 =3D SYSCTL_TWO_HUNDRED;
> +	KUNIT_EXPECT_NOT_NULL(test, register_sysctl_sz("foo", table, size));
Replace all these by register_sysctl please.

> +}
> +
>  static struct kunit_case sysctl_test_cases[] =3D {
>  	KUNIT_CASE(sysctl_test_api_dointvec_null_tbl_data),
>  	KUNIT_CASE(sysctl_test_api_dointvec_table_maxlen_unset),
> @@ -378,6 +407,7 @@ static struct kunit_case sysctl_test_cases[] =3D {
>  	KUNIT_CASE(sysctl_test_dointvec_write_happy_single_negative),
>  	KUNIT_CASE(sysctl_test_api_dointvec_write_single_less_int_min),
>  	KUNIT_CASE(sysctl_test_api_dointvec_write_single_greater_int_max),
> +	KUNIT_CASE(sysctl_test_register_sysctl_sz_invalid_extra_value),
>  	{}
>  };
> =20
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index f67b39d3d6e5..28888744626a 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -977,16 +977,10 @@ int proc_dou8vec_minmax(struct ctl_table *table, in=
t write,
>  	if (table->maxlen !=3D sizeof(u8))
>  		return -EINVAL;
> =20
> -	if (table->extra1) {
> -		min =3D *(unsigned int *) table->extra1;
> -		if (min > 255U)
> -			return -EINVAL;
> -	}
> -	if (table->extra2) {
> -		max =3D *(unsigned int *) table->extra2;
> -		if (max > 255U)
> -			return -EINVAL;
> -	}
> +	if (table->extra1)
> +		min =3D *(unsigned char *) table->extra1;
> +	if (table->extra2)
> +		max =3D *(unsigned char *) table->extra2;
> =20
>  	tmp =3D *table;
> =20
> --=20
> 2.25.1
>=20

--=20

Joel Granados

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

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

iQGzBAABCgAdFiEErkcJVyXmMSXOyyeQupfNUreWQU8FAmYFoJUACgkQupfNUreW
QU8CjQv+Pgocvz4Gd33upP9+RQKbhhX72UVKZFRIIwEViy/M93mkHVAH/OzSwA6x
TMht01qmi2ZtFJjUM4N6WdHnzLtflrDKLqUasdOjvF5ufrBKxa0dj548Xfqg4xHi
06vRchhZbfaRK3Sj5F3E4X3gNAkgY0sfz/ugRw7ZUAx0logTh8lxk/7Yz7kyY/fN
AZGL/3ITwopK/7hb8csHFLndg94LGXTgpvj9yZtql+cKfgRng2AU2MGISUkSJpz1
QBkLDP1Njp48t+YeY5oN4qyeFihD6v/ZaIpGBUFsluVCThMy5I0OEg27O3s++nZX
PTdjkxOR57S1cvn6vlTXgjDgUy8AS5QlxzP2gKPcuhdMdIyGFHFliftG70GnFmk2
05JIeM1HEGxEM+UMgxSsnsEilZoegTZ/KrYMdLJ62fIYdlcaZucCVZhf/BHVoRtA
yTWvNlY3mu0t5uTJwkdDMdPnmnJC1oAtLqGp7PXSY/7s4y3c9M5hG4DPjpK+FPsR
6kWYpr5x
=kGut
-----END PGP SIGNATURE-----

--o5jgyvl67e7knwjj--