Bug 260071

Summary: bad slotid in SEQUENCE reply can crash NFS v4 client
Product: Base System Reporter: Robert Morris <rtm>
Component: kernAssignee: Rick Macklem <rmacklem>
Status: Closed FIXED    
Severity: Affects Some People CC: emaste, rmacklem
Priority: --- Flags: rmacklem: mfc-stable13+
rmacklem: mfc-stable12+
Version: CURRENT   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
A demo of an nfs v4 server crashing the client with a negative slotid.
none
Check that slot in Sequence reply is the same as in the request none

Description Robert Morris 2021-11-26 20:26:59 UTC
Created attachment 229750 [details]
A demo of an nfs v4 server crashing the client with a negative slotid.

This code in newnfs_request() uses a slot number sent by the server
to index into an array without bounds-checking:

                                        slot = fxdr_unsigned(int, *tl++);
                                        freeslot = slot;
                                        if (retseq != sep->nfsess_slotseq[slot])
                                                printf("retseq diff 0x%x\n",
                                                    retseq);

I've attached a demo:

# uname -a
FreeBSD  14.0-CURRENT FreeBSD 14.0-CURRENT #103 main-n250903-2a29c1558e82-dirty: Fri Nov 26 13:54:28 EST 2021     rtm@xxx:/usr/obj/usr/rtm/symbsd/src/riscv.riscv64/sys/RTM  riscv
# cc fnfs_3.c
# ./a.out
...
panic: Fatal page fault at 0xffffffc000207f4c: 0xffffffce02bf4cc0
--- exception 13, tval = 0xffffffce02bf4cc0
newnfs_request() at newnfs_request+0x1002
nfsrpc_reclaimcomplete() at nfsrpc_reclaimcomplete+0x92
nfsrpc_setclient() at nfsrpc_setclient+0x630
nfscl_getcl() at nfscl_getcl+0x53e
mountnfs() at mountnfs+0x722
nfs_mount() at nfs_mount+0x161e
vfs_mount_sigdefer() at vfs_mount_sigdefer+0x20
vfs_domount_first() at vfs_domount_first+0x1d4
vfs_domount() at vfs_domount+0x21c
vfs_donmount() at vfs_donmount+0x79c
sys_nmount() at sys_nmount+0x66
do_trap_user() at do_trap_user+0x206
cpu_exception_handler_user() at cpu_exception_handler_user+0x72
Comment 1 Rick Macklem freebsd_committer freebsd_triage 2021-11-27 02:09:07 UTC
Created attachment 229752 [details]
Check that slot in Sequence reply is the same as in the request

This patch should fix the crash.
It checks that the slotid in the Sequence reply
is the same as in the request and corrects it
if it is not the same.

Maybe the reporter can test it and confirm that it
fixes the problem for them?
Comment 2 Robert Morris 2021-11-27 10:10:29 UTC
(In reply to Rick Macklem from comment #1)
Yes -- the patch fixes the problem for me.
Comment 3 commit-hook freebsd_committer freebsd_triage 2021-11-27 23:06:25 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=1c15c8c0e935f3f7def7b03e9546f7d6790f465e

commit 1c15c8c0e935f3f7def7b03e9546f7d6790f465e
Author:     Rick Macklem <rmacklem@FreeBSD.org>
AuthorDate: 2021-11-27 23:02:04 +0000
Commit:     Rick Macklem <rmacklem@FreeBSD.org>
CommitDate: 2021-11-27 23:02:04 +0000

    nfscl: Sanity check the Sequence slotid in reply

    The slotid in the Sequence reply must be the same as
    in the request.  Check that it is the same and log
    a console message if it is not, plus set it to the
    correct value.

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

 sys/fs/nfs/nfs_commonkrpc.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
Comment 4 Rick Macklem freebsd_committer freebsd_triage 2021-11-28 01:04:13 UTC
Patch has been committed and will be MFC'd.
Comment 5 commit-hook freebsd_committer freebsd_triage 2021-12-11 02:32:09 UTC
A commit in branch stable/13 references this bug:

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

commit 3e8081ac8869e2b5b92d4a84c217a05cdd10487a
Author:     Rick Macklem <rmacklem@FreeBSD.org>
AuthorDate: 2021-11-27 23:02:04 +0000
Commit:     Rick Macklem <rmacklem@FreeBSD.org>
CommitDate: 2021-12-11 02:28:14 +0000

    nfscl: Sanity check the Sequence slotid in reply

    The slotid in the Sequence reply must be the same as
    in the request.  Check that it is the same and log
    a console message if it is not, plus set it to the
    correct value.

    PR:     260071

    (cherry picked from commit 1c15c8c0e935f3f7def7b03e9546f7d6790f465e)

 sys/fs/nfs/nfs_commonkrpc.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
Comment 6 commit-hook freebsd_committer freebsd_triage 2021-12-11 02:43:11 UTC
A commit in branch stable/12 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=9cd92bc5abdefb1897c3f948f50e66a3c21dff8e

commit 9cd92bc5abdefb1897c3f948f50e66a3c21dff8e
Author:     Rick Macklem <rmacklem@FreeBSD.org>
AuthorDate: 2021-11-27 23:02:04 +0000
Commit:     Rick Macklem <rmacklem@FreeBSD.org>
CommitDate: 2021-12-11 02:38:11 +0000

    nfscl: Sanity check the Sequence slotid in reply

    The slotid in the Sequence reply must be the same as
    in the request.  Check that it is the same and log
    a console message if it is not, plus set it to the
    correct value.

    PR:     260071

    (cherry picked from commit 1c15c8c0e935f3f7def7b03e9546f7d6790f465e)

 sys/fs/nfs/nfs_commonkrpc.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
Comment 7 Rick Macklem freebsd_committer freebsd_triage 2021-12-11 02:46:19 UTC
Patch has been MFC'd.