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]

Message-ID: <CAHk-=whmx4Pqw70NDmujGMQY1BrVi54pLnB5b7qpPThjiHatOw@mail.gmail.com>
Date:   Tue, 23 Aug 2022 22:56:21 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Al Viro <viro@...iv.linux.org.uk>
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 Tue, Aug 23, 2022 at 8:10 PM Al Viro <viro@...iv.linux.org.uk> wrote:
>
> 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().

No. Look. Try this modification to that test, and use
'./test-linearize' to see what sparse turns it into without my patch
to keep the signedness.

    static long test(void)
    {
        return (le32) 0xffffffff;
    }

yes, yes, it warns (twice, actually), but it also then generates

        ret.64      $-1

for that return.

Why? Because it thinks that 'le32' is a signed 32-bit thing due to the
clearing of the MOD_UNSIGNED bit, so when it casts it to 'long' it
will sign-extend it.

So the sign confusion exists and is visible regardless of the added
ordered comparison.

Now, we normally don't *notice* any of this, because we obviously
don't rely on sparse generating any code. And we _do_ cast those
bitwise things in many places, although we use "__force" to show that
it's intentional. Including, very much, those kinds of widening casts
where the signedness matters.

See for example very much the csum code:

    __wsum csum_partial(const void *buff, int len, __wsum wsum)
    {
        unsigned int sum = (__force unsigned int)wsum;

which is *exactly* that kind of code where it's fundamentally
important that 'wsum' is an unsigned type, and casting it to 'unsigned
int' does not sign-extend it.

So no. This has absolutely nothing to do with the new ordered comparisons.

Those bitwise types have always been integers, just with special rules
for warning about mis-using them.

And the sign handling has always been wrong.

It just so happens that me using 'test-linearize' to double-check what
sparse does for that signedness check *uncovered* that pre-existing
bug.

It was not introduced by the new code, and the ordered comparisons are
not AT ALL different from the equality comparisons, except for the
fact that they actually care about the signedness being right.

           Linus