Bug 260293

Summary: big counts in LAYOUTRETURN can cause NFS v4 nfsrv_flexlayouterr() to page-fault
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
Crash an NFS v4 server with a bad LAYOUTRETURN
none
check against maxcnt when parsing a flex file error reply none

Description Robert Morris 2021-12-09 17:22:18 UTC
Created attachment 229996 [details]
Crash an NFS v4 server with a bad LAYOUTRETURN

nfsrv_flexlayouterr() uses counts (cnt and errcnt) read from the
client's RPC request without a sanity check, which can cause tl to run
off the end of the allocated space and eventually cause a page fault.

I've attached a demo:

# uname -a
FreeBSD  14.0-CURRENT FreeBSD 14.0-CURRENT #138 main-n250910-5ebdc4cea5b6-dirty: Thu Dec  9 12:07:05 EST 2021     rtm@xxx:/usr/obj/usr/rtm/symbsd/src/riscv.riscv64/sys/RTM  riscv
# cc fnfsd_11.c
# ./a.out
...
panic: Fatal page fault at 0xffffffc0001b79d8: 0xffffffd080000005
--- exception 13, tval = 0xffffffd080000005
nfsrv_flexlayouterr() at nfsrv_flexlayouterr+0x132
nfsrv_layoutreturn() at nfsrv_layoutreturn+0x3a8
nfsrvd_layoutreturn() at nfsrvd_layoutreturn+0x328
nfsrvd_compound() at nfsrvd_compound+0xdc8
nfsrvd_dorpc() at nfsrvd_dorpc+0x294
nfs_proc() at nfs_proc+0x11a
nfssvc_program() at nfssvc_program+0x426
svc_executereq() at svc_executereq+0xa2
svc_run_internal() at svc_run_internal+0x302
svc_run() at svc_run+0x146
nfsrvd_nfsd() at nfsrvd_nfsd+0x194
nfssvc_nfsd() at nfssvc_nfsd+0x300
sys_nfssvc() at sys_nfssvc+0xdc
syscallenter() at syscallenter+0xf4
ecall_handler() at ecall_handler+0x18
do_trap_user() at do_trap_user+0xea
cpu_exception_handler_user() at cpu_exception_handler_user+0x72
Comment 1 Rick Macklem freebsd_committer freebsd_triage 2021-12-13 05:30:46 UTC
Created attachment 230070 [details]
check against maxcnt when parsing a flex file error reply

This patch decrements maxcnt by the appropriate
number of bytes during parsing and checks to see
if there is data remaining.  If not, it just returns
from nfsrv_flexlayouterr() without further processing.

This should fix the crashes.

Maybe the reporter can check to confirm that the patch
fixes the problem for him?
Comment 2 Robert Morris 2021-12-13 16:14:04 UTC
(In reply to Rick Macklem from comment #1)
The patch makes the problem go away for me.
Comment 3 commit-hook freebsd_committer freebsd_triage 2021-12-13 23:25:48 UTC
A commit in branch main references this bug:

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

commit c302f889e21f73746a3b0917df5246e639df1481
Author:     Rick Macklem <rmacklem@FreeBSD.org>
AuthorDate: 2021-12-13 23:21:31 +0000
Commit:     Rick Macklem <rmacklem@FreeBSD.org>
CommitDate: 2021-12-13 23:21:31 +0000

    nfsd: Limit parsing of layout errors to maxcnt bytes

    This patch decrements maxcnt by the appropriate
    number of bytes during parsing and checks to see
    if there is data remaining.  If not, it just returns
    from nfsrv_flexlayouterr() without further processing.
    This prevents the tl pointer from running off the end
    of the error data pointed at by layp, if there are
    flaws in the data.

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

 sys/fs/nfsserver/nfs_nfsdstate.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
Comment 4 commit-hook freebsd_committer freebsd_triage 2021-12-27 00:57:52 UTC
A commit in branch stable/13 references this bug:

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

commit 030acb63d9a86b9a7bd15b06e60699abfa8a0a2b
Author:     Rick Macklem <rmacklem@FreeBSD.org>
AuthorDate: 2021-12-13 23:21:31 +0000
Commit:     Rick Macklem <rmacklem@FreeBSD.org>
CommitDate: 2021-12-27 00:53:50 +0000

    nfsd: Limit parsing of layout errors to maxcnt bytes

    This patch decrements maxcnt by the appropriate
    number of bytes during parsing and checks to see
    if there is data remaining.  If not, it just returns
    from nfsrv_flexlayouterr() without further processing.
    This prevents the tl pointer from running off the end
    of the error data pointed at by layp, if there are
    flaws in the data.

    PR:     260293

    (cherry picked from commit c302f889e21f73746a3b0917df5246e639df1481)

 sys/fs/nfsserver/nfs_nfsdstate.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
Comment 5 commit-hook freebsd_committer freebsd_triage 2021-12-27 01:05:10 UTC
A commit in branch stable/12 references this bug:

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

commit e4a65cff230dd1e055ad1651f285e5e11b160cb8
Author:     Rick Macklem <rmacklem@FreeBSD.org>
AuthorDate: 2021-12-13 23:21:31 +0000
Commit:     Rick Macklem <rmacklem@FreeBSD.org>
CommitDate: 2021-12-27 01:00:57 +0000

    nfsd: Limit parsing of layout errors to maxcnt bytes

    This patch decrements maxcnt by the appropriate
    number of bytes during parsing and checks to see
    if there is data remaining.  If not, it just returns
    from nfsrv_flexlayouterr() without further processing.
    This prevents the tl pointer from running off the end
    of the error data pointed at by layp, if there are
    flaws in the data.

    PR:     260293

    (cherry picked from commit c302f889e21f73746a3b0917df5246e639df1481)

 sys/fs/nfsserver/nfs_nfsdstate.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
Comment 6 Rick Macklem freebsd_committer freebsd_triage 2021-12-27 01:07:08 UTC
Patch has been committed and MFC'd.