Bug 258504

Summary: smbfs doesn't validate msg fields -> potential kernel page fault
Product: Base System Reporter: Robert Morris <rtm>
Component: kernAssignee: John Baldwin <jhb>
Status: Closed FIXED    
Severity: Affects Only Me CC: emaste, grahamperrin, jhb
Priority: --- Keywords: crash
Version: CURRENT   
Hardware: Any   
OS: Any   
See Also: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=263043
Attachments:
Description Flags
demo to produce kernel page fault in smbfs code none

Description Robert Morris 2021-09-14 17:32:43 UTC 
Created attachment 227902 [details]
demo to produce kernel page fault in smbfs code

If the smbfs kernel code receives a message from the server
that has too-large values for the parameter offset/length
or data offset/length, the kernel can page fault. The problem
is that smb_t2_reply() reads fields out of reply messages
and uses them to adjust mbuf fields without validating. Adding
these lines to the start of smb_t2_placedata() is one way to
improve this situation:

    u_int ml = m_length(mtop, (struct mbuf **) 0);
    if(offset + count > ml)
      return 1;

I'm able to cause this crash in FreeBSD-RELEASE-p4 and last month's
CURRENT, on amd64.

I've attached a demonstration program. It expects samba to
be running on localhost, proxies a connection between smbfs
and samba, and sets the high bit of the parameter count field
of the 4th server message as it passes by. Here's the backtrace:

Fatal trap 12: page fault while in kernel mode
cpuid = 1; apic id = 01
fault virtual address   = 0x18
fault code              = supervisor write data, page not present
instruction pointer     = 0x20:0xffffffff8271afcd
stack pointer           = 0x0:0xfffffe00a41ad5c0
frame pointer           = 0x0:0xfffffe00a41ad6d0
code segment            = base rx0, limit 0xfffff, type 0x1b
                        = DPL 0, pres 1, long 1, def32 0, gran 1
processor eflags        = interrupt enabled, resume, IOPL = 0
current process         = 743 (mount_smbfs)
trap number             = 12
panic: page fault
cpuid = 1
time = 1631627929
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe00a41ad260
vpanic() at vpanic+0x187/frame 0xfffffe00a41ad2c0
panic() at panic+0x43/frame 0xfffffe00a41ad320
trap_fatal() at trap_fatal+0x387/frame 0xfffffe00a41ad380
trap_pfault() at trap_pfault+0x99/frame 0xfffffe00a41ad3e0
trap() at trap+0x2a7/frame 0xfffffe00a41ad4f0
calltrap() at calltrap+0x8/frame 0xfffffe00a41ad4f0
--- trap 0xc, rip = 0xffffffff8271afcd, rsp = 0xfffffe00a41ad5c0, rbp = 0xfffffe00a41ad6d0 ---
smb_t2_request() at smb_t2_request+0x83d/frame 0xfffffe00a41ad6d0
smbfs_smb_statfs() at smbfs_smb_statfs+0x76/frame 0xfffffe00a41ad740
smbfs_statfs() at smbfs_statfs+0x61/frame 0xfffffe00a41ad770
vfs_domount() at vfs_domount+0xa53/frame 0xfffffe00a41ad9e0
vfs_donmount() at vfs_donmount+0x880/frame 0xfffffe00a41ada80
sys_nmount() at sys_nmount+0x69/frame 0xfffffe00a41adac0
amd64_syscall() at amd64_syscall+0x12e/frame 0xfffffe00a41adbf0
fast_syscall_common() at fast_syscall_common+0xf8/frame 0xfffffe00a41adbf0
--- syscall (378, FreeBSD ELF64, sys_nmount), rip = 0x8011b7afa, rsp = 0x7fffffffe388, rbp = 0x7fffffffe9d0 ---
KDB: enter: panic
[ thread pid 743 tid 100111 ]
Stopped at      kdb_enter+0x37: movq    $0,0x127ae9e(%rip)
db> 

This is on FreeBSD xxx 14.0-CURRENT FreeBSD 14.0-CURRENT #0 main-n248636-d20e9e02db3: Thu Aug 12 05:47:18 UTC 2021     root@releng1.nyi.freebsd.org:/usr/obj/usr/src/amd64.amd64/sys/GENERIC  amd64
Comment 1 John Baldwin freebsd_committer freebsd_triage 2023-07-28 22:28:15 UTC 
I think there is also a bug in that smb_t2_placedata assumes that the last mbuf in the chain is large enough to contain all of the bits to be discarded.  I've replaced that with a call to m_adj() with a negative length which trims from the end while handling this case.

