Bug 197641 - [PATCH] UEFI loader creates invalid device path
Summary: [PATCH] UEFI loader creates invalid device path
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-14 15:20 UTC by Chris Ruffin
Modified: 2015-09-01 17:43 UTC (History)
3 users (show)

See Also:
emaste: mfc-stable10?


Attachments
original device path (198.04 KB, image/png)
2015-02-14 15:20 UTC, Chris Ruffin
no flags Details
modified device path & code performing the modification (184.56 KB, image/png)
2015-02-14 15:21 UTC, Chris Ruffin
no flags Details
patch which avoids modification of firmware-allocated structures (1.65 KB, patch)
2015-02-23 15:04 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-14 15:20:38 UTC
Created attachment 152971 [details]
original device path

The UEFI loader on the 10.1 release install disk (disc1) modifies an
existing EFI_DEVICE_PATH_PROTOCOL instance in an apparent attempt to
truncate the device path.  In doing so it creates an invalid device
path.

The original UEFI device path is represented textually as follows:
PciRoot(0x0)/Pci(0x18,0x0)/Sata(0x0,0x0,0x0)/CDROM(0x0,0x14,0x4)

The last node in the path has a length of 0x18.

The loader (for unknown reasons) truncates the device path to:
PciRoot(0x0)/Pci(0x18,0x0)/Sata(0x0,0x0,0x0)

It seems to attempt to transform the last node to an END_DEVICE_PATH
node by overwriting the last node of the device path to have a
EFI_DEVICE_PATH_PROTOCOL->Type and SubType as follows:

#define END_DEVICE_PATH_TYPE                 0x7f
#define END_ENTIRE_DEVICE_PATH_SUBTYPE       0xFF

However, it leaves the length of the node unmodified, so that it does
not have a length of 4 as required for an END_DEVICE_PATH structure,
per UEFI 2.4.0 $9.3.1, Table 40 "Device Path End Structure" 

A later call to the boot service LocateDevicePath() sees this device
path as invalid device path and throws an assert.

It ins't clear the purpose behind truncating the device path.  In
general I would not recommend modifying data structures allocated by
the firmware.  But, it isn't clear what the intent of the code is.

At a minimum the loader should not be creating the invalid device path.

The loader is loaded into memory at 75349000, and the device path
modification happens at address 7536bf59.
Comment 1 Chris Ruffin 2015-02-14 15:21:24 UTC
Created attachment 152972 [details]
modified device path & code performing the modification
Comment 2 Chris Ruffin 2015-02-14 15:22:13 UTC
Also I aded a couple of debugger screenshots that show the device path in memory before and after modification, with the capture of the source performing the modification.
Comment 3 Ed Maste freebsd_committer freebsd_triage 2015-02-14 16:31:46 UTC
Thank you for the bug report!

It looks like it's coming from here

https://svnweb.freebsd.org/base/head/sys/boot/efi/libefi/efipart.c?revision=264088&view=markup

and was introduced in

https://svnweb.freebsd.org/base?view=revision&revision=247380
Comment 4 Chris Ruffin 2015-02-14 17:22:08 UTC
Thanks!  It does look like it is an omission of setting the node length to 4 just after line 119:

https://svnweb.freebsd.org/base/head/sys/boot/efi/libefi/efipart.c?revision=264088&view=markup#l119

For reference, the incompatibility is surfaced by implementing a check for this condition here:

https://github.com/tianocore/edk2/blob/svn/branches/UDK2014.SP1/MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c#L88
Comment 5 Chris Ruffin 2015-02-23 15:04:27 UTC
Created attachment 153374 [details]
patch which avoids modification of firmware-allocated structures

The attached patch performs the equivalent action without modification of structures allocated by firmware.
Comment 6 commit-hook freebsd_committer freebsd_triage 2015-07-07 18:44:58 UTC
A commit references this bug:

Author: emaste
Date: Tue Jul  7 18:44:29 UTC 2015
New revision: 285246
URL: https://svnweb.freebsd.org/changeset/base/285246

Log:
  Avoid creating invalid UEFI device path

  The UEFI loader on the 10.1 release install disk (disc1) modifies an
  existing EFI_DEVICE_PATH_PROTOCOL instance in an apparent attempt to
  truncate the device path.  In doing so it creates an invalid device
  path.

  Perform the equivalent action without modification of structures
  allocated by firmware.

  PR:		197641
  MFC After:	1 week
  Submitted by:	Chris Ruffin <chris.ruffin@intel.com>

Changes:
  head/sys/boot/efi/libefi/efipart.c
Comment 7 commit-hook freebsd_committer freebsd_triage 2015-07-28 12:45:25 UTC
A commit references this bug:

Author: emaste
Date: Tue Jul 28 12:45:09 UTC 2015
New revision: 285951
URL: https://svnweb.freebsd.org/changeset/base/285951

Log:
  MFC r285246: Avoid creating invalid UEFI device path

  The UEFI loader on the 10.1 release install disk (disc1) modifies an
  existing EFI_DEVICE_PATH_PROTOCOL instance in an apparent attempt to
  truncate the device path.  In doing so it creates an invalid device
  path.

  Perform the equivalent action without modification of structures
  allocated by firmware.

  PR:		197641
  Submitted by:	Chris Ruffin <chris.ruffin at intel.com>

Changes:
_U  stable/10/
  stable/10/sys/boot/efi/libefi/efipart.c
Comment 8 commit-hook freebsd_committer freebsd_triage 2015-07-28 13:12:30 UTC
A commit references this bug:

Author: emaste
Date: Tue Jul 28 13:11:32 UTC 2015
New revision: 285956
URL: https://svnweb.freebsd.org/changeset/base/285956

Log:
  MFS r285951: Avoid creating invalid UEFI device path

  The UEFI loader on the 10.1 release install disk (disc1) modifies an
  existing EFI_DEVICE_PATH_PROTOCOL instance in an apparent attempt to
  truncate the device path.  In doing so it creates an invalid device
  path.

  Perform the equivalent action without modification of structures
  allocated by firmware.

  PR:		197641
  Submitted by:	Chris Ruffin <chris.ruffin at intel.com>
  Approved by:	re (gjb)

Changes:
_U  releng/10.2/
  releng/10.2/sys/boot/efi/libefi/efipart.c