Bug 260046 - An NFS v4 client can crash the server with a LISTXATTRS RPC
Summary: An NFS v4 client can crash the server with a LISTXATTRS RPC
Status: In Progress
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Rick Macklem
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-11-25 12:10 UTC by Robert Morris
Modified: 2021-11-27 00:02 UTC (History)
2 users (show)

See Also:
rmacklem: mfc-stable13?


Attachments
Demo of an NFS client crashing a server with LISTXATTRS. (28.68 KB, text/plain)
2021-11-25 12:10 UTC, Robert Morris
no flags Details
Handle the case of a small maxcount < minimum reply (700 bytes, patch)
2021-11-26 02:56 UTC, Rick Macklem
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.