On Fri, Oct 11, 2024 at 12:17 PM Jeff Layton jlayton@kernel.org wrote:
On Fri, 2024-10-11 at 10:39 -0400, Chuck Lever wrote:
On Thu, Oct 10, 2024 at 06:18:01PM -0400, Olga Kornievskaia wrote:
There is a race between laundromat handling of revoked delegations and a client sending free_stateid operation. Laundromat thread finds that delegation has expired and needs to be revoked so it marks the delegation stid revoked and it puts it on a reaper list but then it unlock the state lock and the actual delegation revocation happens without the lock. Once the stid is marked revoked a racing free_stateid processing thread does the following (1) it calls list_del_init() which removes it from the reaper list and (2) frees the delegation stid structure. The laundromat thread ends up not calling the revoke_delegation() function for this particular delegation but that means it will no release the lock lease that exists on the file.
Now, a new open for this file comes in and ends up finding that lease list isn't empty and calls nfsd_breaker_owns_lease() which ends up trying to derefence a freed delegation stateid. Leading to the followint use-after-free KASAN warning:
kernel: ================================================================== kernel: BUG: KASAN: slab-use-after-free in nfsd_breaker_owns_lease+0x140/0x160 [nfsd] kernel: Read of size 8 at addr ffff0000e73cd0c8 by task nfsd/6205 kernel: kernel: CPU: 2 UID: 0 PID: 6205 Comm: nfsd Kdump: loaded Not tainted 6.11.0-rc7+ #9 kernel: Hardware name: Apple Inc. Apple Virtualization Generic Platform, BIOS 2069.0.0.0.0 08/03/2024 kernel: Call trace: kernel: dump_backtrace+0x98/0x120 kernel: show_stack+0x1c/0x30 kernel: dump_stack_lvl+0x80/0xe8 kernel: print_address_description.constprop.0+0x84/0x390 kernel: print_report+0xa4/0x268 kernel: kasan_report+0xb4/0xf8 kernel: __asan_report_load8_noabort+0x1c/0x28 kernel: nfsd_breaker_owns_lease+0x140/0x160 [nfsd] kernel: leases_conflict+0x68/0x370 kernel: __break_lease+0x204/0xc38 kernel: nfsd_open_break_lease+0x8c/0xf0 [nfsd] kernel: nfsd_file_do_acquire+0xb3c/0x11d0 [nfsd] kernel: nfsd_file_acquire_opened+0x84/0x110 [nfsd] kernel: nfs4_get_vfs_file+0x634/0x958 [nfsd] kernel: nfsd4_process_open2+0xa40/0x1a40 [nfsd] kernel: nfsd4_open+0xa08/0xe80 [nfsd] kernel: nfsd4_proc_compound+0xb8c/0x2130 [nfsd] kernel: nfsd_dispatch+0x22c/0x718 [nfsd] kernel: svc_process_common+0x8e8/0x1960 [sunrpc] kernel: svc_process+0x3d4/0x7e0 [sunrpc] kernel: svc_handle_xprt+0x828/0xe10 [sunrpc] kernel: svc_recv+0x2cc/0x6a8 [sunrpc] kernel: nfsd+0x270/0x400 [nfsd] kernel: kthread+0x288/0x310 kernel: ret_from_fork+0x10/0x20
Proposing to have laundromat thread hold the state_lock over both marking thru revoking the delegation as well as making free_stateid acquire state_lock before accessing the list. Making sure that revoke_delegation() (ie kernel_setlease(unlock)) is called for every delegation that was revoked and added to the reaper list.
CC: stable@vger.kernel.org Signed-off-by: Olga Kornievskaia okorniev@redhat.com
--- I can't figure out the Fixes: tag. Laundromat's behaviour has been like that forever. But the free_stateid bits wont apply before the 1e3577a4521e ("SUNRPC: discard sv_refcnt, and svc_get/svc_put"). But we used that fixes tag already with a previous fix for a different problem.
fs/nfsd/nfs4state.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 9c2b1d251ab3..c97907d7fb38 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -6605,13 +6605,13 @@ nfs4_laundromat(struct nfsd_net *nn) unhash_delegation_locked(dp, SC_STATUS_REVOKED); list_add(&dp->dl_recall_lru, &reaplist); }
- spin_unlock(&state_lock); while (!list_empty(&reaplist)) { dp = list_first_entry(&reaplist, struct nfs4_delegation, dl_recall_lru); list_del_init(&dp->dl_recall_lru); revoke_delegation(dp); }
- spin_unlock(&state_lock);
Code review suggests revoke_delegation() (and in particular, destroy_unhashed_deleg(), must not be called while holding state_lock().
We'd be calling nfs4_unlock_deleg_lease with a spinlock held, which is sort of gross.
That said, I don't love this fix either.
spin_lock(&nn->client_lock); while (!list_empty(&nn->close_lru)) {@@ -7213,7 +7213,9 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, if (s->sc_status & SC_STATUS_REVOKED) { spin_unlock(&s->sc_lock); dp = delegstateid(s);
spin_lock(&state_lock); list_del_init(&dp->dl_recall_lru);spin_unlock(&state_lock);Existing code is inconsistent about how manipulation of dl_recall_lru is protected. Most instances do use state_lock for this purpose, but a few, including this one, use cl->cl_lock. Does the other instance using cl_lock need review and correction as well?
I'd prefer to see this fix make the protection of dl_recall_lru consistent everywhere.
Agreed. The locking around the delegation handling has been a mess for a long time. I'd really like to have a way to fix this that didn't require having to rework all of this code however.
How about something like this patch instead? Olga, thoughts?
I think this patch will prevent the UAF but it doesn't work for another reason (tested it too). As the free_stateid operation can come in before the freeable flag is set (and so the nfsd4_free_stateid function would not do anything). But it needs to remove this delegation from cl_revoked which the laundromat puts it on as a part of revoked_delegation() otherwise the server never clears recallable_state_revoked. And I think this put_stid() that free_stateid does is also needed. So what happens is free_stateid comes and goes and the sequence flag is set and is never cleared.
Laundromat threat when it starts revocation process it either needs to 'finish it' but it needs to make sure that if free_stateid arrives in the meanwhile it has to wait but still run. Or I was thinking that perhaps, we can make free_stateid act like delegreturn. but I wasn't sure if free_stateid is allowed to act like delegreturn. but this also fixes the problem if the free_stateid arrives and takes the work away from the laundromat thread then free_stateid finishes the return just like a delegreturn (which unlocks the lease). But I'm unclear if there isn't any races between revoke_delegation and destroy_delegation.
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 56b261608af4..1ef6933b1ccb 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -7159,6 +7159,7 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, dp = delegstateid(s); list_del_init(&dp->dl_recall_lru); spin_unlock(&cl->cl_lock); + destroy_delegation(dp); nfs4_put_stid(s); ret = nfs_ok; goto out;
[PATCH] nfsd: add new SC_STATUS_FREEABLE to prevent race with FREE_STATEID
Olga identified a race between the laundromat and FREE_STATEID handling. The crux of the problem is that free_stateid can proceed while the laundromat is still processing the revocation.
Add a new SC_STATUS_FREEABLE flag that is set once the revocation is complete. Have nfsd4_free_stateid only consider delegations that have this flag set.
Reported-by: Olga Kornievskaia okorniev@redhat.com Signed-off-by: Jeff Layton jlayton@kernel.org
fs/nfsd/nfs4state.c | 3 ++- fs/nfsd/state.h | 1 + 2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 73c4b983c048..b71a2cc7f2dd 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -1371,6 +1371,7 @@ static void revoke_delegation(struct nfs4_delegation *dp) spin_lock(&clp->cl_lock); refcount_inc(&dp->dl_stid.sc_count); list_add(&dp->dl_recall_lru, &clp->cl_revoked);
dp->dl_stid.sc_status |= SC_STATUS_FREEABLE; spin_unlock(&clp->cl_lock); } destroy_unhashed_deleg(dp);@@ -7207,7 +7208,7 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, spin_lock(&s->sc_lock); switch (s->sc_type) { case SC_TYPE_DELEG:
if (s->sc_status & SC_STATUS_REVOKED) {
if (s->sc_status & SC_STATUS_FREEABLE) { spin_unlock(&s->sc_lock); dp = delegstateid(s); list_del_init(&dp->dl_recall_lru);diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h index 874fcab2b183..4f3b941b09d3 100644 --- a/fs/nfsd/state.h +++ b/fs/nfsd/state.h @@ -114,6 +114,7 @@ struct nfs4_stid { /* For a deleg stateid kept around only to process free_stateid's: */ #define SC_STATUS_REVOKED BIT(1) #define SC_STATUS_ADMIN_REVOKED BIT(2) +#define SC_STATUS_FREEABLE BIT(3) unsigned short sc_status;
struct list_head sc_cp_list;-- 2.47.0