On Fri, Sep 20, 2024 at 8:51 PM 'Chuck Lever III' via syzkaller-bugs <syzkaller-bugs@xxxxxxxxxxxxxxxx> wrote: > > > > > On Sep 19, 2024, at 10:57 PM, NeilBrown <neilb@xxxxxxx> wrote: > > > >> So we're tearing down the server and cleaning out the nfsd_file hash, > >> and we hit a UAF. That probably means that we freed a nfsd_file without > >> removing it from the hash? Maybe we should add a WARN_ON() in > >> nfsd_file_slab_free that checks whether the item is still hashed? > >> > >> It is strange though. struct nfsd_file is 112 bytes on my machine, but > >> the warning is about a 4k allocation. I guess that just means that the > >> page got recycled into a different slabcache. > > > > The code that is crashing hasn't come close to touching anything that is > > thought to be an nfsd_file. > > The error is detected in the list_add() in rhashtable_walk_enter() when > > the new on-stack iterator is being attached to the bucket_table that is being > > iterated. So that bucket_table must (now) be an invalid address. > > > > The handling of NFSD_FILE_CACHE_UP is strange. nfsd_file_cache_init() > > sets it, but doesn't clear it on failure. So if nfsd_file_cache_init() > > fails for some reason, nfsd_file_cache_shutdown() would still try to > > clean up if it was called. > > > > So suppose nfsd_startup_generic() is called. It increments nfsd_users > > from 0 so continues to nfsd_file_cache_init() which fails for some > > reason after initialising nfsd_file_rhltable and then destroying it. > > This will leave nfsd_file_rhltable.tbl as a pointer to a large > > allocation which has been freed. nfsd_startup_generic() will then > > decrement nfsd_users back to zero, but NFSD_FILE_CACHE_UP will still be > > set. > > > > When nfsd_startup_generic() is called again, nfsd_file_cache_init() will > > skip initialisation because NFSD_FILE_CACHE_UP is set. When > > nfsd_file_cache_shutdown() is then called it will clean up an rhltable > > that has already been destroyed. We get exactly the reported symptom. > > > > I *think* nfsd_file_cache_init() can only fail with -ENOMEM and I would > > expect to see a warning when that happened. In any case > > nfsd_file_cache_init() uses pr_err() for any failure except > > rhltable_init(), and that only fails if the params are inconsistent. > > > > So I think there are problems with NFSD_FILE_CACHE_UP settings and I > > think they could trigger this bug if a kmalloc failed, but I don't think > > that a kmalloc failed and I think there must be some other explanation > > here. > > Also, the FILE_CACHE_UP logic has been around for several releases. > Why is this UAF showing up only now? The most likely reason for that is that syzbot has only recently got some nfsd interface descriptions. It just didn't fuzz nfsd before. -- Aleksandr > The "unable to register" messages suggest a possible reason. > > -- > Chuck Lever > >
