Re: [PATCH v2] sysctl: move the extra1/2 boundary check of u8 to sysctl_check_table_array

[Joel Granados]

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

<--- 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_greater_int_max(
>  	KUNIT_EXPECT_EQ(test, 0, *((int *)table.data));
>  }
>  
> +/*
> + * 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 = 0;
> +	struct ctl_table table[] = {
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	= "foo",
> +			.data		= &data,
> +			.maxlen		= sizeof(u8),
> +			.mode		= 0644,
> +			.proc_handler	= proc_dou8vec_minmax,
> +			.extra1		= SYSCTL_ZERO,
> +			.extra2		= 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 = 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 = SYSCTL_ONE_THOUSAND;
> +	KUNIT_EXPECT_NULL(test, register_sysctl_sz("foo", table, size));
> +	table[0].extra1 = SYSCTL_ONE_HUNDRED;
> +	table[0].extra2 = 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[] = {
>  	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[] = {
>  	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),
>  	{}
>  };
>  
> 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, int write,
>  	if (table->maxlen != sizeof(u8))
>  		return -EINVAL;
>  
> -	if (table->extra1) {
> -		min = *(unsigned int *) table->extra1;
> -		if (min > 255U)
> -			return -EINVAL;
> -	}
> -	if (table->extra2) {
> -		max = *(unsigned int *) table->extra2;
> -		if (max > 255U)
> -			return -EINVAL;
> -	}
> +	if (table->extra1)
> +		min = *(unsigned char *) table->extra1;
> +	if (table->extra2)
> +		max = *(unsigned char *) table->extra2;
>  
>  	tmp = *table;
>  
> -- 
> 2.25.1
> 

-- 

Joel Granados

Attachment: signature.asc
Description: PGP signature