linux-kernel - Re: [PATCH] resource: Elide a conversion warning on 32-bit

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]

Message-ID: <77953469-413b-13e4-72fb-c92b7b1c6dd1@linux.intel.com>
Date: Mon, 29 Jul 2024 18:38:52 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Palmer Dabbelt <palmer@...osinc.com>
cc: bhelgaas@...gle.com, Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, 
    Mika Westerberg <mika.westerberg@...ux.intel.com>, 
    Greg KH <gregkh@...uxfoundation.org>, bhe@...hat.com, 
    alison.schofield@...el.com, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] resource: Elide a conversion warning on 32-bit

On Mon, 29 Jul 2024, Palmer Dabbelt wrote:

> From: Palmer Dabbelt <palmer@...osinc.com>
> 
> I recently ended up with a warning on some compilers along the lines of
> 
>       CC      kernel/resource.o
>     In file included from include/linux/ioport.h:16,
>                      from kernel/resource.c:15:
>     kernel/resource.c: In function 'gfr_start':
>     include/linux/minmax.h:49:37: error: conversion from 'long long unsigned int' to 'resource_size_t' {aka 'unsigned int'} changes value from '17179869183' to '4294967295' [-Werror=overflow]
>        49 |         ({ type ux = (x); type uy = (y); __cmp(op, ux, uy); })
>           |                                     ^
>     include/linux/minmax.h:52:9: note: in expansion of macro '__cmp_once_unique'
>        52 |         __cmp_once_unique(op, type, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
>           |         ^~~~~~~~~~~~~~~~~
>     include/linux/minmax.h:161:27: note: in expansion of macro '__cmp_once'
>       161 | #define min_t(type, x, y) __cmp_once(min, type, x, y)
>           |                           ^~~~~~~~~~
>     kernel/resource.c:1829:23: note: in expansion of macro 'min_t'
>      1829 |                 end = min_t(resource_size_t, base->end,
>           |                       ^~~~~
>     kernel/resource.c: In function 'gfr_continue':
>     include/linux/minmax.h:49:37: error: conversion from 'long long unsigned int' to 'resource_size_t' {aka 'unsigned int'} changes value from '17179869183' to '4294967295' [-Werror=overflow]
>        49 |         ({ type ux = (x); type uy = (y); __cmp(op, ux, uy); })
>           |                                     ^
>     include/linux/minmax.h:52:9: note: in expansion of macro '__cmp_once_unique'
>        52 |         __cmp_once_unique(op, type, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
>           |         ^~~~~~~~~~~~~~~~~
>     include/linux/minmax.h:161:27: note: in expansion of macro '__cmp_once'
>       161 | #define min_t(type, x, y) __cmp_once(min, type, x, y)
>           |                           ^~~~~~~~~~
>     kernel/resource.c:1847:24: note: in expansion of macro 'min_t'
>      1847 |                addr <= min_t(resource_size_t, base->end,
>           |                        ^~~~~
>     cc1: all warnings being treated as errors
> 
> which this elides via an extra cast.

I think you should describe the configuration which triggers this a bit 
more here because it's not just about 32-bit build but also requires 
MAX_PHYSMEM_BITS to be larger than 32.

> Signed-off-by: Palmer Dabbelt <palmer@...osinc.com>
> ---
> Not sure if there's a better way to do this, but I didn't see any
> reports yet and my tester is failing so I figured it'd be best to get
> something on the lists.
> ---
>  kernel/resource.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 14777afb0a99..6459561b746e 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -1845,7 +1845,7 @@ static bool gfr_continue(struct resource *base, resource_size_t addr,
>  	 */
>  	return addr > addr - size &&
>  	       addr <= min_t(resource_size_t, base->end,
> -			     (1ULL << MAX_PHYSMEM_BITS) - 1);
> +			     (resource_size_t)((1ULL << MAX_PHYSMEM_BITS) - 1));

So this effectively casts away 2 bits hiding the warning? It would be 
more proper to limit it with RESOURCE_SIZE_MAX instead of casting the 
bits away if that's the intention of this code (of which I'm not sure).

And why are you fixing just one line when there is another similar
expression?

-- 
 i.