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]

Message-ID: <20221209180934.cknkbvdr7gkphenf@lion.mk-sys.cz>
Date:   Fri, 9 Dec 2022 19:09:34 +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 Fri, Dec 09, 2022 at 09:42:59AM -0800, Jesse Brandeburg wrote:
> On 12/7/2022 10:34 PM, Michal Kubecek wrote:
> > On Wed, Dec 07, 2022 at 05:11:17PM -0800, Jesse Brandeburg wrote:
> 
> > > -	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?
> 
> I had done it this way to separate the "most minimal fix" from the
> "refactor", as I figure that is a clearer way to delineate changes. Also,
> this specifically addresses the issues found by the undefined behavior
> sanitizer.
> 
> I'll do it whichever way you like, but you're correct, later in this series
> I fix up all the BIT() usages. Maybe we can just leave this patch as is,
> knowing the full fix comes during the refactor in 10/13 ?

As we end up with BIT() everywhere anyway, I'm OK with either option,
leaving this patch as it is or dropping it. When I was writing that
comment, I had seen 09/13 (introduction of BIT()) but not 10/13
(refactoring everything to use it).

Michal

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