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
| ||
|
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