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
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?
(In reply to Rick Macklem from comment #1) Yes -- the patches make this crash go away for me.
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(-)
The patch has been committed and will be MFC'd.
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(-)
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(-)
Patch has been committed and MFC'd.