Bug 260076

Summary: bad slot in client SEQUENCE can crash NFS server
Product: Base System Reporter: Robert Morris <rtm>
Component: kernAssignee: Rick Macklem <rmacklem>
Status: Closed FIXED    
Severity: Affects Some People CC: emaste, pen, rmacklem, timf
Priority: --- Flags: rmacklem: mfc-stable13+
rmacklem: mfc-stable12+
Version: CURRENT   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
An NFS client that crashes the server with a bad slotid.
none
Don't set ND_HASSEQUNCE for NFSERR_BADSLOT none

Description Robert Morris 2021-11-27 12:11:46 UTC
Created attachment 229757 [details]
An NFS client that crashes the server with a bad slotid.

nfsrvd_sequence() accepts the client's slotid without checking
against 0..64:

  nd->nd_slotid = fxdr_unsigned(uint32_t, *tl++);

This can cause a crash when nd_slotid is later used to index
into sess_slots.

I've attached a demo:

# uname -a
FreeBSD  14.0-CURRENT FreeBSD 14.0-CURRENT #107 main-n250904-c4c468281fb6-dirty: Sat Nov 27 06:21:53 EST 2021     rtm@xxx:/usr/obj/usr/rtm/symbsd/src/riscv.riscv64/sys/RTM  riscv
# cc fnfsd_6.c
# ./a.out
...
panic: Fatal page fault at 0xffffffc000317a82: 0x4000000000000000
--- exception 13, tval = 0x4000000000000000
m_free() at m_free+0x10
m_freem() at m_freem+0x22
nfsv4_seqsess_cacherep() at nfsv4_seqsess_cacherep+0x56
nfsrv_cache_session() at nfsrv_cache_session+0x114
nfssvc_program() at nfssvc_program+0x624
svc_run_internal() at svc_run_internal+0x808
svc_thread_start() at svc_thread_start+0xe
fork_exit() at fork_exit+0x68
fork_trampoline() at fork_trampoline+0xa
Comment 1 Rick Macklem freebsd_committer freebsd_triage 2021-11-28 23:42:14 UTC
Created attachment 229772 [details]
Don't set ND_HASSEQUNCE for NFSERR_BADSLOT

For the NFSERR_BADSLOT failure, do not set
ND_HASSEQUENCE because the nd_slotid is not
valid. This should avoid the crash.

Maybe the reporter can test the patch?
Comment 2 Tim Foster 2021-11-29 00:24:16 UTC
(In reply to Robert Morris from comment #0)
Comment 3 Tim Foster 2021-11-29 00:26:54 UTC
(In reply to Tim Foster from comment #2)
Apologies - was browsing bugs here and seem to have inadvertently added a comment to this one. Please ignore it.
Comment 4 Robert Morris 2021-11-29 14:46:10 UTC
(In reply to Rick Macklem from comment #1)
Yes -- the patch makes the crash go away for me.
Comment 5 commit-hook freebsd_committer freebsd_triage 2021-12-01 21:50:59 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=33d0be8a923a840ec0724d50815890c5ffe0e884

commit 33d0be8a923a840ec0724d50815890c5ffe0e884
Author:     Rick Macklem <rmacklem@FreeBSD.org>
AuthorDate: 2021-12-01 21:46:41 +0000
Commit:     Rick Macklem <rmacklem@FreeBSD.org>
CommitDate: 2021-12-01 21:46:41 +0000

    nfsd: Do not try to cache a reply for NFSERR_BADSLOT

    When nfsrv_checksequence() replies NFSERR_BADSLOT,
    the value of nd_slotid is not valid.  As such, the
    reply cannot be cached in the session.
    Do not set ND_HASSEQUENCE for this case.

    Reported by:    rtm@lcs.mit.edu
    Tested by:      rtm@lcs.mit.edu
    PR:     260076
    MFC after:      2 weeks

 sys/fs/nfsserver/nfs_nfsdserv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Comment 6 Rick Macklem freebsd_committer freebsd_triage 2021-12-01 22:10:45 UTC
The patch has now been comitted and will be MFC'd.
Comment 7 commit-hook freebsd_committer freebsd_triage 2021-12-15 01:38:16 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=6a6b08d4646e109222a995c8432c025266b6101a

commit 6a6b08d4646e109222a995c8432c025266b6101a
Author:     Rick Macklem <rmacklem@FreeBSD.org>
AuthorDate: 2021-12-01 21:46:41 +0000
Commit:     Rick Macklem <rmacklem@FreeBSD.org>
CommitDate: 2021-12-15 01:34:05 +0000

    nfsd: Do not try to cache a reply for NFSERR_BADSLOT

    When nfsrv_checksequence() replies NFSERR_BADSLOT,
    the value of nd_slotid is not valid.  As such, the
    reply cannot be cached in the session.
    Do not set ND_HASSEQUENCE for this case.

    PR:     260076

    (cherry picked from commit 33d0be8a923a840ec0724d50815890c5ffe0e884)

 sys/fs/nfsserver/nfs_nfsdserv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Comment 8 commit-hook freebsd_committer freebsd_triage 2021-12-15 01:44:19 UTC
A commit in branch stable/12 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=3a2be1fb9906a78841ab5266b9d75128569a2ca5

commit 3a2be1fb9906a78841ab5266b9d75128569a2ca5
Author:     Rick Macklem <rmacklem@FreeBSD.org>
AuthorDate: 2021-12-01 21:46:41 +0000
Commit:     Rick Macklem <rmacklem@FreeBSD.org>
CommitDate: 2021-12-15 01:40:12 +0000

    nfsd: Do not try to cache a reply for NFSERR_BADSLOT

    When nfsrv_checksequence() replies NFSERR_BADSLOT,
    the value of nd_slotid is not valid.  As such, the
    reply cannot be cached in the session.
    Do not set ND_HASSEQUENCE for this case.

    PR:     260076

    (cherry picked from commit 33d0be8a923a840ec0724d50815890c5ffe0e884)

 sys/fs/nfsserver/nfs_nfsdserv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Comment 9 Rick Macklem freebsd_committer freebsd_triage 2021-12-15 01:46:17 UTC
Patch has been committed and MFC'd.