I have not yet tested a potential fix as setting up samba is a bit more work than some of the other bugs.  If it is not easy to test potential fix locally I can work on recreating the testing setup to test.

https://reviews.freebsd.org/D41229
Comment 2 Robert Morris 2023-07-29 17:39:05 UTC 
(In reply to John Baldwin from comment #1)
D41229 does fix the problem for me.

With the patch, I can mount a samba server and
read and write files with no problems.
Comment 3 commit-hook freebsd_committer freebsd_triage 2023-08-04 23:43:52 UTC 
A commit in branch main references this bug:

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

commit aca3d65fedffbbe71399a88d33ea8ecf550177eb
Author:     John Baldwin <jhb@FreeBSD.org>
AuthorDate: 2023-08-04 23:42:41 +0000
Commit:     John Baldwin <jhb@FreeBSD.org>
CommitDate: 2023-08-04 23:42:41 +0000

    netsmb: Add bounds checking to smb_t2_placedata

    Verify that the requested region of the mbuf chain is not beyond the
    end of the chain before trimming it from the end.  If it is out of
    bounds, fail with an error (EPROTO).

    While here, properly handle the case that the amount of data at the
    end of the chain might span more than one mbuf by using m_adj to drop
    the extra bytes rather than assuming m_len of the last mbuf can be
    adjusted directly.

    PR:             258504
    Reported by:    Robert Morris <rtm@lcs.mit.edu>
    Co-authored-by: Robert Morris <rtm@lcs.mit.edu>
    MFC after:      1 week
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D41229

 sys/netsmb/smb_rq.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)
Comment 4 commit-hook freebsd_committer freebsd_triage 2023-09-06 21:57:14 UTC 
A commit in branch stable/13 references this bug:

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

commit 5b8178fa46a766ece29aafcceed3c3db01f51a0b
Author:     John Baldwin <jhb@FreeBSD.org>
AuthorDate: 2023-08-04 23:42:41 +0000
Commit:     John Baldwin <jhb@FreeBSD.org>
CommitDate: 2023-09-06 21:56:10 +0000

    netsmb: Add bounds checking to smb_t2_placedata

    Verify that the requested region of the mbuf chain is not beyond the
    end of the chain before trimming it from the end.  If it is out of
    bounds, fail with an error (EPROTO).

    While here, properly handle the case that the amount of data at the
    end of the chain might span more than one mbuf by using m_adj to drop
    the extra bytes rather than assuming m_len of the last mbuf can be
    adjusted directly.

    PR:             258504
    Reported by:    Robert Morris <rtm@lcs.mit.edu>
    Co-authored-by: Robert Morris <rtm@lcs.mit.edu>
    MFC after:      1 week
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D41229

    (cherry picked from commit aca3d65fedffbbe71399a88d33ea8ecf550177eb)

 sys/netsmb/smb_rq.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)
Comment 5 commit-hook freebsd_committer freebsd_triage 2023-09-06 21:57:19 UTC 
A commit in branch stable/12 references this bug:

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

commit dd66ba430cb9d4c53fdd583fa2f20521552d58ff
Author:     John Baldwin <jhb@FreeBSD.org>
AuthorDate: 2023-08-04 23:42:41 +0000
Commit:     John Baldwin <jhb@FreeBSD.org>
CommitDate: 2023-09-06 20:03:18 +0000

    netsmb: Add bounds checking to smb_t2_placedata

    Verify that the requested region of the mbuf chain is not beyond the
    end of the chain before trimming it from the end.  If it is out of
    bounds, fail with an error (EPROTO).

    While here, properly handle the case that the amount of data at the
    end of the chain might span more than one mbuf by using m_adj to drop
    the extra bytes rather than assuming m_len of the last mbuf can be
    adjusted directly.

    PR:             258504
    Reported by:    Robert Morris <rtm@lcs.mit.edu>
    Co-authored-by: Robert Morris <rtm@lcs.mit.edu>
    MFC after:      1 week
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D41229

    (cherry picked from commit aca3d65fedffbbe71399a88d33ea8ecf550177eb)

 sys/netsmb/smb_rq.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)