On Fri, Oct 25, 2024 at 10:45 PM Jann Horn <jannh@xxxxxxxxxx> wrote: > On Mon, Oct 7, 2024 at 4:23 PM Christian Brauner <brauner@xxxxxxxxxx> wrote: > > Port files to rely on file_ref reference to improve scaling and gain > > overflow protection. > [...] > > diff --git a/fs/file_table.c b/fs/file_table.c > > index 4b23eb7b79dd9d4ec779f4c01ba2e902988895dc..3f5dc4176b21ff82cc9440ed92a0ad962fdb2046 100644 > > --- a/fs/file_table.c > > +++ b/fs/file_table.c > > @@ -178,7 +178,7 @@ static int init_file(struct file *f, int flags, const struct cred *cred) > > * fget-rcu pattern users need to be able to handle spurious > > * refcount bumps we should reinitialize the reused file first. > > */ > > - atomic_long_set(&f->f_count, 1); > > + file_ref_init(&f->f_ref, 1); > > It is good that you use file_ref_init() here to atomically initialize > the file_ref; however, I think it is problematic that before this, > alloc_empty_file() uses kmem_cache_zalloc(filp_cachep, GFP_KERNEL) to > allocate the file, because that sets __GFP_ZERO, which means that > slab_post_alloc_hook() will use memset() to zero the file object. That > causes trouble in two different ways: > > > 1. After the memset() has changed the file ref to zero, I think > file_ref_get() can return true? Which means __get_file_rcu() could > believe that it acquired a reference, and we could race like this: > > task A task B > __get_file_rcu() > rcu_dereference_raw() > close() > [frees file] > alloc_empty_file() > kmem_cache_zalloc() > [reallocates same file] > memset(..., 0, ...) > file_ref_get() > [increments 0->1, returns true] > init_file() > file_ref_init(..., 1) > [sets to 0] > rcu_dereference_raw() > fput() > file_ref_put() > [decrements 0->FILE_REF_NOREF, frees file] > [UAF] > > > 2. AFAIK the memset() is not guaranteed to atomically update an > "unsigned long", so you could see an entirely bogus torn counter > value. > > The only reason this worked in the old code is that the refcount value > stored in freed files is 0. > > So I think you need to stop using kmem_cache_zalloc() to allocate > files, and instead use a constructor function that zeroes the refcount > field, and manually memset() the rest of the "struct file" to 0 after > calling kmem_cache_alloc(). Actually, thinking about this again, I guess there's actually no reason why you'd need a constructor function; if you just avoid memset()ing the refcount field on allocation, that'd be good enough.
