Bug 260012

Summary: NFS v4 client can crash server with a bad LAYOUTRETURN 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+
rmacklem: mfc-stable12+
Version: CURRENT   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
An NFS client that crashes the server with a bad LAYOUTRETURN.
none
Check for a non-NULL layp before calling nfsrv_flexlayouterr() none

Description Robert Morris 2021-11-24 09:47:47 UTC
Created attachment 229690 [details]
An NFS client that crashes the server with a bad LAYOUTRETURN.

If an NFS v4 client sends a LAYOUTRETURN RPC with lr_returntype of
NFSV4LAYOURRET_FILE and an lrf_body<> size <= 0, nfsrvd_layoutreturn()
will pass a NULL layp to nfsrv_layoutreturn(). If the RPC also has
layouttype==NFSV4LAYOUT_FLEXFILE (4), nfsrv_layoutreturn() will call
nfsrv_flexlayouterr(). The latter dereferences layp (as tl) without
checking for NULL.

I've attached a demo:

# uname -a
FreeBSD  14.0-CURRENT FreeBSD 14.0-CURRENT #96 main-n250901-77e3db078984-dirty: Tue Nov 23 16:58:28 EST 2021     rtm@xxx:/usr/obj/usr/rtm/symbsd/src/riscv.riscv64/sys/RTM  riscv
# cc fnfsd_1.c
# ./a.out
...
nfsrv_layoutreturn: updatemdsattr failed=2
panic: Fatal page fault at 0xffffffc00025ce8e: 0000000000000000
cpuid = 0
time = 1637591542
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
nfsrv_layoutreturn() at nfsrv_layoutreturn+0xa6
nfsrvd_layoutreturn() at nfsrvd_layoutreturn+0x35c
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
Comment 1 Rick Macklem freebsd_committer freebsd_triage 2021-11-25 23:58:35 UTC
Created attachment 229733 [details]
Check for a non-NULL layp before calling nfsrv_flexlayouterr()

This patch should avoid the crash.
While here I also added a sanity check on the
size of the error data.

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

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

commit bdd57cbb1bdafcf2ebffa73c52f0fffc9410ea7b
Author:     Rick Macklem <rmacklem@FreeBSD.org>
AuthorDate: 2021-11-26 23:42:32 +0000
Commit:     Rick Macklem <rmacklem@FreeBSD.org>
CommitDate: 2021-11-26 23:42:32 +0000

    nfsd: Add checks for layout errors in LayoutReturn

    For a LayoutReturn when using the Flexible File Layout,
    error reports may be provided in the request.
    Sanity check the size of these error reports and
    check that they exist before calling nfsrv_flexlayouterr().

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

 sys/fs/nfsserver/nfs_nfsdserv.c  | 6 ++++++
 sys/fs/nfsserver/nfs_nfsdstate.c | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)
Comment 4 Rick Macklem freebsd_committer freebsd_triage 2021-11-26 23:47:58 UTC
The patch has been committed to main and will be MFC'd.
Comment 5 commit-hook freebsd_committer freebsd_triage 2021-12-10 00:59:45 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=8e74cc2b4ec090e592cc808e55a6936207b4d302

commit 8e74cc2b4ec090e592cc808e55a6936207b4d302
Author:     Rick Macklem <rmacklem@FreeBSD.org>
AuthorDate: 2021-11-26 23:42:32 +0000
Commit:     Rick Macklem <rmacklem@FreeBSD.org>
CommitDate: 2021-12-10 00:55:47 +0000

    nfsd: Add checks for layout errors in LayoutReturn

    For a LayoutReturn when using the Flexible File Layout,
    error reports may be provided in the request.
    Sanity check the size of these error reports and
    check that they exist before calling nfsrv_flexlayouterr().

    PR:     260012

    (cherry picked from commit bdd57cbb1bdafcf2ebffa73c52f0fffc9410ea7b)

 sys/fs/nfsserver/nfs_nfsdserv.c  | 6 ++++++
 sys/fs/nfsserver/nfs_nfsdstate.c | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)
Comment 6 commit-hook freebsd_committer freebsd_triage 2021-12-10 01:07:48 UTC
A commit in branch stable/12 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=0f2244008573e4a3d8dd4131972eefbf1bec681e

commit 0f2244008573e4a3d8dd4131972eefbf1bec681e
Author:     Rick Macklem <rmacklem@FreeBSD.org>
AuthorDate: 2021-11-26 23:42:32 +0000
Commit:     Rick Macklem <rmacklem@FreeBSD.org>
CommitDate: 2021-12-10 01:04:17 +0000

    nfsd: Add checks for layout errors in LayoutReturn

    For a LayoutReturn when using the Flexible File Layout,
    error reports may be provided in the request.
    Sanity check the size of these error reports and
    check that they exist before calling nfsrv_flexlayouterr().

    PR:     260012

    (cherry picked from commit bdd57cbb1bdafcf2ebffa73c52f0fffc9410ea7b)

 sys/fs/nfsserver/nfs_nfsdserv.c  | 6 ++++++
 sys/fs/nfsserver/nfs_nfsdstate.c | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)
Comment 7 Ed Maste freebsd_committer freebsd_triage 2022-04-23 02:34:37 UTC
Can this one be closed now?
Comment 8 Rick Macklem freebsd_committer freebsd_triage 2022-04-23 13:47:47 UTC
Patch has been committed and MFC'd.