| 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: <swc4pmfezm7dyttpi55g24yyrh2obfermxlb3fy7atrlvvl3g4@gjsxredtsi4m>
Date: Thu, 16 Jan 2025 09:53:22 +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: Re: [RESEND PATCH v4] sysctl: simplify the min/max boundary check
On Thu, Jan 02, 2025 at 12:26:36AM +0800, Wen Yang wrote:
>
>
> On 2024/12/18 22:11, Joel Granados wrote:
> > 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.
> >
I'm answering this one late because you sent a V5.
>
> Thanks for your comments.
>
> The implementation of do_proc_dointvec_conv() utilizes the default range of
> the int type, while do_proc_dointvec_minmax_conv() additionally utilizes
> min/max pointers, which are actually table->extra {1,2} pointers passed in.
>
> If we can dereference the table->extra {1,2} pointers to numerical values in
> advance (such as the modification here), we can take advantage of memory
> locality and improve performance a bit.
1. Locality for performance? What is your use case here? These are
functions that are usually called once. Is there some hot path
through the sysctls that I'm not aware of?
2. Please clarify what do you want to accomplish? It seem like you keep jumping:
- "Simplify code a bit"
- "Locality"
- "Avoid duplicate code"
- "prepare for removing extra{1,2}
>
> If the current simplification is too small, we could further improve it,
> such as considering do_proc_minmax_conv_param, do_proc_minmax_conv_param,
> do_proc_dointvec_minmax_conv, do_proc_douintvec_conv,
> do_proc_douintvec_minmax_conv , etc.
I did not mean "send a bigger simplification"; sorry if I did not
explain myself. I meant that I do *not* see any gain in removing lines
of code for this particular case.
>
> All of this is in preparation for ultimately killing the table ->{extra1,
> extra2} pointers.
I am curious about this one because I think this is what you ultimately
want to do. But I'm still not clear on why. Is it to save memory?
>
> We will rework later and send v5.
I see that you have already sent it. I'll add further comments to that
one.
Best
>
> --
> Best wishes，
> Wen
>
>
> > 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 ram {
> > > - 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