linux-kernel - Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings triggered by is_signed

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:   Wed, 24 Aug 2022 04:10:25 +0100
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Bart Van Assche <bvanassche@....org>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Steven Rostedt <rostedt@...dmis.org>,
        linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Christoph Hellwig <hch@....de>,
        Luc Van Oostenryck <luc.vanoostenryck@...il.com>,
        Jens Axboe <axboe@...nel.dk>
Subject: Re: [for-linus][PATCH 01/10] tracing: Suppress sparse warnings
 triggered by is_signed_type()

On Wed, Aug 24, 2022 at 03:09:07AM +0100, Al Viro wrote:
> On Tue, Aug 23, 2022 at 04:57:00PM -0700, Linus Torvalds wrote:
> > On Tue, Aug 23, 2022 at 4:18 PM Linus Torvalds
> > <torvalds@...ux-foundation.org> wrote:
> > >
> > > Can you try the sparse version at
> > >
> > >    git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/sparse.git
> > >
> > > which I just set up temporarily with some patches of mine.
> > 
> > Ugh, and while testing this with sparse, I noticed that sparse itself
> > got that whole 'is_signed_type()' check wrong.
> > 
> > The sparse fix was to remove one line of code, but that one worries
> > me, because that one line was clearly very intentional:
> > 
> >    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/sparse.git/commit/?id=7e5f1c2eba1426e414698071dd0de7d039eb385d
> > 
> > Adding Al, since he's actually the original source of that bitwise
> > code (and did a lot of other sparse code on the type handling and
> > preprocessor side in particular).
> 
> Ouch...  That'll take quite a bit of swap-in (and digging through the
> old notes).

The basic approach was to have them *NOT* treated as integer types;
it's SYM_RESTRICT node refering to the node for underlying type.
MOD_CHAR/MOD_LONG/MOD_UNSIGNED/etc. make no more sense for that than
they do for e.g. pointers.

Any operations like ordered comparisons would trigger unrestrict() on
these suckers, which would warn and convert to underlying type.

Your addition of "ordered comparison with 0 or -1" evades unrestrict().
Which shuts the warning up, but that leaves evaluate_compare() with
something unexpected -
		if (ctype->ctype.modifiers & MOD_UNSIGNED)
			expr->op = modify_for_unsigned(expr->op);
running into SYM_RESTRICT node.

*IF* you want to go that way, I would suggest a new return value for
restricted_binop() ("comparison with magical value"), with
something like
		switch (restricted_binop(op, ctype)) {
		case 1:
		....

		default:
			break;
		case 4:
			// comparison with magic value:
			// quietly go for underlying type, if not fouled
			// if fouled, just return NULL and let the caller
			// deal with that - loudly.
			if (!(lclass & rclass & TYPE_FOULED))
				ctype = ctype->ctype.base_type;
			else
				ctype = NULL;
		}
in restricted_binop_type().