| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ce2na5wzbkpvrh4tccmrfwi5hukwjnrpkhnggdfgce7ccs5rvq@w2c76uttfxq3>
Date: Thu, 16 Jan 2025 10:37:47 +0100
From: Joel Granados <joel.granados@...nel.org>
To: Wen Yang <wen.yang@...ux.dev>
Cc: "Eric W . Biederman" <ebiederm@...ssion.com>,
Luis Chamberlain <mcgrof@...nel.org>, Kees Cook <keescook@...omium.org>,
Christian Brauner <brauner@...nel.org>, Dave Young <dyoung@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5] sysctl: simplify the min/max boundary check
On Sun, Jan 05, 2025 at 11:28:53PM +0800, Wen Yang wrote:
> do_proc_dointvec_conv() used the default range of int type, while
> do_proc_dointvec_minmax_conv() additionally used "int * {min, max}" of
> struct do_proc_dointvec_minmax_conv_param, which are actually passed
> in table->extra{1,2} pointers.
>
> By changing the struct do_proc_dointvec_minmax_conv_param's
> "int * {min, max}" to "int {min, max}" and setting the min/max value
> via dereference the table->extra{1, 2}, those do_proc_dointvec_conv() and
> do_proc_dointvec_minmax_conv() functions can be merged into one and reduce
> code by one-third.
>
> Similar changes were also made for do_proc_douintvec_minmax_conv_param,
> do_proc_douintvec_conv() and do_proc_douintvec_minmax_conv().
>
Before moving forward, I need the motivation.
Please answer this:
*Why* are you pushing these two [1], [2] series?
Best
[1]
* https://lore.kernel.org/tencent_A7FD9E9E75AA74A87583A427F5D20F988B09@qq.com
* https://lore.kernel.org/tencent_C5E6023F97E7CC2A046AAEA09BC9ACF43907@qq.com
* https://lore.kernel.org/cover.1726365007.git.wen.yang@linux.dev
* https://lore.kernel.org/cover.1726910671.git.wen.yang@linux.dev
[2]
* https://lore.kernel.org/20240714173858.52163-1-wen.yang@linux.dev
* https://lore.kernel.org/20240715150223.54707-1-wen.yang@linux.dev
* https://lore.kernel.org/20240905134818.4104-1-wen.yang@linux.dev
* https://lore.kernel.org/20241201140058.5653-1-wen.yang@linux.dev
> Selftest were also made:
>
> [23:16:38] ================ sysctl_test (11 subtests) =================
> [23:16:38] [PASSED] sysctl_test_api_dointvec_null_tbl_data
> [23:16:38] [PASSED] sysctl_test_api_dointvec_table_maxlen_unset
> [23:16:38] [PASSED] sysctl_test_api_dointvec_table_len_is_zero
> [23:16:38] [PASSED] sysctl_test_api_dointvec_table_read_but_position_set
> [23:16:38] [PASSED] sysctl_test_dointvec_read_happy_single_positive
> [23:16:38] [PASSED] sysctl_test_dointvec_read_happy_single_negative
> [23:16:38] [PASSED] sysctl_test_dointvec_write_happy_single_positive
> [23:16:38] [PASSED] sysctl_test_dointvec_write_happy_single_negative
> [23:16:38] [PASSED] sysctl_test_api_dointvec_write_single_less_int_min
> [23:16:38] [PASSED] sysctl_test_api_dointvec_write_single_greater_int_max
> [23:16:38] [PASSED] sysctl_test_register_sysctl_sz_invalid_extra_value
> [23:16:38] =================== [PASSED] sysctl_test ===================
> [23:16:38] ============================================================
> [23:16:38] Testing complete. Ran 11 tests: passed: 11
>
> Signed-off-by: Wen Yang <wen.yang@...ux.dev>
> Cc: Joel Granados <joel.granados@...nel.org>
> Cc: Luis Chamberlain <mcgrof@...nel.org>
> Cc: Kees Cook <keescook@...omium.org>
> Cc: Eric W. Biederman <ebiederm@...ssion.com>
> Cc: Christian Brauner <brauner@...nel.org>
> Cc: Dave Young <dyoung@...hat.com>
> Cc: linux-kernel@...r.kernel.org
> ---
> kernel/sysctl.c | 211 ++++++++++++++++++++----------------------------
> 1 file changed, 86 insertions(+), 125 deletions(-)
>
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 7ae7a4136855..250dc9057259 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -424,22 +424,44 @@ static void proc_put_char(void **buf, size_t *size, char c)
> }
> }
>
> -static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp,
> - int *valp,
> - int write, void *data)
> +/**
> + * struct do_proc_dointvec_minmax_conv_param - proc_dointvec_minmax() range checking structure
> + * @min: the minimum allowable value
> + * @max: the maximum allowable value
> + *
> + * The do_proc_dointvec_minmax_conv_param structure provides the
> + * minimum and maximum values for doing range checking for those sysctl
> + * parameters that use the proc_dointvec_minmax(), proc_dou8vec_minmax() and so on.
> + */
> +struct do_proc_dointvec_minmax_conv_param {
> + int min;
> + int max;
> +};
> +
> +static inline int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
> + int *valp,
> + int write, void *data)
> {
> + struct do_proc_dointvec_minmax_conv_param *param = data;
> + long min, max;
> + long val;
> +
> if (write) {
> if (*negp) {
> - if (*lvalp > (unsigned long) INT_MAX + 1)
> - return -EINVAL;
> - WRITE_ONCE(*valp, -*lvalp);
> + max = param ? param->max : 0;
> + min = param ? param->min : INT_MIN;
> + val = -*lvalp;
> } else {
> - if (*lvalp > (unsigned long) INT_MAX)
> - return -EINVAL;
> - WRITE_ONCE(*valp, *lvalp);
> + max = param ? param->max : INT_MAX;
> + min = param ? param->min : 0;
> + val = *lvalp;
> }
> +
> + if (val > max || val < min)
> + return -EINVAL;
> + WRITE_ONCE(*valp, (int)val);
> } else {
> - int val = READ_ONCE(*valp);
> + val = (int)READ_ONCE(*valp);
> if (val < 0) {
> *negp = true;
> *lvalp = -(unsigned long)val;
> @@ -448,21 +470,44 @@ static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp,
> *lvalp = (unsigned long)val;
> }
> }
> +
> return 0;
> }
>
> -static int do_proc_douintvec_conv(unsigned long *lvalp,
> - unsigned int *valp,
> - int write, void *data)
> +/**
> + * struct do_proc_douintvec_minmax_conv_param - proc_douintvec_minmax() range checking structure
> + * @min: minimum allowable value
> + * @max: maximum allowable value
> + *
> + * The do_proc_douintvec_minmax_conv_param structure provides the
> + * minimum and maximum values for doing range checking for those sysctl
> + * parameters that use the proc_douintvec_minmax() handler.
> + */
> +struct do_proc_douintvec_minmax_conv_param {
> + unsigned int min;
> + unsigned int max;
> +};
> +
> +static inline int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
> + unsigned int *valp,
> + int write, void *data)
> {
> + struct do_proc_douintvec_minmax_conv_param *param = data;
> + unsigned long min, max;
> +
> if (write) {
> - if (*lvalp > UINT_MAX)
> + max = param ? param->max : UINT_MAX;
> + min = param ? param->min : 0;
> +
> + if (*lvalp > max || *lvalp < min)
> return -EINVAL;
> - WRITE_ONCE(*valp, *lvalp);
> +
> + WRITE_ONCE(*valp, (unsigned int)*lvalp);
> } else {
> unsigned int val = READ_ONCE(*valp);
> *lvalp = (unsigned long)val;
> }
> +
> return 0;
> }
>
> @@ -489,7 +534,7 @@ static int __do_proc_dointvec(void *tbl_data, const struct ctl_table *table,
> left = *lenp;
>
> if (!conv)
> - conv = do_proc_dointvec_conv;
> + conv = do_proc_dointvec_minmax_conv;
>
> if (write) {
> if (proc_first_pos_non_zero_ignore(ppos, table))
> @@ -667,7 +712,7 @@ static int __do_proc_douintvec(void *tbl_data, const struct ctl_table *table,
> }
>
> if (!conv)
> - conv = do_proc_douintvec_conv;
> + conv = do_proc_douintvec_minmax_conv;
>
> if (write)
> return do_proc_douintvec_w(i, table, buffer, lenp, ppos,
> @@ -762,7 +807,7 @@ int proc_douintvec(const struct ctl_table *table, int write, void *buffer,
> size_t *lenp, loff_t *ppos)
> {
> return do_proc_douintvec(table, write, buffer, lenp, ppos,
> - do_proc_douintvec_conv, NULL);
> + do_proc_douintvec_minmax_conv, NULL);
> }
>
> /*
> @@ -808,46 +853,6 @@ static int proc_taint(const struct ctl_table *table, int write,
> return err;
> }
>
> -/**
> - * struct do_proc_dointvec_minmax_conv_param - proc_dointvec_minmax() range checking structure
> - * @min: pointer to minimum allowable value
> - * @max: pointer to maximum allowable value
> - *
> - * The do_proc_dointvec_minmax_conv_param structure provides the
> - * minimum and maximum values for doing range checking for those sysctl
> - * parameters that use the proc_dointvec_minmax() handler.
> - */
> -struct do_proc_dointvec_minmax_conv_param {
> - int *min;
> - int *max;
> -};
> -
> -static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
> - int *valp,
> - int write, void *data)
> -{
> - int tmp, ret;
> - struct do_proc_dointvec_minmax_conv_param *param = data;
> - /*
> - * If writing, first do so via a temporary local int so we can
> - * bounds-check it before touching *valp.
> - */
> - int *ip = write ? &tmp : valp;
> -
> - ret = do_proc_dointvec_conv(negp, lvalp, ip, write, data);
> - if (ret)
> - return ret;
> -
> - if (write) {
> - if ((param->min && *param->min > tmp) ||
> - (param->max && *param->max < tmp))
> - return -EINVAL;
> - WRITE_ONCE(*valp, tmp);
> - }
> -
> - return 0;
> -}
> -
> /**
> * proc_dointvec_minmax - read a vector of integers with min/max values
> * @table: the sysctl table
> @@ -867,53 +872,14 @@ static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
> int proc_dointvec_minmax(const struct ctl_table *table, int write,
> void *buffer, size_t *lenp, loff_t *ppos)
> {
> - struct do_proc_dointvec_minmax_conv_param param = {
> - .min = (int *) table->extra1,
> - .max = (int *) table->extra2,
> - };
> + struct do_proc_dointvec_minmax_conv_param param;
> +
> + param.min = (table->extra1) ? *(int *) table->extra1 : INT_MIN;
> + param.max = (table->extra2) ? *(int *) table->extra2 : INT_MAX;
> return do_proc_dointvec(table, write, buffer, lenp, ppos,
> do_proc_dointvec_minmax_conv, ¶m);
> }
>
> -/**
> - * struct do_proc_douintvec_minmax_conv_param - proc_douintvec_minmax() range checking structure
> - * @min: pointer to minimum allowable value
> - * @max: pointer to maximum allowable value
> - *
> - * The do_proc_douintvec_minmax_conv_param structure provides the
> - * minimum and maximum values for doing range checking for those sysctl
> - * parameters that use the proc_douintvec_minmax() handler.
> - */
> -struct do_proc_douintvec_minmax_conv_param {
> - unsigned int *min;
> - unsigned int *max;
> -};
> -
> -static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
> - unsigned int *valp,
> - int write, void *data)
> -{
> - int ret;
> - unsigned int tmp;
> - struct do_proc_douintvec_minmax_conv_param *param = data;
> - /* write via temporary local uint for bounds-checking */
> - unsigned int *up = write ? &tmp : valp;
> -
> - ret = do_proc_douintvec_conv(lvalp, up, write, data);
> - if (ret)
> - return ret;
> -
> - if (write) {
> - if ((param->min && *param->min > tmp) ||
> - (param->max && *param->max < tmp))
> - return -ERANGE;
> -
> - WRITE_ONCE(*valp, tmp);
> - }
> -
> - return 0;
> -}
> -
> /**
> * proc_douintvec_minmax - read a vector of unsigned ints with min/max values
> * @table: the sysctl table
> @@ -936,10 +902,11 @@ static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
> int proc_douintvec_minmax(const struct ctl_table *table, int write,
> void *buffer, size_t *lenp, loff_t *ppos)
> {
> - struct do_proc_douintvec_minmax_conv_param param = {
> - .min = (unsigned int *) table->extra1,
> - .max = (unsigned int *) table->extra2,
> - };
> + struct do_proc_douintvec_minmax_conv_param param;
> +
> + param.min = (table->extra1) ? *(unsigned int *) table->extra1 : 0;
> + param.max = (table->extra2) ? *(unsigned int *) table->extra2 : UINT_MAX;
> +
> return do_proc_douintvec(table, write, buffer, lenp, ppos,
> do_proc_douintvec_minmax_conv, ¶m);
> }
> @@ -965,23 +932,17 @@ int proc_dou8vec_minmax(const struct ctl_table *table, int write,
> void *buffer, size_t *lenp, loff_t *ppos)
> {
> struct ctl_table tmp;
> - unsigned int min = 0, max = 255U, val;
> + unsigned int val;
> u8 *data = table->data;
> - struct do_proc_douintvec_minmax_conv_param param = {
> - .min = &min,
> - .max = &max,
> - };
> + struct do_proc_douintvec_minmax_conv_param param;
> int res;
>
> /* Do not support arrays yet. */
> if (table->maxlen != sizeof(u8))
> return -EINVAL;
>
> - if (table->extra1)
> - min = *(unsigned int *) table->extra1;
> - if (table->extra2)
> - max = *(unsigned int *) table->extra2;
> -
> + param.min = (table->extra1) ? *(unsigned int *) table->extra1 : 0;
> + param.max = (table->extra2) ? *(unsigned int *) table->extra2 : 255U;
> tmp = *table;
>
> tmp.maxlen = sizeof(val);
> @@ -1022,7 +983,7 @@ static int __do_proc_doulongvec_minmax(void *data,
> void *buffer, size_t *lenp, loff_t *ppos,
> unsigned long convmul, unsigned long convdiv)
> {
> - unsigned long *i, *min, *max;
> + unsigned long *i, min, max;
> int vleft, first = 1, err = 0;
> size_t left;
> char *p;
> @@ -1033,8 +994,9 @@ static int __do_proc_doulongvec_minmax(void *data,
> }
>
> i = data;
> - min = table->extra1;
> - max = table->extra2;
> + min = (table->extra1) ? *(unsigned long *) table->extra1 : 0;
> + max = (table->extra2) ? *(unsigned long *) table->extra2 : ULONG_MAX;
> +
> vleft = table->maxlen / sizeof(unsigned long);
> left = *lenp;
>
> @@ -1066,7 +1028,7 @@ static int __do_proc_doulongvec_minmax(void *data,
> }
>
> val = convmul * val / convdiv;
> - if ((min && val < *min) || (max && val > *max)) {
> + if ((val < min) || (val > max)) {
> err = -EINVAL;
> break;
> }
> @@ -1236,8 +1198,7 @@ static int do_proc_dointvec_ms_jiffies_minmax_conv(bool *negp, unsigned long *lv
> return ret;
>
> if (write) {
> - if ((param->min && *param->min > tmp) ||
> - (param->max && *param->max < tmp))
> + if ((param->min > tmp) || (param->max < tmp))
> return -EINVAL;
> *valp = tmp;
> }
> @@ -1269,10 +1230,10 @@ int proc_dointvec_jiffies(const struct ctl_table *table, int write,
> int proc_dointvec_ms_jiffies_minmax(const struct ctl_table *table, int write,
> void *buffer, size_t *lenp, loff_t *ppos)
> {
> - struct do_proc_dointvec_minmax_conv_param param = {
> - .min = (int *) table->extra1,
> - .max = (int *) table->extra2,
> - };
> + struct do_proc_dointvec_minmax_conv_param param;
> +
> + param.min = (table->extra1) ? *(int *) table->extra1 : INT_MIN;
> + param.max = (table->extra2) ? *(int *) table->extra2 : INT_MAX;
> return do_proc_dointvec(table, write, buffer, lenp, ppos,
> do_proc_dointvec_ms_jiffies_minmax_conv, ¶m);
> }
> --
> 2.25.1
>
--
Joel Granados
Powered by blists - more mailing lists