| 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: <CAOMFOmVcANaPBau-gVkhna33WdOsajD9vHjivuyqFES0csu=-Q@mail.gmail.com>
Date: Tue, 8 Oct 2013 10:28:17 -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 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); \
+ 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/
Powered by blists - more mailing lists