linux-hardening - Re: [PATCH v13 5/9] drm/i915: Check for integer truncation on scatterlist creation

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-=wivJwvVbMUKma8600F6qaVLZHT=BY90SEnjiHWw2ZUVEg@mail.gmail.com>
Date:   Wed, 28 Sep 2022 10:09:04 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Gwan-gyeong Mun <gwan-gyeong.mun@...el.com>
Cc:     intel-gfx@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, mchehab@...nel.org,
        chris@...is-wilson.co.uk, matthew.auld@...el.com,
        thomas.hellstrom@...ux.intel.com, jani.nikula@...el.com,
        nirmoy.das@...el.com, airlied@...hat.com, daniel@...ll.ch,
        andi.shyti@...ux.intel.com, andrzej.hajda@...el.com,
        keescook@...omium.org, mauro.chehab@...ux.intel.com,
        linux@...musvillemoes.dk, vitor@...saru.org, dlatypov@...gle.com,
        ndesaulniers@...gle.com, trix@...hat.com, llvm@...ts.linux.dev,
        linux-hardening@...r.kernel.org, linux-sparse@...r.kernel.org,
        nathan@...nel.org, gustavoars@...nel.org,
        luc.vanoostenryck@...il.com
Subject: Re: [PATCH v13 5/9] drm/i915: Check for integer truncation on
 scatterlist creation

On Wed, Sep 28, 2022 at 1:15 AM Gwan-gyeong Mun
<gwan-gyeong.mun@...el.com> wrote:
>
> +       if (check_assign(obj->base.size >> PAGE_SHIFT, &npages))
> +               return -E2BIG;

I have to say, I find that new "check_assign()" macro use to be disgusting.

It's one thing to check for overflows.

It's another thing entirely to just assign something to a local variable.

This disgusting "let's check and assign" needs to die. It makes the
code a completely unreadable mess. The "user" wersion is even worse.

If you worry about overflow, then use a mix of

 (a) use a sufficiently large type to begin with

 (b) check for value range separately

and in this particular case, I also suspect that the whole range check
should have been somewhere else entirely - at the original creation of
that "obj" structure, not at one random end-point where it is used.

In other words, THIS WHOLE PATCH is just end-points checking the size
requirements of that "base.size" thing much too late, when it should
have been checked originally for some "maximum acceptable base size"
instead.

And that "maximum acceptable base size" should *not* be about "this is
the size of the variables we use". It should be a sanity check of
"this value is sane and fits in sane use cases".

Because "let's plug security checks" is most definitely not about
picking random assignments and saying "let's check this one". It's
about trying to catch things earlier than that.

Kees, you need to reign in the craziness in overflow.h.

                 Linus