Bug 197881 - [PATCH] boot1.efi UEFI system table corruption
Summary: [PATCH] boot1.efi UEFI system table corruption
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 10.1-RELEASE
Hardware: Any Any
: --- Affects Some People
Assignee: freebsd-bugs (Nobody)
URL:
Keywords: uefi
Depends on:
Blocks:
 
Reported: 2015-02-21 16:51 UTC by Chris Ruffin
Modified: 2015-02-24 22:12 UTC (History)
3 users (show)

See Also:


Attachments
patch which resolves disagreement on block size (380 bytes, patch)
2015-02-23 15:09 UTC, Chris Ruffin
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Ruffin 2015-02-21 16:51:33 UTC
boot1.efi from 10.1-RELEASE can cause data corruption of its own UEFI system table pointer in memory.

The EfiBlockIoProtocol instance associated with a CDROM parition typically has a block size of 2048. boot1.efi assumes a block size of 512 in the call to dskread() to read the superblock, yet the dskread() function in boot1.efi uses the block size from the BlockIoProtocol instance.  This causes the call to BlkIoProtocol->ReadBlocks() in dskread() to expose a larger-than-actual buffer size to UEFI FW, which will cause the ReadBlocks() call to overrun the dmadat buffer. 

So if a partition is present in the system with a block size of 2048 (such as cdrom media partitions), boot1.efi can overrun its dmadat structure and cause corruption of other global data including a global pointe r to the UEFI system table. 

Failure sequence:
	1) dmadat.sbbuf  is allocated 8192 bytes at build time (SBLOCKSIZE)
	2) Boot1.efi retrieves handles with all instances  of BlockIoProtocol.
	3) If cdrom partition is present in the system, UEFI FW will return a BlockIoProtocol of the CDROM partition which has a BlockIoProtocol->Media->BlockSize = 2048.
	4) domount() is called for this BlockIoProtocol instance.
	5) fsread() is called to read the superblock
	6) Boot1.efi dskread() is called with nblk=16 (fixed SBLOCKSIZE (8192) / fixed DEV_BSIZE (512)).  This is assuming a block size of 512.
	7) dskread() in boot1.efi  calls 
	
	        status = bootdev->ReadBlocks(bootdev, bootdev->Media->MediaId, lba,
	            nblk * bootdev->Media->BlockSize, buf);
	
	Here nblk=16 but bootdev->Media->BlockSize is 2048 which results in passing a Buffer Size argument of 16 * 2048 = 32768 which overruns the dmadat->sbbuf  fixed allocation.
	8) UEFI ReaadBlocks() overruns dmadat and overwrites the global pointer to the UEFI system table (sets it to zero)
	9) A subsequent call to systab->BootServices->HandleProtocol() causes path of exeution at physical address 0x98  which does not contain valid code.
	10) Processor eventually raises an invalid opcode exception.
Comment 1 Chris Ruffin 2015-02-23 15:09:29 UTC
Created attachment 153375 [details]
patch which resolves disagreement on block size

ufsread.c assumes a device block size of 512 in all calls to dskread() which is implemented in boot1.c.  This patch causes dskread() to follow that same assumption.
Comment 2 Ed Maste freebsd_committer freebsd_triage 2015-02-23 15:54:22 UTC
Looks like this was fixed in a slightly different way on HEAD, to support 4Kn drives, here: https://svnweb.freebsd.org/base?view=revision&revision=273865 (or git 37cb22f7)

That change also divides lba by bootdev->Media->BlockSize / DEV_BSIZE - i.e., by 4 for CDs and by 8 for 4Kn.
Comment 3 Chris Ruffin 2015-02-23 17:44:38 UTC
I reviewed this change and agree it is equivalent / more complete fix.
Comment 4 commit-hook freebsd_committer freebsd_triage 2015-02-24 22:11:50 UTC
A commit references this bug:

Author: emaste
Date: Tue Feb 24 22:11:08 UTC 2015
New revision: 279254
URL: https://svnweb.freebsd.org/changeset/base/279254

Log:
  MFC part of r273865: fix boot1.efi for block size != 512

  r273865 is part of the work for supporting 4Kn drives, but it turns out
  the underlying bug can actually cause corruption of the UEFI system
  table in any case where block size is not 512.

  Relevant portion of the original commit message:

    convert boot1.efi to corrrectly calculate the lba for what the
    media reports and convert the size based on what FreeBSD uses.
    existing code would use the 512 byte lba and convert the
    using 4K byte size.

  PR:		197881
  Reviewed by:	Chris Ruffin

Changes:
  stable/10/sys/boot/amd64/boot1.efi/boot1.c
Comment 5 Ed Maste freebsd_committer freebsd_triage 2015-02-24 22:12:28 UTC
Resolved with partial merge of r273865