Bug 214423 - dosfs support in libstand is broken since r298230
Summary: dosfs support in libstand is broken since r298230
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 11.0-STABLE
Hardware: amd64 Any
: --- Affects Some People
Assignee: Allan Jude
URL:
Keywords: patch, regression
Depends on:
Blocks:
 
Reported: 2016-11-11 15:11 UTC by Mikhail.Kupchik
Modified: 2017-02-06 22:04 UTC (History)
2 users (show)

See Also:


Attachments
Patch for lib/libstand/dosfs.c (1.82 KB, patch)
2016-11-11 15:11 UTC, Mikhail.Kupchik
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail.Kupchik 2016-11-11 15:11:56 UTC
Created attachment 176899 [details]
Patch for lib/libstand/dosfs.c

Support for dosfs filesystem in loader.efi (via libstand) works as expected in FreeBSD 10.3, but is broken since FreeBSD 11.0.

How to reproduce this problem:
  # cd ~
  # dd if=/dev/zero of=mfsroot bs=1m count=12
  # set MDEV=`mdconfig -a -t vnode -f mfsroot`
  # newfs -O 1 /dev/$MDEV
  # mount /dev/$MDEV /mnt
  # mkdir /mnt/dev
  # mkdir /mnt/sbin
  # cp /rescue/sh /mnt/sbin/init
  # umount /mnt
  # mdconfig -d -u $MDEV
  # dd if=/dev/zero of=testbootfs bs=1m count=128
  # set MDEV=`mdconfig -a -t vnode -f testbootfs`
  # gpart create -s gpt $MDEV
  # gpart add -t efi $MDEV
  # newfs_msdos "${MDEV}p1"
  # mount -t msdosfs "/dev/${MDEV}p1" /mnt
  # mkdir /mnt/boot
  # mv mfsroot /mnt/boot/
  # cp /boot/kernel/kernel /mnt/boot/kernel
  # echo 'set vfs.root.mountfrom="ufs:/dev/md0"' > /mnt/boot/loader.rc
  # echo 'load /boot/kernel' >> /mnt/boot/loader.rc
  # echo 'load -t mfsroot /boot/mfsroot' >> /mnt/boot/loader.rc
  # echo 'boot' >> /mnt/boot/loader.rc
  # mkdir -p /mnt/efi/boot
  # cp /boot/loader.efi /mnt/efi/boot/bootx64.efi
  # umount /mnt
  # mdconfig -d -u $MDEV
then write ~/testbootfs to USB thumbdrive and attempt to boot from it in the UEFI mode.

Expected behavior: normal boot until init.

Actual behavior: when loader is reading kernel from disk, UEFI firmware crashes due to the heap corruption.

Problem can be reproduced under 12-CURRENT and 11.0-RELEASE-p2. Problem can't be reproduced under 10.3-RELEASE-p11. Bisection shows that problem is related to changes in lib/libstand/dosfs.c made in r298230.

This problem seems to be caused by long disk read (past the end of allocated buffer) in lib/libstand/dosfs.c:ioread() or device strategy functions called in that context.

Attached patch for lib/libstand/dosfs.c fixes this heap corruption (and also simplifies fetching of the next cluster from FAT cache):
Comment 1 Mark Linimon freebsd_committer freebsd_triage 2016-11-13 08:23:15 UTC
Assign to committer of 298230.
Comment 2 Toomas Soome freebsd_committer 2016-11-26 00:23:24 UTC
https://reviews.freebsd.org/D8644
Comment 3 Mikhail.Kupchik 2016-11-26 08:59:59 UTC
https://reviews.freebsd.org/D8644 looks good to me.
Comment 4 Mikhail.Kupchik 2016-11-26 18:33:33 UTC
I have tested D8644 in my environment and confirm that it works as expected (fixes this bug).
Comment 5 commit-hook freebsd_committer 2016-12-30 19:06:56 UTC
A commit references this bug:

Author: tsoome
Date: Fri Dec 30 19:06:32 UTC 2016
New revision: 310850
URL: https://svnweb.freebsd.org/changeset/base/310850

Log:
  dosfs support in libstand is broken since r298230

  Apparently the libstand dosfs optimization is a bit too optimistic
  and did introduce possible memory corruption.

  This patch is backing out the bad part and since this results in
  dosfs reading full blocks now, we can also remove extra offset argument
  from dv_strategy callback.

  The analysis of the issue and the backout patch is provided by Mikhail Kupchik.

  PR:		214423
  Submitted by:	Mikhail Kupchik
  Reported by:	Mikhail Kupchik
  Reviewed by:	bapt, allanjude
  Approved by:	allanjude (mentor)
  MFC after:	1 month
  Differential Revision:	https://reviews.freebsd.org/D8644

