Linux-mm Archive on lore.kernel.org
 help / color / Atom feed
* Re: [PATCH] slab: Redefine ZERO_SIZE_PTR to include ERR_PTR range
       [not found] <20191010103151.7708-1-mayhs11saini@gmail.com>
@ 2019-10-10 14:22 ` Christopher Lameter
  2019-10-10 17:44   ` Matthew Wilcox
  2019-10-20 15:38 ` Jann Horn
  1 sibling, 1 reply; 5+ messages in thread
From: Christopher Lameter @ 2019-10-10 14:22 UTC (permalink / raw)
  To: Shyam Saini; +Cc: linux-mm, kernel-hardening, Matthew Wilcox, Kees Cook

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?



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] slab: Redefine ZERO_SIZE_PTR to include ERR_PTR range
  2019-10-10 14:22 ` [PATCH] slab: Redefine ZERO_SIZE_PTR to include ERR_PTR range Christopher Lameter
@ 2019-10-10 17:44   ` Matthew Wilcox
  2019-10-10 18:35     ` Christopher Lameter
  2019-10-20  6:06     ` Shyam Saini
  0 siblings, 2 replies; 5+ messages in thread
From: Matthew Wilcox @ 2019-10-10 17:44 UTC (permalink / raw)
  To: Christopher Lameter; +Cc: Shyam Saini, linux-mm, kernel-hardening, Kees Cook

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.


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] slab: Redefine ZERO_SIZE_PTR to include ERR_PTR range
  2019-10-10 17:44   ` Matthew Wilcox
@ 2019-10-10 18:35     ` Christopher Lameter
  2019-10-20  6:06     ` Shyam Saini
  1 sibling, 0 replies; 5+ messages in thread
From: Christopher Lameter @ 2019-10-10 18:35 UTC (permalink / raw)
  To: Matthew Wilcox; +Cc: Shyam Saini, linux-mm, kernel-hardening, Kees Cook

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.


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] slab: Redefine ZERO_SIZE_PTR to include ERR_PTR range
  2019-10-10 17:44   ` Matthew Wilcox
  2019-10-10 18:35     ` Christopher Lameter
@ 2019-10-20  6:06     ` Shyam Saini
  1 sibling, 0 replies; 5+ messages in thread
From: Shyam Saini @ 2019-10-20  6:06 UTC (permalink / raw)
  To: Matthew Wilcox, Christopher Lameter; +Cc: linux-mm, Kernel Hardening, Kees Cook

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] slab: Redefine ZERO_SIZE_PTR to include ERR_PTR range
       [not found] <20191010103151.7708-1-mayhs11saini@gmail.com>
  2019-10-10 14:22 ` [PATCH] slab: Redefine ZERO_SIZE_PTR to include ERR_PTR range Christopher Lameter
@ 2019-10-20 15:38 ` Jann Horn
  1 sibling, 0 replies; 5+ messages in thread
From: Jann Horn @ 2019-10-20 15:38 UTC (permalink / raw)
  To: Shyam Saini
  Cc: Linux-MM, Kernel Hardening, Matthew Wilcox, Christopher Lameter,
	Kees Cook

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.


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20191010103151.7708-1-mayhs11saini@gmail.com>
2019-10-10 14:22 ` [PATCH] slab: Redefine ZERO_SIZE_PTR to include ERR_PTR range Christopher Lameter
2019-10-10 17:44   ` Matthew Wilcox
2019-10-10 18:35     ` Christopher Lameter
2019-10-20  6:06     ` Shyam Saini
2019-10-20 15:38 ` Jann Horn

Linux-mm Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mm/0 linux-mm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mm linux-mm/ https://lore.kernel.org/linux-mm \
		linux-mm@kvack.org
	public-inbox-index linux-mm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kvack.linux-mm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git