nfsd_file is RCU-freed, so it's possible that one could be found that's
in the process of being freed and the memory recycled. Ensure we hold
the rcu_read_lock while attempting to get a reference on the object.
Cc: NeilBrown <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfsd/filecache.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index d5c57360b418..f4f75ae2e4ea 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -1077,10 +1077,12 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
retry:
/* Avoid allocation if the item is already in cache */
+ rcu_read_lock();
nf = rhashtable_lookup_fast(&nfsd_file_rhash_tbl, &key,
nfsd_file_rhash_params);
if (nf)
nf = nfsd_file_get(nf);
+ rcu_read_unlock();
if (nf)
goto wait_for_construction;
@@ -1090,21 +1092,21 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
goto out_status;
}
+ rcu_read_lock();
nf = rhashtable_lookup_get_insert_key(&nfsd_file_rhash_tbl,
&key, &new->nf_rhash,
nfsd_file_rhash_params);
+ if (!IS_ERR_OR_NULL(nf)) {
+ nf = nfsd_file_get(nf);
+ nfsd_file_slab_free(&new->nf_rcu);
+ }
+ rcu_read_unlock();
if (!nf) {
nf = new;
goto open_file;
}
if (IS_ERR(nf))
goto insert_err;
- nf = nfsd_file_get(nf);
- if (nf == NULL) {
- nf = new;
- goto open_file;
- }
- nfsd_file_slab_free(&new->nf_rcu);
wait_for_construction:
wait_on_bit(&nf->nf_flags, NFSD_FILE_PENDING, TASK_UNINTERRUPTIBLE);
--
2.37.3
Hi Jeff-
> On Oct 1, 2022, at 5:59 AM, Jeff Layton <[email protected]> wrote:
>
> nfsd_file is RCU-freed, so it's possible that one could be found that's
> in the process of being freed and the memory recycled. Ensure we hold
> the rcu_read_lock while attempting to get a reference on the object.
I'm OK with entertaining clean-up patches in this code, but I
am skeptical that this patch addresses the concern enumerated
in bug #394. As you've pointed out to me before, the "UAF on
DELEGRETURN crashes" appeared well before v5.19, which is the
kernel release where this bit of code was introduced.
> Cc: NeilBrown <[email protected]>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> fs/nfsd/filecache.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index d5c57360b418..f4f75ae2e4ea 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -1077,10 +1077,12 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
>
> retry:
> /* Avoid allocation if the item is already in cache */
> + rcu_read_lock();
> nf = rhashtable_lookup_fast(&nfsd_file_rhash_tbl, &key,
> nfsd_file_rhash_params);
> if (nf)
> nf = nfsd_file_get(nf);
> + rcu_read_unlock();
Again:
657 static inline void *rhashtable_lookup_fast(
658 struct rhashtable *ht, const void *key,
659 const struct rhashtable_params params)
660 {
661 void *obj;
662
663 rcu_read_lock();
664 obj = rhashtable_lookup(ht, key, params);
665 rcu_read_unlock();
666
667 return obj;
668 }
Since rhashtable_lookup() itself is a public API, please
just call that in nfsd_file_do_acquire() after explicitly
taking the RCU read lock.
> if (nf)
> goto wait_for_construction;
>
> @@ -1090,21 +1092,21 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> goto out_status;
> }
>
> + rcu_read_lock();
> nf = rhashtable_lookup_get_insert_key(&nfsd_file_rhash_tbl,
> &key, &new->nf_rhash,
> nfsd_file_rhash_params);
> + if (!IS_ERR_OR_NULL(nf)) {
> + nf = nfsd_file_get(nf);
Note that nfsd_file_get() can still return NULL.
> + nfsd_file_slab_free(&new->nf_rcu);
Why is the slab_free call now inside the RCU critical section?
Granted this should be a rare case, but this adds unnecessary
latency while the read lock is held.
> + }
> + rcu_read_unlock();
Is there a problem replacing rhashtable_lookup_get_insert_key()
with rhashtable_lookup_insert_key() and just retrying the normal
lookup if insertion returns EEXIST? That way, an nfsd_file_get()
is necessary only at the rhashtable_lookup() call site above.
> if (!nf) {
> nf = new;
@new was just released above, so won't this set @nf to point
to freed memory in some cases?
> goto open_file;
> }
> if (IS_ERR(nf))
> goto insert_err;
> - nf = nfsd_file_get(nf);
> - if (nf == NULL) {
> - nf = new;
> - goto open_file;
> - }
> - nfsd_file_slab_free(&new->nf_rcu);
>
> wait_for_construction:
> wait_on_bit(&nf->nf_flags, NFSD_FILE_PENDING, TASK_UNINTERRUPTIBLE);
> --
> 2.37.3
--
Chuck Lever
On Sat, 2022-10-01 at 15:33 +0000, Chuck Lever III wrote:
> Hi Jeff-
>
> > On Oct 1, 2022, at 5:59 AM, Jeff Layton <[email protected]> wrote:
> >
> > nfsd_file is RCU-freed, so it's possible that one could be found that's
> > in the process of being freed and the memory recycled. Ensure we hold
> > the rcu_read_lock while attempting to get a reference on the object.
>
> I'm OK with entertaining clean-up patches in this code, but I
> am skeptical that this patch addresses the concern enumerated
> in bug #394. As you've pointed out to me before, the "UAF on
> DELEGRETURN crashes" appeared well before v5.19, which is the
> kernel release where this bit of code was introduced.
>
Yeah, there may be more than one bug here. In any case, these patches
should close potential races, so I think we ought to take them.
>
> > Cc: NeilBrown <[email protected]>
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > fs/nfsd/filecache.c | 14 ++++++++------
> > 1 file changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> > index d5c57360b418..f4f75ae2e4ea 100644
> > --- a/fs/nfsd/filecache.c
> > +++ b/fs/nfsd/filecache.c
> > @@ -1077,10 +1077,12 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >
> > retry:
> > /* Avoid allocation if the item is already in cache */
> > + rcu_read_lock();
> > nf = rhashtable_lookup_fast(&nfsd_file_rhash_tbl, &key,
> > nfsd_file_rhash_params);
> > if (nf)
> > nf = nfsd_file_get(nf);
> > + rcu_read_unlock();
>
> Again:
>
> 657 static inline void *rhashtable_lookup_fast(
> 658 struct rhashtable *ht, const void *key,
> 659 const struct rhashtable_params params)
> 660 {
> 661 void *obj;
> 662
> 663 rcu_read_lock();
> 664 obj = rhashtable_lookup(ht, key, params);
> 665 rcu_read_unlock();
> 666
> 667 return obj;
> 668 }
>
> Since rhashtable_lookup() itself is a public API, please
> just call that in nfsd_file_do_acquire() after explicitly
> taking the RCU read lock.
>
>
Understood. Sorry I missed your point. I'll fix that up.
> > if (nf)
> > goto wait_for_construction;
> >
> > @@ -1090,21 +1092,21 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > goto out_status;
> > }
> >
> > + rcu_read_lock();
> > nf = rhashtable_lookup_get_insert_key(&nfsd_file_rhash_tbl,
> > &key, &new->nf_rhash,
> > nfsd_file_rhash_params);
> > + if (!IS_ERR_OR_NULL(nf)) {
> > + nf = nfsd_file_get(nf);
>
> Note that nfsd_file_get() can still return NULL.
>
True, and that would leak. Good catch.
>
> > + nfsd_file_slab_free(&new->nf_rcu);
>
> Why is the slab_free call now inside the RCU critical section?
> Granted this should be a rare case, but this adds unnecessary
> latency while the read lock is held.
>
Fair point.
>
> > + }
> > + rcu_read_unlock();
>
> Is there a problem replacing rhashtable_lookup_get_insert_key()
> with rhashtable_lookup_insert_key() and just retrying the normal
> lookup if insertion returns EEXIST? That way, an nfsd_file_get()
> is necessary only at the rhashtable_lookup() call site above.
>
>
I like this idea, and it allows for a rather nice cleanup of the code.
I'll send a v3 set after I've had a chance to do some testing.
> > if (!nf) {
> > nf = new;
>
> @new was just released above, so won't this set @nf to point
> to freed memory in some cases?
>
>
> > goto open_file;
> > }
> > if (IS_ERR(nf))
> > goto insert_err;
> > - nf = nfsd_file_get(nf);
> > - if (nf == NULL) {
> > - nf = new;
> > - goto open_file;
> > - }
> > - nfsd_file_slab_free(&new->nf_rcu);
> >
> > wait_for_construction:
> > wait_on_bit(&nf->nf_flags, NFSD_FILE_PENDING, TASK_UNINTERRUPTIBLE);
> > --
> > 2.37.3
>
>
> --
> Chuck Lever
>
>
>
--
Jeff Layton <[email protected]>
> On Oct 1, 2022, at 12:40 PM, Jeff Layton <[email protected]> wrote:
>
> On Sat, 2022-10-01 at 15:33 +0000, Chuck Lever III wrote:
>> Hi Jeff-
>>
>>> On Oct 1, 2022, at 5:59 AM, Jeff Layton <[email protected]> wrote:
>>>
>>> nfsd_file is RCU-freed, so it's possible that one could be found that's
>>> in the process of being freed and the memory recycled. Ensure we hold
>>> the rcu_read_lock while attempting to get a reference on the object.
>>
>> I'm OK with entertaining clean-up patches in this code, but I
>> am skeptical that this patch addresses the concern enumerated
>> in bug #394. As you've pointed out to me before, the "UAF on
>> DELEGRETURN crashes" appeared well before v5.19, which is the
>> kernel release where this bit of code was introduced.
>>
>
> Yeah, there may be more than one bug here. In any case, these patches
> should close potential races, so I think we ought to take them.
Agreed, all of these are valid and desirable improvements.
I've applied the two from yesterday to my internal tree for more
testing. I plan to apply this one as well once the wrinkles are
ironed out. Since these are a bit late in the cycle, I plan to
push the set to Linus after the initial nfsd-6.1 PR, either near
the end of the merge window or in -rc1.
--
Chuck Lever