Bug 260111

Summary: NFS v4 server crash due to ACL handling
Product: Base System Reporter: Robert Morris <rtm>
Component: kernAssignee: Rick Macklem <rmacklem>
Status: Closed FIXED    
Severity: Affects Some People CC: emaste, pen, rmacklem
Priority: --- Flags: rmacklem: mfc-stable13+
rmacklem: mfc-stable12+
Version: CURRENT   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
Crash in NFS server's nfsm_advance() due to bad ACL attr.
none
Sanity check the acecnt and who size, plus fix a nfsv4_dissectacl() error case none

Description Robert Morris 2021-11-29 11:18:59 UTC
Created attachment 229786 [details]
Crash in NFS server's nfsm_advance()  due to bad ACL attr.

nfsrv_dissectacl() doesn't set *aclsizep if nfsrv_skipace() returns an
error. But there are paths through the NFSATTRBIT_ACL code in
nfsv4_loadattr() in which attrsum += cnt despite nfsrv_dissectacl()
returning an error -- in which case cnt is never initialized or
contains junk from a previous attribute. As a result
attrsum can end up negative, so that nfsv4_loadattr()
calls nfsm_advance() at the end even though maybe it shouldn't.

Further, when nfsv4_loadattr() is called due to a client's VERIFY, and
the VERIFY contains an ACL attribute to compare, but the file has no
ACL, nfsrv_dissectacl() calls nfsrv_skipace(). nfsrv_skipace() skips
forward in the query by the client-supplied length of the "who"
string. If client specifies a huge length, nfsrv_skipace()'s call to
nfsm_advance() will leave nd->nd_md == 0. Then the call to
nfsm_advance() at the end of of nfsv4_loadattr() will crash when it
tries to use nd->nd_md.

I've attached a demo:

# uname -a
FreeBSD  14.0-CURRENT FreeBSD 14.0-CURRENT #109 main-n250904-c4c468281fb6-dirty: Sun Nov 28 06:24:00 EST 2021     rtm@xxx:/usr/obj/usr/rtm/symbsd/src/riscv.riscv64/sys/RTM  riscv
# cc fnfsd_7.c
# ./a.out
...
panic: Fatal page fault at 0xffffffc0002107a6: 0x00000000000010
cpuid = 0
time = 1638181422
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 = 0x10
nfsv4_loadattr() at nfsv4_loadattr+0x3efe
nfsrvd_verify() at nfsrvd_verify+0xd6
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-12-01 00:47:04 UTC
Created attachment 229822 [details]
Sanity check the acecnt and who size, plus fix a nfsv4_dissectacl() error case

This patch should fix the crash, plus a couple of
other things. It...
- Adds an if (error) check for the nfsrv_dissectacl()
  call that didn't have it to avoid the nfsm_advance()
  call at the end of nfsv4_loadattr(), as noted by Robert.
- Adds a sanity check for the acecnt. (Checking for negative
  was the minimal check needed, but I also included an upper bound.)
  (As an aside, I should probably have made most of these count
   variables uint32_t instead of int, but that was 20years ago.)
- Add a sanity check on the size of the "who" string, since a data
  buffer for it is malloc()d.

Maybe Robert can check to see this patch fixes the crash for him?
Comment 2 Robert Morris 2021-12-01 11:37:31 UTC
(In reply to Rick Macklem from comment #1)
Yes -- the patches make this crash go away for me.
Comment 3 commit-hook freebsd_committer 2021-12-01 21:59:02 UTC
A commit in branch main references this bug:

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

commit fd020f197d6ac481d36171ea69fe555eb911d673
Author:     Rick Macklem <rmacklem@FreeBSD.org>
AuthorDate: 2021-12-01 21:55:17 +0000
Commit:     Rick Macklem <rmacklem@FreeBSD.org>
CommitDate: 2021-12-01 21:55:17 +0000

    nfsd: Sanity check the ACL attribute

    When an ACL is presented to the NFSv4 server in
    Setattr or Verify, parsing of the ACL assumed a
    sane acecnt and sane sizes for the "who" strings.
    This patch adds sanity checks for these.

    The patch also fixes handling of an error
    return from nfsrv_dissectacl() for one broken
    case.

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

 sys/fs/nfs/nfs_commonacl.c  |  6 +++++-
 sys/fs/nfs/nfs_commonsubs.c | 10 ++++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)
Comment 4 Rick Macklem freebsd_committer 2021-12-01 22:11:55 UTC
The patch has been committed and will be MFC'd.
Comment 5 commit-hook freebsd_committer 2021-12-15 01:36:14 UTC
A commit in branch stable/13 references this bug:

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

commit 081d40b8ea244d0700dd2b6031f9722394e3ce09
Author:     Rick Macklem <rmacklem@FreeBSD.org>
AuthorDate: 2021-12-01 21:55:17 +0000
Commit:     Rick Macklem <rmacklem@FreeBSD.org>
CommitDate: 2021-12-15 01:32:27 +0000

    nfsd: Sanity check the ACL attribute

    When an ACL is presented to the NFSv4 server in
    Setattr or Verify, parsing of the ACL assumed a
    sane acecnt and sane sizes for the "who" strings.
    This patch adds sanity checks for these.

    The patch also fixes handling of an error
    return from nfsrv_dissectacl() for one broken
    case.

    PR:     260111

    (cherry picked from commit fd020f197d6ac481d36171ea69fe555eb911d673)

 sys/fs/nfs/nfs_commonacl.c  |  6 +++++-
 sys/fs/nfs/nfs_commonsubs.c | 10 ++++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)
Comment 6 commit-hook freebsd_committer 2021-12-15 01:42:18 UTC
A commit in branch stable/12 references this bug:

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

commit b5c577931db12c68f5a20fa11c9502049c3cdb40
Author:     Rick Macklem <rmacklem@FreeBSD.org>
AuthorDate: 2021-12-01 21:55:17 +0000
Commit:     Rick Macklem <rmacklem@FreeBSD.org>
CommitDate: 2021-12-15 01:38:56 +0000

    nfsd: Sanity check the ACL attribute

    When an ACL is presented to the NFSv4 server in
    Setattr or Verify, parsing of the ACL assumed a
    sane acecnt and sane sizes for the "who" strings.
    This patch adds sanity checks for these.

    The patch also fixes handling of an error
    return from nfsrv_dissectacl() for one broken
    case.

    PR:     260111

    (cherry picked from commit fd020f197d6ac481d36171ea69fe555eb911d673)

 sys/fs/nfs/nfs_commonacl.c  |  6 +++++-
 sys/fs/nfs/nfs_commonsubs.c | 10 ++++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)
Comment 7 Rick Macklem freebsd_committer 2021-12-15 01:45:25 UTC
Patch has been committed and MFC'd.