[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <91311377-4cb5-42a4-82fd-c30de56b1121@linux.dev>
Date: Wed, 6 Nov 2024 21:19:36 +0800
From: Wen Yang <wen.yang@...ux.dev>
To: Joel Granados <joel.granados@...nel.org>
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: [RESEND PATCH v3] sysctl: simplify the min/max boundary check
On 2024/10/31 17:39, Joel Granados wrote:
> 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
>
Thanks.
I will make the modifications according to your suggestions and send v4
soon.
--
Best wishes,
Wen
Powered by blists - more mailing lists