[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4rrwkbj5sh4anblrxzhehcir2z2w5qhrdxfu4gc4irfg4ubb7q@hjt3e6agz42i>
Date: Thu, 31 Oct 2024 10:39:39 +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>,
Joel Granados <j.granados@...sung.com>, Christian Brauner <brauner@...nel.org>,
Dave Young <dyoung@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: Re: [RESEND PATCH v3] sysctl: simplify the min/max boundary check
On Wed, Oct 30, 2024 at 12:26:17AM +0800, Wen Yang wrote:
>
>
> On 2024/10/23 03:12, Joel Granados wrote:
> > On Thu, Sep 05, 2024 at 09:48:18PM +0800, Wen Yang wrote:
...
> >> @@ -936,10 +921,10 @@ 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 proc_minmax_conv_param param;
> >> +
> >> + param.min = (table->extra1) ? *(unsigned int *) table->extra1 : 0;
> >> + param.max = (table->extra2) ? *(unsigned int *) table->extra2 : UINT_MAX;
> > This is one of the cases where there is potential issues. Here, if the
> > value of table->extra{1,2}'s value is greater than when
> > the maximum value of a signed long, then the value assigned would be
> > incorrect. Note that the problem does not go away if you remove the
> > "unsigned" qualifier; it remains if table->extra{1,2} are originally
> > unsigned.
> >
>
> I set up a CentOS 7.9 32-bit VM on Virtuanbox:
> # uname -a
> Linux osboxes.org 3.10.0-1160.2.2.el7.centos.plus.i686 #1 SMP Mon Oct 26
> 11:56:29 UTC 2020 i686 i686 i386 GNU/Linux
>
> And the following test code:
>
> #include <stdio.h>
> #include <stdlib.h>
>
> int main()
> {
> unsigned int i = 4294967294;
> long j = i;
>
> printf("original hex(i) = 0x%x\n", i);
> printf("unsigned int(i) = %lu\n", i);
> printf("---------------------\n");
> printf("hex(j) = 0x%x\n", j);
> printf("long(j) = %ld\n", j);
> printf("unsigned long(j) = %lu\n", j);
> printf("int(j) = %d\n", j);
> printf("unsigned int(j) = %lu\n", j);
> return 0;
> }
>
>
> ./a.out
>
> original hex(i) = 0xfffffffe
> unsigned int(i) = 4294967294
> ---------------------
> hex(j) = 0xfffffffe
> long(j) = -2
This ^^^^^ is exactly what I expected. Thx for the test!
When you transfer that to your patch, it means that for certain cases
[1] the value resulting from the interpretation of param.{min,max}
(signed long) is going to be different than the value resulting from the
interpretation of table-extra{1,2} (unsigned int).
Here is another way of thinking about it:
We are avoiding bugs where a developer thinks they are handling longs,
when in reality they are handling unsinged ints; The result of
subtracting 1 from (-2) is very different from subtracting 1 from
4294967294.
> unsigned long(j) = 4294967294
> int(j) = -2
> unsigned int(j) = 4294967294
>
>
> The original hexadecimal values are the same, using unsigned int, int,
> unsigned long, or long is just interpreted in different ways.
Exactly. Hex remains the same but the interpretation changes. And it is
there where pain lies.
Please re-work the patch without merging everything into
do_proc_douintvec_minmax_conv_param
>
> We also ensure consistency in numerical writing and type conversion in
> the patch. For example, in proc_rointvec_jiffies, convert to int; And in
> proc_rouintvec_minmax, it is converted to unsigned int.
>
>
> --
> Best wishes,
> Wen
>
>
> > I'm not sure if there are more, but just having one of these things
> > around make me uncomfortable. Please re-work the patch in order to
> > remove this issue in order to continue review.
> >
> > best
> >
Best
[1] This is one case that I can imagine
1. On 32 bit architectures
2. Where UINT_MAX > LONG_MAX
3. The value being assigned is greater LONG_MAX
--
Joel Granados
Powered by blists - more mailing lists