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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 17 May 2022 08:49:38 +0200 From: Rasmus Villemoes <linux@...musvillemoes.dk> To: Kees Cook <keescook@...omium.org>, Yury Norov <yury.norov@...il.com> Cc: Christophe de Dinechin <dinechin@...hat.com>, Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, Andrew Morton <akpm@...ux-foundation.org>, Zhen Lei <thunder.leizhen@...wei.com>, linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org Subject: Re: [PATCH] bitmap: Fix return values to be unsigned On 17/05/2022 05.54, Kees Cook wrote: > Both nodemask and bitmap routines had mixed return values that provided > potentially signed results that could never happen. This was leading to > the compiler getting confusing about the range of possible return values > (it was thinking things could be negative where they could not be). Fix > all the nodemask and bitmap routines that should be returning unsigned > (or bool) values. Silences GCC 12 warnings: So, for the bitmap functions themselves, makes sense, and then also for the nodemask functions which are merely wrappers around the bitmap functions (or wrappers around wrappers ...). But see below. > > #define first_node(src) __first_node(&(src)) > -static inline int __first_node(const nodemask_t *srcp) > +static inline unsigned int __first_node(const nodemask_t *srcp) > { > - return min_t(int, MAX_NUMNODES, find_first_bit(srcp->bits, MAX_NUMNODES)); > + return min_t(unsigned int, MAX_NUMNODES, find_first_bit(srcp->bits, MAX_NUMNODES)); > } Unrelated to the type change, but what's that min() doing there in the first place? Doesn't find_first_bit() already return the nbits argument if no "first bit" exists (i.e., the bitmap is empty)? > #define next_node(n, src) __next_node((n), &(src)) > -static inline int __next_node(int n, const nodemask_t *srcp) > +static inline unsigned int __next_node(int n, const nodemask_t *srcp) > { > - return min_t(int,MAX_NUMNODES,find_next_bit(srcp->bits, MAX_NUMNODES, n+1)); > + return min_t(unsigned int, MAX_NUMNODES, find_next_bit(srcp->bits, MAX_NUMNODES, n+1)); > } Same here and a few more places. It seems to go all the way back to pre-git. Hm. Could be cleaned up separately I guess. > > #if defined(CONFIG_NUMA) && (MAX_NUMNODES > 1) > -extern int node_random(const nodemask_t *maskp); > +extern unsigned int node_random(const nodemask_t *maskp); So this one I'm not convinced about. It has a documented return value of NUMA_NO_NODE aka -1 if the mask is empty. And since it's not a wrapper around a corresponding bitmap_random() (which would presumably, did it exist, use the "return nbits if empty" convention), there's no compelling reason to make its return type unsigned. > > @@ -18,9 +18,9 @@ EXPORT_SYMBOL(__next_node_in); > * Return the bit number of a random bit set in the nodemask. > * (returns NUMA_NO_NODE if nodemask is empty) > */ > -int node_random(const nodemask_t *maskp) > +unsigned int node_random(const nodemask_t *maskp) > { > - int w, bit = NUMA_NO_NODE; > + unsigned int w, bit = NUMA_NO_NODE; > > w = nodes_weight(*maskp); > if (w) Rasmus
Powered by blists - more mailing lists