| 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: <7cuqiavbm3vhdnpwumknb6eog4r73w7c2tcivwkvjer3a6hrwc@4zu7d5bng4wj>
Date: Wed, 18 Dec 2024 15:11: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: [RESEND PATCH v4] sysctl: simplify the min/max boundary check
On Sun, Dec 01, 2024 at 10:00:58PM +0800, Wen Yang wrote:
> The do_proc_dointvec_minmax_conv_param structure provides the minimum and
> maximum values for doing range checking for the proc_dointvec_minmax()
> handler, however min/max is a pointer and may be NULL, so the following
> code snippet appears multiple times:
> if ((param->min && *param->min > tmp) ||
> (param->max && *param->max < tmp))
>
> And int types also have min/max values, so when the pointer is NULL,
> explicitly setting min/max to INT_{MIN/MAX} could simplify the code a bit.
Sorry, but this does not make sense to me. This simplification is way
too small and it seems that it is just being done for the sake of it.
Additionally, by giving these min/max values you are potentially
changing the behaviour of existing calls, which is concerning for
such a small change/gain.
Thx for the contribution but will stay with the pointers for now.
Best
>
> Similar changes were also made for do_proc_douintvec_minmax_conv_param.
>
> 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 | 75 ++++++++++++++++++++++---------------------------
> 1 file changed, 34 insertions(+), 41 deletions(-)
>
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 79e6cb1d5c48..47e2fe4fe978 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -810,16 +810,16 @@ static int proc_taint(const struct ctl_table *table, int write,
>
> /**
> * 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
> + * @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() handler.
> + * parameters that use the proc_dointvec_minmax(), proc_dou8vec_minmax() and so on.
> */
> struct do_proc_dointvec_minmax_conv_param {
> - int *min;
> - int *max;
> + int min;
> + int max;
> };
>
> static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
> @@ -839,8 +839,7 @@ static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
> return ret;
>
> if (write) {
> - if ((param->min && *param->min > tmp) ||
> - (param->max && *param->max < tmp))
> + if ((param->min > tmp) || (param->max < tmp))
> return -EINVAL;
> WRITE_ONCE(*valp, tmp);
> }
> @@ -867,26 +866,26 @@ 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
> + * @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;
> + unsigned int min;
> + unsigned int max;
> };
>
> static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
> @@ -904,8 +903,7 @@ static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
> return ret;
>
> if (write) {
> - if ((param->min && *param->min > tmp) ||
> - (param->max && *param->max < tmp))
> + if ((param->min > tmp) || (param->max < tmp))
> return -ERANGE;
>
> WRITE_ONCE(*valp, tmp);
> @@ -936,10 +934,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 +964,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 +1015,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 +1026,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 +1060,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 +1230,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 +1262,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