Bug 260046

Summary: An NFS v4 client can crash the server with a LISTXATTRS RPC
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+
Version: CURRENT   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
Demo of an NFS client crashing a server with LISTXATTRS.
none
Handle the case of a small maxcount < minimum reply none

Description Robert Morris 2021-11-25 12:10:04 UTC
Created attachment 229724 [details]
Demo of an NFS client crashing a server with LISTXATTRS.

nfsrvd_listxattr() calls nfsvno_listxattr(). The latter can return
zero at the same time as leaving buf == NULL. But if the return value
is zero, nfsrvd_listxattr() dereferences the NULL buf, causing a
crash.

I've attached a demo:

# uname -a
FreeBSD  14.0-CURRENT FreeBSD 14.0-CURRENT #99 main-n250901-77e3db078984-dirty: Wed Nov 24 05:42:01 EST 2021     rtm@xxx:/usr/obj/usr/rtm/symbsd/src/riscv.riscv64/sys/RTM  riscv
# cc fnfsd_3.c
# ./a.out
...
panic: Fatal page fault at 0xffffffc00026f256: 0000000000000000
cpuid = 0
time = 1637841164
KDB: stack backtrace:
db_trace_self() at db_trace_self
db_trace_self_wrapper() at db_trace_self_wrapper+0x38
kdb_backtrace() at kdb_backtrace+0x2c
vpanic() at vpanic+0x154
panic() at panic+0x2a
page_fault_handler() at page_fault_handler+0x1ee
do_trap_supervisor() at do_trap_supervisor+0x76
cpu_exception_handler_supervisor() at cpu_exception_handler_supervisor+0x70
--- exception 13, tval = 0
nfsrvd_listxattr() at nfsrvd_listxattr+0x306
nfsrvd_dorpc() at nfsrvd_dorpc+0x154c
nfssvc_program() at nfssvc_program+0x5a4
svc_run_internal() at svc_run_internal+0x808
svc_run() at svc_run+0x1a8
nfsrvd_nfsd() at nfsrvd_nfsd+0x30e
nfssvc_nfsd() at nfssvc_nfsd+0x386
sys_nfssvc() at sys_nfssvc+0xd0
do_trap_user() at do_trap_user+0x206
cpu_exception_handler_user() at cpu_exception_handler_user+0x72
--- exception 8, tval = 0
Comment 1 Rick Macklem freebsd_committer 2021-11-26 02:56:11 UTC
Created attachment 229735 [details]
Handle the case of a small maxcount < minimum reply

I think this patch will fix the crash
(although the demo program used a maxcount == 16K),
which I don't think would cause the crash.
Anyhow, it tests ok with 16K and with 5, so I
think it deals with the problem.

Maybe the reporter can test this patch?
Comment 2 Robert Morris 2021-11-26 16:45:39 UTC
(In reply to Rick Macklem from comment #1)
The patch fixes the crash for me -- thank you.
Comment 3 commit-hook freebsd_committer 2021-11-27 00:00:34 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=5b430a132330bd4a4ea37780807947f3800d009e

commit 5b430a132330bd4a4ea37780807947f3800d009e
Author:     Rick Macklem <rmacklem@FreeBSD.org>
AuthorDate: 2021-11-26 23:56:29 +0000
Commit:     Rick Macklem <rmacklem@FreeBSD.org>
CommitDate: 2021-11-26 23:56:29 +0000

    nfsd: Sanity check the len argument for ListXattr

    The check for the original len being >= retlen needs to
    be done before the "if (nd->nd_repstat == 0)" code, so
    that it can be reported as too small.

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

 sys/fs/nfsserver/nfs_nfsdserv.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
Comment 4 Rick Macklem freebsd_committer 2021-11-27 00:02:24 UTC
The patch has been committed to main and will be MFC'd.
Comment 5 commit-hook freebsd_committer 2021-12-11 02:30:08 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=5b76a6b862ad5ea35c3f9a4572b6388acdfd3de7

commit 5b76a6b862ad5ea35c3f9a4572b6388acdfd3de7
Author:     Rick Macklem <rmacklem@FreeBSD.org>
AuthorDate: 2021-11-26 23:56:29 +0000
Commit:     Rick Macklem <rmacklem@FreeBSD.org>
CommitDate: 2021-12-11 02:26:22 +0000

    nfsd: Sanity check the len argument for ListXattr

    The check for the original len being >= retlen needs to
    be done before the "if (nd->nd_repstat == 0)" code, so
    that it can be reported as too small.

    PR:     260046

    (cherry picked from commit 5b430a132330bd4a4ea37780807947f3800d009e)

 sys/fs/nfsserver/nfs_nfsdserv.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
Comment 6 Rick Macklem freebsd_committer 2021-12-11 02:45:20 UTC
Patch has been MFC'd.