netdev - Re: [PATCH ethtool v2 08/13] ethtool: fix runtime errors found by sanitizers

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]

Date:   Thu, 8 Dec 2022 07:34:32 +0100
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Jesse Brandeburg <jesse.brandeburg@...el.com>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH ethtool v2 08/13] ethtool: fix runtime errors found by
 sanitizers

On Wed, Dec 07, 2022 at 05:11:17PM -0800, Jesse Brandeburg wrote:
> The sanitizers[1] found a couple of things, but this change addresses
> some bit shifts that cannot be contained by the target type.
> 
> The mistake is that the code is using unsigned int a = (1 << N) all over
> the place, but the appropriate way to code this is unsigned int an
> assignment of (1UL << N) especially if N can ever be 31.
> 
> Fix the most egregious of these problems by changing "1" to "1UL", as
> per it would be if we had used the BIT() macro.
> 
> [1] make CFLAGS+='-fsanitize=address,undefined' \
>          LDFLAGS+='-lubsan -lasan'
> 
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@...el.com>
> ---
>  amd8111e.c         | 2 +-
>  internal.h         | 4 ++--
>  netlink/features.c | 4 ++--
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/amd8111e.c b/amd8111e.c
> index 175516bd2904..5a2fc2082e55 100644
> --- a/amd8111e.c
> +++ b/amd8111e.c
> @@ -75,7 +75,7 @@ typedef enum {
>  }CMD3_BITS;
>  typedef enum {
>  
> -	INTR			= (1 << 31),
> +	INTR			= (1UL << 31),
>  	PCSINT			= (1 << 28),
>  	LCINT			= (1 << 27),
>  	APINT5			= (1 << 26),

While the signedness issue only directly affects only INTR value,
I would rather prefer to keep the code consistent and fix the whole enum.
Also, as you intend to introduce the BIT() macro in the series anyway,
wouldn't it be cleaner to move this patch after the UAPI update and use
BIT() instead?

Michal

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