| Message ID | 20191010103151.7708-1-mayhs11saini@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | slab: Redefine ZERO_SIZE_PTR to include ERR_PTR range | expand |
On Thu, 10 Oct 2019, Shyam Saini wrote: > This will help error related to ERR_PTR stand out better. Maybe make ZERO_SIZE_PTR an ERRNO value instead? Then allow ERR_PTRs to be used instead of ZERO_SIZE_PTRs ERRNO_ZERO_OBJECT or something like that? > */ > -#define ZERO_SIZE_PTR ((void *)16) #define ZERO_SIZE_PTR ((void *)-ERRNO_ZERO_OBJECT) > + > +#define ZERO_OR_NULL_PTR(x) ((unsigned long)(x) - 1 >= \ > + (unsigned long)ZERO_SIZE_PTR - 1) And call this something different?
On Thu, Oct 10, 2019 at 02:22:40PM +0000, Christopher Lameter wrote: > On Thu, 10 Oct 2019, Shyam Saini wrote: > > > This will help error related to ERR_PTR stand out better. > > Maybe make ZERO_SIZE_PTR an ERRNO value instead? Then allow ERR_PTRs to be > used instead of ZERO_SIZE_PTRs > > ERRNO_ZERO_OBJECT > > or something like that? I was wondering about something like that too, but allocating zero bytes isn't actually an error, and if we have code that does something like: void *p = my_funky_alloc(size, ...); if (IS_ERR(p)) return PTR_ERR(p); then we might get this errno returned to userspace. The change is definitely worth thinking about.
On Thu, 10 Oct 2019, Matthew Wilcox wrote: > I was wondering about something like that too, but allocating zero bytes > isn't actually an error, and if we have code that does something like: True. But it is in a greyzone. You cannot store anything in zero bytes after all.
Hi Matthew, Christopher, > > > This will help error related to ERR_PTR stand out better. > > > > Maybe make ZERO_SIZE_PTR an ERRNO value instead? Then allow ERR_PTRs to be > > used instead of ZERO_SIZE_PTRs > > > > ERRNO_ZERO_OBJECT > > > > or something like that? > > I was wondering about something like that too, but allocating zero bytes > isn't actually an error, and if we have code that does something like: > > void *p = my_funky_alloc(size, ...); > > if (IS_ERR(p)) > return PTR_ERR(p); > > then we might get this errno returned to userspace. > > The change is definitely worth thinking about. Any further comments on this ? Please let me know. Thanks!!
On Thu, Oct 10, 2019 at 12:32 PM Shyam Saini <mayhs11saini@gmail.com> wrote: > Currently kfree does not accept ERR_PTR range so redefine ZERO_SIZE_PTR > to include this and also change ZERO_OR_NULL_PTR macro to check this new > range. With this change kfree will skip and behave as no-ops when ERR_PTR > is passed. > > This will help error related to ERR_PTR stand out better. What do you mean by "stand out better"? To me it sounds like before, the kernel would probably blow up in some way if you passed an error pointer into kfree(), and with this change, it will silently ignore it instead, right? If you actually wanted this kind of error to stand out, wouldn't it make more sense to add something like "if (WARN_ON(IS_ERR(x))) return;" to the implementations of kfree()? I would prefer that, since "kfree(<error pointer>)" probably indicates that someone messed up their error handling jumps. > After this, we don't need to reset any ERR_PTR variable to NULL before > being passed to any kfree or related wrappers calls, as everything would > be handled by ZERO_SIZE_PTR itself. With the caveat that you still can't do it in code that might be stable-backported, otherwise it will blow up occasionally in the rare case where the error path is hit? [...] > +#define ZERO_SIZE_PTR \ > +({ \ > + BUILD_BUG_ON(!(MAX_ERRNO & ~PAGE_MASK));\ > + (void *)(-MAX_ERRNO-1L); \ > +}) > + > +#define ZERO_OR_NULL_PTR(x) ((unsigned long)(x) - 1 >= \ > + (unsigned long)ZERO_SIZE_PTR - 1) If you do go through with this change, you'll probably want to adjust the message in check_bogus_address() - "null address" really isn't an appropriate error message for an address near the end of the address space.
diff --git a/include/linux/slab.h b/include/linux/slab.h index 877a95c6a2d2..8ffdabd218f8 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -127,11 +127,16 @@ * * ZERO_SIZE_PTR can be passed to kfree though in the same way that NULL can. * Both make kfree a no-op. + * Note: ZERO_SIZE_PTR also cover ERR_PTR Range. */ -#define ZERO_SIZE_PTR ((void *)16) - -#define ZERO_OR_NULL_PTR(x) ((unsigned long)(x) <= \ - (unsigned long)ZERO_SIZE_PTR) +#define ZERO_SIZE_PTR \ +({ \ + BUILD_BUG_ON(!(MAX_ERRNO & ~PAGE_MASK));\ + (void *)(-MAX_ERRNO-1L); \ +}) + +#define ZERO_OR_NULL_PTR(x) ((unsigned long)(x) - 1 >= \ + (unsigned long)ZERO_SIZE_PTR - 1) #include <linux/kasan.h>
Currently kfree does not accept ERR_PTR range so redefine ZERO_SIZE_PTR to include this and also change ZERO_OR_NULL_PTR macro to check this new range. With this change kfree will skip and behave as no-ops when ERR_PTR is passed. This will help error related to ERR_PTR stand out better. After this, we don't need to reset any ERR_PTR variable to NULL before being passed to any kfree or related wrappers calls, as everything would be handled by ZERO_SIZE_PTR itself. This patch is verbatim from Brad Spengler/PaX Team's code in the last public patch of grsecurity/PaX based on my understanding of the code. Changes or omissions from the original code are mine and don't reflect the original grsecurity/PaX code. Cc: Matthew Wilcox <willy@infradead.org> Cc: Christopher Lameter <cl@linux.com> Cc: Kees Cook <keescook@chromium.org> Signed-off-by: Shyam Saini <mayhs11saini@gmail.com> --- include/linux/slab.h | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)