Changes:
  head/lib/libstand/cd9660.c
  head/lib/libstand/dosfs.c
  head/lib/libstand/ext2fs.c
  head/lib/libstand/nandfs.c
  head/lib/libstand/read.c
  head/lib/libstand/stand.h
  head/lib/libstand/ufs.c
  head/lib/libstand/write.c
  head/sys/boot/common/bcache.c
  head/sys/boot/common/bootstrap.h
  head/sys/boot/common/disk.c
  head/sys/boot/common/md.c
  head/sys/boot/efi/libefi/efipart.c
  head/sys/boot/i386/libfirewire/firewire.c
  head/sys/boot/i386/libi386/bioscd.c
  head/sys/boot/i386/libi386/biosdisk.c
  head/sys/boot/i386/libi386/pxe.c
  head/sys/boot/mips/beri/loader/beri_disk_cfi.c
  head/sys/boot/mips/beri/loader/beri_disk_sdcard.c
  head/sys/boot/ofw/libofw/ofw_disk.c
  head/sys/boot/pc98/libpc98/bioscd.c
  head/sys/boot/pc98/libpc98/biosdisk.c
  head/sys/boot/powerpc/kboot/hostdisk.c
  head/sys/boot/powerpc/ps3/ps3cdrom.c
  head/sys/boot/powerpc/ps3/ps3disk.c
  head/sys/boot/uboot/lib/disk.c
  head/sys/boot/usb/storage/umass_loader.c
  head/sys/boot/userboot/userboot/host.c
  head/sys/boot/userboot/userboot/userboot_disk.c
  head/sys/boot/zfs/zfs.c
Comment 6 Mikhail.Kupchik 2016-12-30 19:15:44 UTC
Fixed by commit 310850:
https://svnweb.freebsd.org/base?view=revision&revision=310850
Comment 7 commit-hook freebsd_committer 2017-02-06 22:04:05 UTC
A commit references this bug:

Author: tsoome
Date: Mon Feb  6 22:03:09 UTC 2017
New revision: 313355
URL: https://svnweb.freebsd.org/changeset/base/313355

Log:
  MFC r309369,310850,310853:

  libstand: dosfs cstyle cleanup for return keyword.
  dosfs support in libstand is broken since r298230

  PR:		214423
  Submitted by:	Mikhail Kupchik
  Reported by:	Mikhail Kupchik
  Approved by:	imp (mentor)

Changes:
_U  stable/11/
  stable/11/lib/libstand/cd9660.c
  stable/11/lib/libstand/dosfs.c
  stable/11/lib/libstand/ext2fs.c
  stable/11/lib/libstand/nandfs.c
  stable/11/lib/libstand/read.c
  stable/11/lib/libstand/stand.h
  stable/11/lib/libstand/ufs.c
  stable/11/lib/libstand/write.c
  stable/11/sys/boot/common/bcache.c
  stable/11/sys/boot/common/bootstrap.h
  stable/11/sys/boot/common/disk.c
  stable/11/sys/boot/common/md.c
  stable/11/sys/boot/efi/libefi/efipart.c
  stable/11/sys/boot/i386/libfirewire/firewire.c
  stable/11/sys/boot/i386/libi386/bioscd.c
  stable/11/sys/boot/i386/libi386/biosdisk.c
  stable/11/sys/boot/i386/libi386/pxe.c
  stable/11/sys/boot/mips/beri/loader/beri_disk_cfi.c
  stable/11/sys/boot/mips/beri/loader/beri_disk_sdcard.c
  stable/11/sys/boot/ofw/libofw/ofw_disk.c
  stable/11/sys/boot/pc98/libpc98/bioscd.c
  stable/11/sys/boot/pc98/libpc98/biosdisk.c
  stable/11/sys/boot/powerpc/kboot/hostdisk.c
  stable/11/sys/boot/powerpc/ps3/ps3cdrom.c
  stable/11/sys/boot/powerpc/ps3/ps3disk.c
  stable/11/sys/boot/uboot/lib/disk.c
  stable/11/sys/boot/usb/storage/umass_loader.c
  stable/11/sys/boot/userboot/userboot/host.c
  stable/11/sys/boot/userboot/userboot/userboot_disk.c
  stable/11/sys/boot/zfs/zfs.c