Summary: | NFS v4 server crash due to ACL handling | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | Base System | Reporter: | Robert Morris <rtm> | ||||||
Component: | kern | Assignee: | 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: |
|
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. |
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