linux-kernel - Re: [PATCH 1/2] lib: test_scanf: Fix incorrect use of type

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]

Date:   Thu, 27 May 2021 23:59:18 +0200
From:   Luc Van Oostenryck <luc.vanoostenryck@...il.com>
To:     Rasmus Villemoes <linux@...musvillemoes.dk>
Cc:     Richard Fitzgerald <rf@...nsource.cirrus.com>, pmladek@...e.com,
        rostedt@...dmis.org, sergey.senozhatsky@...il.com,
        andriy.shevchenko@...ux.intel.com, w@....eu, lkml@....org,
        davem@...emloft.net, kuba@...nel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, patches@...nsource.cirrus.com
Subject: Re: [PATCH 1/2] lib: test_scanf: Fix incorrect use of type_min()
 with unsigned types

On Tue, May 25, 2021 at 12:30:02PM +0200, Rasmus Villemoes wrote:
> On 25/05/2021 12.10, Richard Fitzgerald wrote:
> > On 25/05/2021 10:55, Rasmus Villemoes wrote:
> >> On 24/05/2021 17.59, Richard Fitzgerald wrote:
> >>> sparse was producing warnings of the form:
> >>>
> >>>   sparse: cast truncates bits from constant value (ffff0001 becomes 1)
> >>>
> >>> The problem was that value_representable_in_type() compared unsigned
> >>> types
> >>> against type_min(). But type_min() is only valid for signed types
> >>> because
> >>> it is calculating the value -type_max() - 1.
> > 
> > Ok, I see I was wrong about that. It does in fact work safely. Do you
> > want me to update the commit message to remove this?
> 
> Well, it was the "is only valid for signed types" I reacted to, so yes,
> please reword.
> 
> >> ... and casts that to (T), so it does produce 0 as it should. E.g. for
> >> T==unsigned char, we get
> >>
> >> #define type_min(T) ((T)((T)-type_max(T)-(T)1))
> >> (T)((T)-255 - (T)1)
> >> (T)(-256)
> >>
> > 
> > sparse warns about those truncating casts.
> 
> That's sad. As the comments and commit log indicate, I was very careful
> to avoid gcc complaining, even with various -Wfoo that are not normally
> enabled in a kernel build. I think sparse is wrong here. Cc += Luc.

Well, there is a cast and it effectively truncates the upper bits
of the constant, so sparse is kinda right but ... months ago I once
investigated these warnings and in all cases but one the use of the
cast was legit. Most of them was for:
1) a 32-bit constant that was (via some macro) split as two 16-bit
   constants which were then written to some 16-bit HW registers.
   The problem would not happen if the macro would use a AND mask
   instead of a cast but it seems that people tend to refer the cast,
   I think it's the wrong choice but eh.

2) some generic macro that do things like:
   #define macro(size, value) \
	switch (size) {
	case 1:
		... (u8) value;
	case 2:
		... (u16) value;
	...

    x = macro(sizeof(int), 0xffff0001);

   So, each time the macro is used for 32-bit, the code still contains
   a cast of the value to some smaller type, even if all uses are OK.
   The problem here is that these warnings are issued by sparse well
   before it can know that the code is dead and when it know it, these
   casts are already eliminated.

I'm sure this warning can sometimes catch a real problem but most of
the time it's not, just false warnings.

I think it would be best to disable this warning by default, but IIRC
this has already be discussed (years ago) and there was some opposition.
Maybe enabling it only at W=2 or something. I dunno.

-- Luc