linux-kernel - Re: [PATCH] core: Catch overflows in do

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] [day] [month] [year] [list]
Date:	Tue, 8 Oct 2013 10:40:30 -0700
From:	Anatol Pomozov <anatol.pomozov@...il.com>
To:	Richard Weinberger <richard.weinberger@...il.com>
Cc:	Joe Perches <joe@...ches.com>, LKML <linux-kernel@...r.kernel.org>,
	Randy Dunlap <rdunlap@...radead.org>
Subject: Re: [PATCH] core: Catch overflows in do_div() function

Hi

On Tue, Oct 8, 2013 at 10:28 AM, Anatol Pomozov
<anatol.pomozov@...il.com> wrote:
> Hi
>
> On Tue, Oct 8, 2013 at 9:45 AM, Richard Weinberger
> <richard.weinberger@...il.com> wrote:
>> On Tue, Oct 8, 2013 at 6:18 PM, Joe Perches <joe@...ches.com> wrote:
>>> On Tue, 2013-10-08 at 09:10 -0700, Anatol Pomozov wrote:
>>>> If second parameter passed to this function was 64 then it silently
>>>> truncates to 32 bits. Catch such situation.
>>> []
>>>> diff --git a/include/asm-generic/div64.h b/include/asm-generic/div64.h
>>> []
>>>> @@ -25,6 +26,7 @@
>>>>  # define do_div(n,base) ({                                   \
>>>>       uint32_t __base = (base);                               \
>>>>       uint32_t __rem;                                         \
>>>> +     BUG_ON(sizeof(base) > 4 && base >= (1UL<<32));          \
>>>
>>> I think this would be better as a BUILD_BUG_ON
>>
>> No. BUILD_BUG_ON works only for constants.
>
> BUILD_BUG_ON might actually work. In case if 'base' is const it will
> check if it fits 32 bits. As far as I see all such usages (when 'base'
> is const) are fine. In case if 'base' is 64 bit variable the
> compilation fails.
>
> Comparing with previous patch (without "&& base >= (1UL<<32)") it
> eliminates warnings in situations when we pass small constants as long
> (dozens of such places in HEAD).
>
> Looking at the cases when we use do_div() I see that in many cases we
> pass "long" as a second parameter (see __setup_per_zone_wmarks). If we
> replace it with div64_s64() we force to use 64 bit arithmetic. But on
> 32bit platform "long" is 32bit and using div64_s64() here is
> redundant. Wouldn't it be better if do_div() would handle this
> situation and called required functions based on a) current
> architecture b) size of base/n parameters. Something like this
> (completely untested and we need __div64_64 on 32 bit platform):
>
> --- a/include/asm-generic/div64.h
> +++ b/include/asm-generic/div64.h
> @@ -22,12 +22,12 @@
>
>  #if BITS_PER_LONG == 64
>
> -# define do_div(n,base) ({                                     \
> -       uint32_t __base = (base);                               \
> -       uint32_t __rem;                                         \
> -       __rem = ((uint64_t)(n)) % __base;                       \
> -       (n) = ((uint64_t)(n)) / __base;                         \
> -       __rem;                                                  \
> +# define do_div(n,base) ({             \
> +       typeof(base) __base = (base);   \

Documentation says typeof() has side-effects and can be used on
arithmetic types only. :(

> +       typeof(base) __rem;             \
> +       __rem = (n) % __base;           \
> +       (n) = (n) / __base;             \
> +       __rem;                          \
>   })
>
>  #elif BITS_PER_LONG == 32
> @@ -37,16 +37,20 @@ extern uint32_t __div64_32(uint64_t *dividend,
> uint32_t divisor);
>  /* The unnecessary pointer compare is there
>   * to check for type safety (n must be 64bit)
>   */
> -# define do_div(n,base) ({                             \
> -       uint32_t __base = (base);                       \
> -       uint32_t __rem;                                 \
> -       (void)(((typeof((n)) *)0) == ((uint64_t *)0));  \
> -       if (likely(((n) >> 32) == 0)) {                 \
> -               __rem = (uint32_t)(n) % __base;         \
> -               (n) = (uint32_t)(n) / __base;           \
> -       } else                                          \
> -               __rem = __div64_32(&(n), __base);       \
> -       __rem;                                          \
> +# define do_div(n,base) ({                                     \
> +       typeof(base) __base = (base);                           \
> +       typeof(base) __rem;                                     \
> +       (void)(((typeof((n)) *)0) == ((uint64_t *)0));          \
> +       if (sizeof(__base) <= 4 || (__builtin_constant_p(__base) &&
> __base < (1ULL<<32)) ) { \
> +               if (likely(((n) >> 32) == 0)) {                 \
> +                       __rem = (uint32_t)(n) % __base;         \
> +                       (n) = (uint32_t)(n) / __base;           \
> +               } if (sizeof(base) <= 4)                        \
> +                       __rem = __div64_32(&(n), __base);       \
> +       } else {                                                \
> +               __rem = __div64_64(&(n), __base);               \
> +       }                                                       \
> +       __rem;                                                  \
>   })
>
>  #else /* BITS_PER_LONG == ?? */
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/