linux-kernel - Re: [GIT PULL] configfs fix for Linux 5.14

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-=wjuDBQdUvaO=XaptgmvE_qeg_EuZjsUZf2vVoXPUMgAvg@mail.gmail.com>
Date:   Wed, 14 Jul 2021 13:05:08 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Christoph Hellwig <hch@...radead.org>,
        Bart Van Assche <bvanassche@....org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>
Subject: Re: [GIT PULL] configfs fix for Linux 5.14

On Wed, Jul 14, 2021 at 9:33 AM Christoph Hellwig <hch@...radead.org> wrote:
>
> configfs fix for Linux 5.14
>
>  - fix the read and write iterators (Bart Van Assche)

I've pulled this, but I'm somewhat disgusted by it.

The overflow "protection" is just wrong:

+       to_copy = SIMPLE_ATTR_SIZE - 1 - pos;
+       if (to_copy <= 0)
+               return 0;

because if users control "pos", then that "to_copy" could be a huge
positive value even after overflow protection.

I hope/think that we always end up checking 'pos' in the VFS layer so
that this isn't a bug in practice, but people - the above is just
fundamentally bad code.

It's simply not the correct way to check limits. It does it badly, and
it's hard to read (*).

If you want to check limits, then do it (a) the obvious way and (b) right.

Something like

        if (pos < 0 || pos >= SIMPLE_ATTR_SIZE - 1)
                return 0;
        to_copy = SIMPLE_ATTR_SIZE - 1 - pos;

would have been a hell of a lot more obvious, would have been CORRECT,
and a compiler would likely be able to equally good code for it.

Doing a "x <0 || x > C" test is actually nice and cheap, and compilers
should all be smart enough to turn it into a single (unsigned)
comparison.

Possibly it even generates better code, since "to_copy" could then -
and should - no longer be a 64-bit loff_t, since it's pointless. We've
just checked the range of the values, so it can be the natural size
for the machine.

Although from a small test, gcc does seem to be too simple to take
advantage of that, and on 32-bit x86 it does the range check using
64-bit arithmetic even when unnecessary (it should just check "are the
upper 32 bits zero" rather than play around with doing a 64-bit
sub/sbb - I'm surprised, because I thought gcc already knew about
this, but maybe compiler people are starting to forget about 32-bit
stuff too).

But even if the compiler doesn't figure it out, the simple "just check
the limits" is a lot more readable for humans, and avoids the whole
overflow issue. And maybe some compilers will do better at it.

            Linus

(*) Ok, it's easy to read if you ignore the overflow possibility. IOW,
it's easy to read WRONG.