linux-kernel - Re: [PATCH v2 3/4] kstrtox: add unit tests for memparse

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a13d0f90-58bd-45cb-bf4e-880880f54a32@suse.com>
Date: Wed, 3 Jan 2024 20:15:27 +1030
From: Qu Wenruo <wqu@...e.com>
To: Geert Uytterhoeven <geert@...ux-m68k.org>,
 Qu Wenruo <quwenruo.btrfs@....com>
Cc: linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org,
 akpm@...ux-foundation.org, christophe.jaillet@...adoo.fr,
 andriy.shevchenko@...ux.intel.com, David.Laight@...lab.com, ddiss@...e.de
Subject: Re: [PATCH v2 3/4] kstrtox: add unit tests for memparse_safe()



On 2024/1/3 19:57, Geert Uytterhoeven wrote:
> Hi Qu,
> 
> On Tue, Jan 2, 2024 at 9:56 PM Qu Wenruo <quwenruo.btrfs@....com> wrote:
>> On 2024/1/2 23:53, Geert Uytterhoeven wrote:
>>> On Tue, Jan 2, 2024 at 5:13 AM Qu Wenruo <wqu@...e.com> wrote:
>>>> The new tests cases for memparse_safe() include:
>>>>
>>>> - The existing test cases for kstrtoull()
>>>>     Including all the 3 bases (8, 10, 16), and all the ok and failure
>>>>     cases.
>>>>     Although there are something we need to verify specific for
>>>>     memparse_safe():
>>>>
>>>>     * @retptr and @value are not modified for failure cases
>>>>
>>>>     * return value are correct for failure cases
>>>>
>>>>     * @retptr is correct for the good cases
>>>>
>>>> - New test cases
>>>>     Not only testing the result value, but also the @retptr, including:
>>>>
>>>>     * good cases with extra tailing chars, but without valid prefix
>>>>       The @retptr should point to the first char after a valid string.
>>>>       3 cases for all the 3 bases.
>>>>
>>>>     * good cases with extra tailing chars, with valid prefix
>>>>       5 cases for all the suffixes.
>>>>
>>>>     * bad cases without any number but stray suffix
>>>>       Should be rejected with -EINVAL
>>>>
>>>> Signed-off-by: Qu Wenruo <wqu@...e.com>
>>>
>>> Thanks for your patch!
>>>
>>>> --- a/lib/test-kstrtox.c
>>>> +++ b/lib/test-kstrtox.c
>>>> @@ -268,6 +268,237 @@ static void __init test_kstrtoll_ok(void)
>>>>           TEST_OK(kstrtoll, long long, "%lld", test_ll_ok);
>>>>    }
>>>>
>>>> +/*
>>>> + * The special pattern to make sure the result is not modified for error cases.
>>>> + */
>>>> +#define ULL_PATTERN            (0xefefefef7a7a7a7aULL)
>>>> +#if BITS_PER_LONG == 32
>>>> +#define POINTER_PATTERN                (0xefef7a7a7aUL)
>>>
>>> This pattern needs 40 bits to fit, so it doesn't fit in a 32-bit
>>> unsigned long or pointer.  Probably you wanted to use 0xef7a7a7aUL
>>> instead?
>>
>> My bad, one extra byte...
> 
> So did that fix the sparse warning? ;-)

Intel guys have already masked this particular warning.

But your newer suggestion is much better.

> 
>>>> +#else
>>>> +#define POINTER_PATTERN                (ULL_PATTERN)
>>>> +#endif
>>>
>>> Shouldn't a simple cast to uintptr_t work fine for both 32-bit and
>>> 64-bit systems:
>>>
>>>       #define POINTER_PATTERN  ((uintptr_t)ULL_PATTERN)
>>>
>>> Or even better, incorporate the cast to a pointer:
>>>
>>>       #define POINTER_PATTERN  ((void *)(uintptr_t)ULL_PATTERN)
>>
>> The problem is reported by sparse, which warns about that ULL_PATTERN
>> converted to a pointer would lose its width:
>>
>> lib/test-kstrtox.c:339:40: sparse: sparse: cast truncates bits from
>> constant value (efefefef7a7a7a7a becomes 7a7a7a7a)
> 
> Ah yes, sparse can be annoying.
> I'm still looking for a clean and concise way to shut up [1].
> 
>> I'm not sure if using uiintptr_t would solve it, thus I go the macro to
>> switch the value to avoid the static checker's warning.
>>
>> I tried to check how other locations handles patterned pointer value,
>> like CONFIG_INIT_STACK_ALL_PATTERN, but they're either relying on the
>> compiler or just memset().
>>
>> Any better idea to solve the problem in a better way?
> 
> Masking off the extra bits, like lower_32_bits()[2] does?
> 
>      #define POINTER_PATTERN  ((void *)(uintptr_t)((ULL_PATTERN) & UINTPTR_MAX))

This sounds much better to me.

I would go this path instead, and finally no need to manually count how 
many bytes (which I already failed once).

Thanks,
Qu

> 
> [1] https://lore.kernel.org/oe-kbuild-all/202312181649.u6k6hLIm-lkp@intel.com/
> [2] https://elixir.bootlin.com/linux/latest/source/include/linux/kernel.h#L82
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 

Download attachment "OpenPGP_0xC23D91F3A125FEA8.asc" of type "application/pgp-keys" (6918 bytes)

Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (496 bytes)