Bug 257768

Summary: Corrupt UDF disk image can cause crash when mounted.
Product: Base System Reporter: Robert Morris <rtm>
Component: kernAssignee: John Baldwin <jhb>
Status: Closed FIXED    
Severity: Affects Some People CC: emaste, grahamperrin, jhb
Priority: ---    
Version: 13.0-RELEASE   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
A corrupt UDF disk image that causes a crash when mounted. none

Description Robert Morris 2021-08-11 13:19:25 UTC 
If a corrupt (or malicious) UDF disk contains root_icb.len -2032,
udf_mountfs() ends up passing a size of zero to breadn_flags(),
which causes it to return a bp with an unmapped b_data, which causes
udf_mountfs()/udf_checktag() to pagefault. You can see this with the
attached UDF image, using

  mdconfig -f bad-udf.iso
  mount_udf /dev/md0 /mnt

which yields (on my amd64 13.0-RELEASE-p3 machine):

panic: vm_fault_lookup: fault on nofault entry, addr: 0xfffffe0009cd8000
cpuid = 0
time = 1628673343
KDB: stack backtrace:
#0 0xffffffff80c57515 at kdb_backtrace+0x65
#1 0xffffffff80c09ef1 at vpanic+0x181
#2 0xffffffff80c09d63 at panic+0x43
#3 0xffffffff80f289db at vm_fault+0x144b
#4 0xffffffff80f274b1 at vm_fault_trap+0xb1
#5 0xffffffff8108b3b8 at trap_pfault+0x1f8
#6 0xffffffff8108a86d at trap+0x27d
#7 0xffffffff810619a8 at calltrap+0x8
#8 0xffffffff80cdaa09 at vfs_domount+0x5e9
#9 0xffffffff80cd9c27 at vfs_donmount+0x8e7
#10 0xffffffff80cd9309 at sys_nmount+0x69
#11 0xffffffff8108babc at amd64_syscall+0x10c
#12 0xffffffff810622ce at fast_syscall_common+0xf8
Comment 1 Robert Morris 2021-08-11 14:02:26 UTC 
Created attachment 227112 [details]
A corrupt UDF disk image that causes a crash when mounted.
Comment 2 Graham Perrin freebsd_committer freebsd_triage 2021-09-05 16:29:00 UTC 
Comparably interesting: bug 244342 and seven other UFS-related kernel panic bugs that were reported by Neeraj on 2020-02-23
Comment 3 John Baldwin freebsd_committer freebsd_triage 2023-07-27 23:08:32 UTC 
Thanks for the report.  I have a UDF-specific fix at https://reviews.freebsd.org/D41220.

However, I somewhat worry that bread*() and getblk() have no checks for negative sizes in general, and struct buf is full of signed fields for lengths (b_bcount, b_length, b_kvasize) that really should all be unsigned I think.  I think the code effectively treats the values as unsigned in practice, but there might be some subtle bugs due to the signed lengths.
Comment 4 Robert Morris 2023-07-28 21:03:33 UTC 
(In reply to John Baldwin from comment #3)
Patch D41220 fixes the problem for me.
Comment 5 commit-hook freebsd_committer freebsd_triage 2023-08-04 23:43:53 UTC 
A commit in branch main references this bug:

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

commit c70e615051b00671d54651d99af5cdec4b091d92
Author:     John Baldwin <jhb@FreeBSD.org>
AuthorDate: 2023-08-04 23:40:19 +0000
Commit:     John Baldwin <jhb@FreeBSD.org>
CommitDate: 2023-08-04 23:40:19 +0000

    udf: Reject read requests with an invalid length

    - If the size is negative or if rounding it up to a multiple of
      the block size overflows, fail the read request with ERANGE.

    - While here, add a sanity check that the ICB length for the root
      directory is at least as long as a minimum-sized file entry.

    PR:             257768
    Reported by:    Robert Morris <rtm@lcs.mit.edu>
    MFC after:      1 week
    Sponsored by:   FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D41220

 sys/fs/udf/udf.h        | 4 +++-
 sys/fs/udf/udf_vfsops.c | 5 +++++
 2 files changed, 8 insertions(+), 1 deletion(-)
Comment 6 commit-hook freebsd_committer freebsd_triage 2023-09-06 21:57:17 UTC 
A commit in branch stable/13 references this bug:

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

commit 202c1d76218695ec094f289dbb23e96310eae2c1
Author:     John Baldwin <jhb@FreeBSD.org>
AuthorDate: 2023-08-04 23:40:19 +0000
Commit:     John Baldwin <jhb@FreeBSD.org>
CommitDate: 2023-09-06 21:56:09 +0000

    udf: Reject read requests with an invalid length

    - If the size is negative or if rounding it up to a multiple of
      the block size overflows, fail the read request with ERANGE.

    - While here, add a sanity check that the ICB length for the root
      directory is at least as long as a minimum-sized file entry.

    PR:             257768
    Reported by:    Robert Morris <rtm@lcs.mit.edu>
    MFC after:      1 week
    Sponsored by:   FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D41220

    (cherry picked from commit c70e615051b00671d54651d99af5cdec4b091d92)

 sys/fs/udf/udf.h        | 4 +++-
 sys/fs/udf/udf_vfsops.c | 5 +++++
 2 files changed, 8 insertions(+), 1 deletion(-)
Comment 7 commit-hook freebsd_committer freebsd_triage 2023-09-06 21:57:21 UTC 
A commit in branch stable/12 references this bug:

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

commit 0db7f4b419dbaa2c23a737393d35564cd0b2f35a
Author:     John Baldwin <jhb@FreeBSD.org>
AuthorDate: 2023-08-04 23:40:19 +0000
Commit:     John Baldwin <jhb@FreeBSD.org>
CommitDate: 2023-09-06 20:02:33 +0000

    udf: Reject read requests with an invalid length

    - If the size is negative or if rounding it up to a multiple of
      the block size overflows, fail the read request with ERANGE.

    - While here, add a sanity check that the ICB length for the root
      directory is at least as long as a minimum-sized file entry.

    PR:             257768
    Reported by:    Robert Morris <rtm@lcs.mit.edu>
    MFC after:      1 week
    Sponsored by:   FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D41220

    (cherry picked from commit c70e615051b00671d54651d99af5cdec4b091d92)

 sys/fs/udf/udf.h        | 4 +++-
 sys/fs/udf/udf_vfsops.c | 5 +++++
 2 files changed, 8 insertions(+), 1 deletion(-)