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

Powered by blists - more mailing lists