Received-SPF: pass (google.com: domain of linux-kernel+bounces-91311-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 4 Mar 2024 13:32:20 -0800
From: Kees Cook <keescook@chromium.org>
To: Dave Chinner <david@fromorbit.com>
Cc: Vlastimil Babka <vbabka@suse.cz>,
	Christian Brauner <brauner@kernel.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>, Jan Kara <jack@suse.cz>,
	linux-fsdevel@vger.kernel.org,
	"GONG, Ruiqi" <gongruiqi@huaweicloud.com>,
	Xiu Jianfeng <xiujianfeng@huawei.com>,
	Suren Baghdasaryan <surenb@google.com>,
	Kent Overstreet <kent.overstreet@linux.dev>,
	Andrew Morton <akpm@linux-foundation.org>,
	Christoph Lameter <cl@linux.com>, Pekka Enberg <penberg@kernel.org>,
	David Rientjes <rientjes@google.com>,
	Joonsoo Kim <iamjoonsoo.kim@lge.com>,
	Roman Gushchin <roman.gushchin@linux.dev>,
	Hyeonggon Yoo <42.hyeyoo@gmail.com>, linux-kernel@vger.kernel.org,
	linux-mm@kvack.org, linux-hardening@vger.kernel.org
Subject: Re: [PATCH 3/4] xattr: Use dedicated slab buckets for setxattr()
Message-ID: <202403041330.06842D397@keescook>
References: <20240304184252.work.496-kees@kernel.org>
 <20240304184933.3672759-3-keescook@chromium.org>
 <ZeY6Lv4rfUyFHgOr@dread.disaster.area>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZeY6Lv4rfUyFHgOr@dread.disaster.area>

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

-- 
Kees Cook