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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 24 Feb 2022 20:47:02 -0800 From: Kees Cook <keescook@...omium.org> To: David Laight <David.Laight@...lab.com> Cc: Matthew Wilcox <willy@...radead.org>, Josh Poimboeuf <jpoimboe@...hat.com>, Andrew Morton <akpm@...ux-foundation.org>, "linux-mm@...ck.org" <linux-mm@...ck.org>, Muhammad Usama Anjum <usama.anjum@...labora.com>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "x86@...nel.org" <x86@...nel.org>, "linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>, "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>, "linux-s390@...r.kernel.org" <linux-s390@...r.kernel.org>, "linux-sh@...r.kernel.org" <linux-sh@...r.kernel.org>, "linux-hardening@...r.kernel.org" <linux-hardening@...r.kernel.org> Subject: Re: [PATCH v2] usercopy: Check valid lifetime via stack depth On Thu, Feb 24, 2022 at 08:58:20AM +0000, David Laight wrote: > From: Kees Cook > > Sent: 24 February 2022 06:04 > > > > Under CONFIG_HARDENED_USERCOPY=y, when exact stack frame boundary checking > > is not available (i.e. everything except x86 with FRAME_POINTER), check > > a stack object as being at least "current depth valid", in the sense > > that any object within the stack region but not between start-of-stack > > and current_stack_pointer should be considered unavailable (i.e. its > > lifetime is from a call no longer present on the stack). > > > ... > > diff --git a/mm/usercopy.c b/mm/usercopy.c > > index d0d268135d96..5d28725af95f 100644 > > --- a/mm/usercopy.c > > +++ b/mm/usercopy.c > > @@ -22,6 +22,30 @@ > > #include <asm/sections.h> > > #include "slab.h" > > > > +/* > > + * Only called if obj is within stack/stackend bounds. Determine if within > > + * current stack depth. > > + */ > > +static inline int check_stack_object_depth(const void *obj, > > + unsigned long len) > > +{ > > +#ifdef CONFIG_ARCH_HAS_CURRENT_STACK_POINTER > > +#ifndef CONFIG_STACK_GROWSUP > > Pointless negation > > > + const void * const high = stackend; > > + const void * const low = (void *)current_stack_pointer; > > +#else > > + const void * const high = (void *)current_stack_pointer; > > + const void * const low = stack; > > +#endif > > + > > + /* Reject: object not within current stack depth. */ > > + if (obj < low || high < obj + len) > > + return BAD_STACK; > > + > > +#endif > > + return GOOD_STACK; > > +} > > If the comment at the top of the function is correct then > only a single test for the correct end of the buffer against > the current stack pointer is needed. > Something like: > #ifdef CONFIG_STACK_GROWSUP > if ((void *)current_stack_pointer < obj + len) > return BAD_STACK; > #else > if (obj < (void *)current_stack_pointer) > return BAD_STACK; > #endif > return GOOD_STACK; Oh, yeah, excellent point. I suspect the compiler would probably optimize it all away, but yes, this is, in fact, easier to read, and short enough I should probably just not bother with a separate function. Thanks! -Kees > > Although it may depend on exactly where the stack pointer > points to - especially for GROWSUP. > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) > -- Kees Cook
Powered by blists - more mailing lists