diff mbox series

[3/4] xattr: Use dedicated slab buckets for setxattr()

Message ID 20240304184933.3672759-3-keescook@chromium.org (mailing list archive)
State Superseded
Headers show
Series slab: Introduce dedicated bucket allocator | expand

Commit Message

Kees Cook March 4, 2024, 6:49 p.m. UTC
The setxattr() API can be used for exploiting[1][2][3] use-after-free
type confusion flaws in the kernel. Avoid having a user-controlled size
cache share the global kmalloc allocator by using a separate set of
kmalloc buckets.

Link: https://duasynt.com/blog/linux-kernel-heap-spray [1]
Link: https://etenal.me/archives/1336 [2]
Link: https://github.com/a13xp0p0v/kernel-hack-drill/blob/master/drill_exploit_uaf.c [3]
Signed-off-by: Kees Cook <keescook@chromium.org>
---
Cc: Christian Brauner <brauner@kernel.org>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Jan Kara <jack@suse.cz>
Cc: linux-fsdevel@vger.kernel.org
---
 fs/xattr.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Comments

Dave Chinner March 4, 2024, 9:16 p.m. UTC | #1
On Mon, Mar 04, 2024 at 10:49:31AM -0800, Kees Cook wrote:
> The setxattr() API can be used for exploiting[1][2][3] use-after-free
> type confusion flaws in the kernel. Avoid having a user-controlled size
> cache share the global kmalloc allocator by using a separate set of
> kmalloc buckets.
> 
> Link: https://duasynt.com/blog/linux-kernel-heap-spray [1]
> Link: https://etenal.me/archives/1336 [2]
> Link: https://github.com/a13xp0p0v/kernel-hack-drill/blob/master/drill_exploit_uaf.c [3]
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: Jan Kara <jack@suse.cz>
> Cc: linux-fsdevel@vger.kernel.org
> ---
>  fs/xattr.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xattr.c b/fs/xattr.c
> index 09d927603433..2b06316f1d1f 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -821,6 +821,16 @@ SYSCALL_DEFINE4(fgetxattr, int, fd, const char __user *, name,
>  	return error;
>  }
>  
> +static struct kmem_buckets *xattr_buckets;
> +static int __init init_xattr_buckets(void)
> +{
> +	xattr_buckets = kmem_buckets_create("xattr", 0, 0, 0,
> +					    XATTR_LIST_MAX, NULL);
> +
> +	return 0;
> +}
> +subsys_initcall(init_xattr_buckets);
> +
>  /*
>   * Extended attribute LIST operations
>   */
> @@ -833,7 +843,7 @@ listxattr(struct dentry *d, char __user *list, size_t size)
>  	if (size) {
>  		if (size > XATTR_LIST_MAX)
>  			size = XATTR_LIST_MAX;
> -		klist = kvmalloc(size, GFP_KERNEL);
> +		klist = kmem_buckets_alloc(xattr_buckets, size, GFP_KERNEL);

There's a reason this uses kvmalloc() - allocations can be up to
64kB in size and it's not uncommon for large slab allocation to
fail on long running machines. hence this needs to fall back to
vmalloc() to ensure that large xattrs can always be read.

Essentially, you're trading a heap spraying vector that almost
no-one will ever see for a far more frequent -ENOMEM denial of
service that will be seen on production systems where large xattrs
are used.

-Dave.
Kees Cook March 4, 2024, 9:32 p.m. UTC | #2
On Tue, Mar 05, 2024 at 08:16:30AM +1100, Dave Chinner wrote:
> On Mon, Mar 04, 2024 at 10:49:31AM -0800, Kees Cook wrote:
> > The setxattr() API can be used for exploiting[1][2][3] use-after-free
> > type confusion flaws in the kernel. Avoid having a user-controlled size
> > cache share the global kmalloc allocator by using a separate set of
> > kmalloc buckets.
> > 
> > Link: https://duasynt.com/blog/linux-kernel-heap-spray [1]
> > Link: https://etenal.me/archives/1336 [2]
> > Link: https://github.com/a13xp0p0v/kernel-hack-drill/blob/master/drill_exploit_uaf.c [3]
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> > Cc: Christian Brauner <brauner@kernel.org>
> > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > Cc: Jan Kara <jack@suse.cz>
> > Cc: linux-fsdevel@vger.kernel.org
> > ---
> >  fs/xattr.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xattr.c b/fs/xattr.c
> > index 09d927603433..2b06316f1d1f 100644
> > --- a/fs/xattr.c
> > +++ b/fs/xattr.c
> > @@ -821,6 +821,16 @@ SYSCALL_DEFINE4(fgetxattr, int, fd, const char __user *, name,
> >  	return error;
> >  }
> >  
> > +static struct kmem_buckets *xattr_buckets;
> > +static int __init init_xattr_buckets(void)
> > +{
> > +	xattr_buckets = kmem_buckets_create("xattr", 0, 0, 0,
> > +					    XATTR_LIST_MAX, NULL);
> > +
> > +	return 0;
> > +}
> > +subsys_initcall(init_xattr_buckets);
> > +
> >  /*
> >   * Extended attribute LIST operations
> >   */
> > @@ -833,7 +843,7 @@ listxattr(struct dentry *d, char __user *list, size_t size)
> >  	if (size) {
> >  		if (size > XATTR_LIST_MAX)
> >  			size = XATTR_LIST_MAX;
> > -		klist = kvmalloc(size, GFP_KERNEL);
> > +		klist = kmem_buckets_alloc(xattr_buckets, size, GFP_KERNEL);
> 
> There's a reason this uses kvmalloc() - allocations can be up to
> 64kB in size and it's not uncommon for large slab allocation to
> fail on long running machines. hence this needs to fall back to
> vmalloc() to ensure that large xattrs can always be read.

I can add a vmalloc fallback interface too. It looked like the larger
xattr usage (8k-64k) was less common, but yeah, let's not remove the
correct allocation fallback here. I'll fix this for v2.

Thanks!

-Kees
Eric Biggers March 4, 2024, 10:16 p.m. UTC | #3
On Mon, Mar 04, 2024 at 10:49:31AM -0800, Kees Cook wrote:
> xattr: Use dedicated slab buckets for setxattr()

This patch actually changes listxattr(), not setxattr().

getxattr(), setxattr(), and listxattr() all allocate a user controlled size.
Perhaps you meant to change all three?  What is special about listxattr() (or
setxattr() if you actually meant to change that one)?

- Eric
Kees Cook March 4, 2024, 11:03 p.m. UTC | #4
On Mon, Mar 04, 2024 at 02:16:48PM -0800, Eric Biggers wrote:
> On Mon, Mar 04, 2024 at 10:49:31AM -0800, Kees Cook wrote:
> > xattr: Use dedicated slab buckets for setxattr()
> 
> This patch actually changes listxattr(), not setxattr().
> 
> getxattr(), setxattr(), and listxattr() all allocate a user controlled size.
> Perhaps you meant to change all three?  What is special about listxattr() (or
> setxattr() if you actually meant to change that one)?

Whoops. Yes, I did one and stopped. :P I'll fix it up in v2.
diff mbox series

Patch

diff --git a/fs/xattr.c b/fs/xattr.c
index 09d927603433..2b06316f1d1f 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -821,6 +821,16 @@  SYSCALL_DEFINE4(fgetxattr, int, fd, const char __user *, name,
 	return error;
 }
 
+static struct kmem_buckets *xattr_buckets;
+static int __init init_xattr_buckets(void)
+{
+	xattr_buckets = kmem_buckets_create("xattr", 0, 0, 0,
+					    XATTR_LIST_MAX, NULL);
+
+	return 0;
+}
+subsys_initcall(init_xattr_buckets);
+
 /*
  * Extended attribute LIST operations
  */
@@ -833,7 +843,7 @@  listxattr(struct dentry *d, char __user *list, size_t size)
 	if (size) {
 		if (size > XATTR_LIST_MAX)
 			size = XATTR_LIST_MAX;
-		klist = kvmalloc(size, GFP_KERNEL);
+		klist = kmem_buckets_alloc(xattr_buckets, size, GFP_KERNEL);
 		if (!klist)
 			return -ENOMEM;
 	}