From bdf0e029ae8c9a0e173d5988ee64ff910538af53 Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Sat, 14 Mar 2020 09:49:49 +0000 Subject: [efi] Disambiguate errors returned by ConnectController Signed-off-by: Michael Brown --- src/interface/efi/efi_driver.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) (limited to 'src/interface/efi') diff --git a/src/interface/efi/efi_driver.c b/src/interface/efi/efi_driver.c index 7be2e585d..f6038e56b 100644 --- a/src/interface/efi/efi_driver.c +++ b/src/interface/efi/efi_driver.c @@ -39,6 +39,20 @@ FILE_LICENCE ( GPL2_OR_LATER ); * */ +/* Disambiguate the various error causes */ +#define EINFO_EEFI_CONNECT \ + __einfo_uniqify ( EINFO_EPLATFORM, 0x01, \ + "Could not connect controllers" ) +#define EINFO_EEFI_CONNECT_PROHIBITED \ + __einfo_platformify ( EINFO_EEFI_CONNECT, \ + EFI_SECURITY_VIOLATION, \ + "Connecting controllers prohibited by " \ + "security policy" ) +#define EEFI_CONNECT_PROHIBITED \ + __einfo_error ( EINFO_EEFI_CONNECT_PROHIBITED ) +#define EEFI_CONNECT( efirc ) EPLATFORM ( EINFO_EEFI_CONNECT, efirc, \ + EEFI_CONNECT_PROHIBITED ) + static EFI_DRIVER_BINDING_PROTOCOL efi_driver_binding; /** List of controlled EFI devices */ @@ -457,7 +471,7 @@ static int efi_driver_connect ( EFI_HANDLE device ) { efi_handle_name ( device ) ); if ( ( efirc = bs->ConnectController ( device, drivers, NULL, FALSE ) ) != 0 ) { - rc = -EEFI ( efirc ); + rc = -EEFI_CONNECT ( efirc ); DBGC ( device, "EFIDRV %s could not connect new drivers: " "%s\n", efi_handle_name ( device ), strerror ( rc ) ); return rc; -- cgit v1.2.3-55-g7522 From ac28bbb7ea3a1c88aa47c086e92fab78a75c351d Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Fri, 5 Jun 2020 09:40:36 +0100 Subject: [efi] Work around UEFI specification bug in LoadImage for SAN boot As described in the previous commit, work around a UEFI specification bug that necessitates calling UnloadImage if the return value from LoadImage is EFI_SECURITY_VIOLATION. Signed-off-by: Michael Brown --- src/interface/efi/efi_block.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'src/interface/efi') diff --git a/src/interface/efi/efi_block.c b/src/interface/efi/efi_block.c index 91f830a11..64d1e1980 100644 --- a/src/interface/efi/efi_block.c +++ b/src/interface/efi/efi_block.c @@ -594,11 +594,14 @@ static int efi_block_boot_image ( struct san_device *sandev, EFI_HANDLE handle, sandev->drive, efi_devpath_text ( boot_path ) ); /* Try loading boot image from this device */ + *image = NULL; if ( ( efirc = bs->LoadImage ( FALSE, efi_image_handle, boot_path, NULL, 0, image ) ) != 0 ) { rc = -EEFI ( efirc ); DBGC ( sandev, "EFIBLK %#02x could not load image: %s\n", sandev->drive, strerror ( rc ) ); + if ( efirc == EFI_SECURITY_VIOLATION ) + bs->UnloadImage ( *image ); goto err_load_image; } -- cgit v1.2.3-55-g7522 From 9ee70fb95bc266885ff88be228b044a2bb226eeb Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Wed, 10 Jun 2020 22:52:11 +0100 Subject: [efi] Attempt to connect our driver directly if ConnectController fails Some platforms (observed with an AMI BIOS on an Apollo Lake system) will spuriously fail the call to ConnectController() when the UEFI network stack is disabled. This appears to be a BIOS bug that also affects attempts to connect any non-iPXE driver to the NIC controller handle via the UEFI shell "connect" utility. Work around this BIOS bug by falling back to calling our efi_driver_start() directly if the call to ConnectController() fails. This bypasses any BIOS policy in terms of deciding which driver to connect but still cooperates with the UEFI driver model in terms of handle ownership, since the use of EFI_OPEN_PROTOCOL_BY_DRIVER ensures that the BIOS is aware of our ownership claim. Signed-off-by: Michael Brown --- src/interface/efi/efi_driver.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) (limited to 'src/interface/efi') diff --git a/src/interface/efi/efi_driver.c b/src/interface/efi/efi_driver.c index f6038e56b..760ee41a8 100644 --- a/src/interface/efi/efi_driver.c +++ b/src/interface/efi/efi_driver.c @@ -474,7 +474,16 @@ static int efi_driver_connect ( EFI_HANDLE device ) { rc = -EEFI_CONNECT ( efirc ); DBGC ( device, "EFIDRV %s could not connect new drivers: " "%s\n", efi_handle_name ( device ), strerror ( rc ) ); - return rc; + DBGC ( device, "EFIDRV %s connecting driver directly\n", + efi_handle_name ( device ) ); + if ( ( efirc = efi_driver_start ( &efi_driver_binding, device, + NULL ) ) != 0 ) { + rc = -EEFI_CONNECT ( efirc ); + DBGC ( device, "EFIDRV %s could not connect driver " + "directly: %s\n", efi_handle_name ( device ), + strerror ( rc ) ); + return rc; + } } DBGC2 ( device, "EFIDRV %s after connecting:\n", efi_handle_name ( device ) ); -- cgit v1.2.3-55-g7522 From a61b27b97f572a83ede765a0e779694865950cf2 Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Tue, 23 Jun 2020 23:08:49 +0100 Subject: [efi] Enable stack protection where possible Enable -fstack-protector for EFI builds, where binary size is less critical than for BIOS builds. The stack cookie must be constructed immediately on entry, which prohibits the use of any viable entropy source. Construct a cookie by XORing together various mildly random quantities to produce a value that will at least not be identical on each run. On detecting a stack corruption, attempt to call Exit() with an appropriate error. If that fails, then lock up the machine since there is no other safe action that can be taken. The old conditional check for support of -fno-stack-protector is omitted since this flag dates back to GCC 4.1. Signed-off-by: Michael Brown --- src/Makefile.efi | 8 ++++++ src/Makefile.housekeeping | 18 +++++------- src/include/ipxe/efi/efi.h | 32 +++++++++++++++++++++ src/interface/efi/efi_init.c | 62 ++++++++++++++++++++++++++++++++++++++++ src/interface/efi/efidrvprefix.c | 3 ++ src/interface/efi/efiprefix.c | 3 ++ 6 files changed, 115 insertions(+), 11 deletions(-) (limited to 'src/interface/efi') diff --git a/src/Makefile.efi b/src/Makefile.efi index 151b33186..10f3fe74b 100644 --- a/src/Makefile.efi +++ b/src/Makefile.efi @@ -1,5 +1,13 @@ # -*- makefile -*- : Force emacs to use Makefile mode +# Enable stack protection if available +# +SPG_TEST = $(CC) -fstack-protector-strong -mstack-protector-guard=global \ + -x c -c /dev/null -o /dev/null >/dev/null 2>&1 +SPG_FLAGS := $(shell $(SPG_TEST) && $(ECHO) '-fstack-protector-strong ' \ + '-mstack-protector-guard=global') +CFLAGS += $(SPG_FLAGS) + # The EFI linker script # LDSCRIPT = scripts/efi.lds diff --git a/src/Makefile.housekeeping b/src/Makefile.housekeeping index 1dd147949..66d6dd449 100644 --- a/src/Makefile.housekeeping +++ b/src/Makefile.housekeeping @@ -146,17 +146,6 @@ define NEWLINE endef -# Some widespread patched versions of gcc include -fstack-protector by -# default, even when -ffreestanding is specified. We therefore need -# to disable -fstack-protector if the compiler supports it. -# -ifeq ($(CCTYPE),gcc) -SP_TEST = $(CC) -fno-stack-protector -x c -c /dev/null \ - -o /dev/null >/dev/null 2>&1 -SP_FLAGS := $(shell $(SP_TEST) && $(ECHO) '-fno-stack-protector') -WORKAROUND_CFLAGS += $(SP_FLAGS) -endif - # gcc 4.4 generates .eh_frame sections by default, which distort the # output of "size". Inhibit this. # @@ -415,6 +404,13 @@ ifdef BIN incdirs : @$(ECHO) $(INCDIRS) +# Inhibit -fstack-protector (which is implicitly enabled in some +# patched gcc versions) unless explicitly mentioned in CFLAGS. +# +ifeq ($(findstring -fstack-protector,$(CFLAGS)),) +CFLAGS += -fno-stack-protector +endif + # Common flags # CFLAGS += $(foreach INC,$(INCDIRS),-I$(INC)) diff --git a/src/include/ipxe/efi/efi.h b/src/include/ipxe/efi/efi.h index 669e5364a..b8777ef47 100644 --- a/src/include/ipxe/efi/efi.h +++ b/src/include/ipxe/efi/efi.h @@ -65,6 +65,8 @@ typedef struct {} *EFI_HANDLE; #include #include +#include +#include /** An EFI protocol used by iPXE */ struct efi_protocol { @@ -272,6 +274,36 @@ extern void dbg_efi_protocols ( EFI_HANDLE handle ); #define DBGCP_EFI_PROTOCOLS( ... ) \ DBGC_EFI_PROTOCOLS_IF ( PROFILE, ##__VA_ARGS__ ) +extern unsigned long __stack_chk_guard; +extern unsigned long efi_stack_cookie ( EFI_HANDLE handle ); +extern void __stack_chk_fail ( void ); + +/** + * Initialise stack cookie + * + * @v handle Image handle + */ +static inline __attribute__ (( always_inline )) void +efi_init_stack_guard ( EFI_HANDLE handle ) { + + /* The calling function must not itself use stack protection, + * since the change in the stack guard value would trigger a + * false positive. + * + * There is unfortunately no way to annotate a function to + * exclude the use of stack protection. We must therefore + * rely on correctly anticipating the compiler's decision on + * the use of stack protection. + * + * The calculation of the stack cookie value deliberately + * takes the address of a stack variable (to provide an + * additional source of entropy). This operation would + * trigger the application of stack protection to the calling + * function, and so must be externalised. + */ + __stack_chk_guard = efi_stack_cookie ( handle ); +} + extern EFI_STATUS efi_init ( EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *systab ); diff --git a/src/interface/efi/efi_init.c b/src/interface/efi/efi_init.c index ed9707f2c..df46bb17b 100644 --- a/src/interface/efi/efi_init.c +++ b/src/interface/efi/efi_init.c @@ -47,6 +47,16 @@ int efi_shutdown_in_progress; /** Event used to signal shutdown */ static EFI_EVENT efi_shutdown_event; +/** Stack cookie */ +unsigned long __stack_chk_guard; + +/** Exit function + * + * Cached to minimise external dependencies when a stack check + * failure is triggered. + */ +static EFI_EXIT efi_exit; + /* Forward declarations */ static EFI_STATUS EFIAPI efi_unload ( EFI_HANDLE image_handle ); @@ -87,6 +97,29 @@ static void * efi_find_table ( EFI_GUID *guid ) { return NULL; } +/** + * Construct a stack cookie value + * + * @v handle Image handle + * @ret cookie Stack cookie + */ +__attribute__ (( noinline )) unsigned long +efi_stack_cookie ( EFI_HANDLE handle ) { + + /* There is no viable source of entropy available at this + * point. Construct a value that is at least likely to vary + * between platforms and invocations. + * + * Ensure that the value contains a NUL byte, to act as a + * runaway string terminator. Construct the NUL using a shift + * rather than a mask, to avoid losing valuable entropy in the + * low-order bits. + */ + return ( ( ( ( unsigned long ) handle ) ^ + ( ( unsigned long ) &handle ) ^ + profile_timestamp() ^ build_id ) << 8 ); +} + /** * Initialise EFI environment * @@ -130,6 +163,9 @@ EFI_STATUS efi_init ( EFI_HANDLE image_handle, DBGC ( systab, "EFI handle %p systab %p\n", image_handle, systab ); bs = systab->BootServices; + /* Store abort function pointer */ + efi_exit = bs->Exit; + /* Look up used protocols */ for_each_table_entry ( prot, EFI_PROTOCOLS ) { if ( ( efirc = bs->LocateProtocol ( &prot->guid, NULL, @@ -240,3 +276,29 @@ static EFI_STATUS EFIAPI efi_unload ( EFI_HANDLE image_handle __unused ) { return 0; } + +/** + * Abort on stack check failure + * + */ +__attribute__ (( noreturn )) void __stack_chk_fail ( void ) { + EFI_STATUS efirc; + int rc; + + /* Report failure (when debugging) */ + DBGC ( efi_systab, "EFI stack check failed (cookie %#lx); aborting\n", + __stack_chk_guard ); + + /* Attempt to exit cleanly with an error status */ + if ( efi_exit ) { + efirc = efi_exit ( efi_image_handle, EFI_COMPROMISED_DATA, + 0, NULL ); + rc = -EEFI ( efirc ); + DBGC ( efi_systab, "EFI stack check exit failed: %s\n", + strerror ( rc ) ); + } + + /* If the exit fails for any reason, lock the system */ + while ( 1 ) {} + +} diff --git a/src/interface/efi/efidrvprefix.c b/src/interface/efi/efidrvprefix.c index 4fbb19ff7..f9bfa4c1d 100644 --- a/src/interface/efi/efidrvprefix.c +++ b/src/interface/efi/efidrvprefix.c @@ -36,6 +36,9 @@ EFI_STATUS EFIAPI _efidrv_start ( EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *systab ) { EFI_STATUS efirc; + /* Initialise stack cookie */ + efi_init_stack_guard ( image_handle ); + /* Initialise EFI environment */ if ( ( efirc = efi_init ( image_handle, systab ) ) != 0 ) return efirc; diff --git a/src/interface/efi/efiprefix.c b/src/interface/efi/efiprefix.c index de3572c75..2c5a5b31d 100644 --- a/src/interface/efi/efiprefix.c +++ b/src/interface/efi/efiprefix.c @@ -41,6 +41,9 @@ EFI_STATUS EFIAPI _efi_start ( EFI_HANDLE image_handle, EFI_STATUS efirc; int rc; + /* Initialise stack cookie */ + efi_init_stack_guard ( image_handle ); + /* Initialise EFI environment */ if ( ( efirc = efi_init ( image_handle, systab ) ) != 0 ) goto err_init; -- cgit v1.2.3-55-g7522 From a87537d44c4fc46a41620af4591616057a89bf95 Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Sun, 28 Jun 2020 19:24:30 +0100 Subject: [efi] Detect and disable seriously broken EFI_RNG_PROTOCOL implementations The EFI_RNG_PROTOCOL on the Microsoft Surface Go does not generate random numbers. Successive calls to GetRNG() without any intervening I/O operations (such as writing to the console) will produce identical results. Successive reboots will produce identical results. It is unclear what the Microsoft Surface Go is attempting to use as an entropy source, but it is demonstrably producing zero bits of entropy. The failure is already detected by the ANS-mandated Repetition Count Test performed as part of our GetEntropy implementation. This currently results in the entropy source being marked as broken, with the result that iPXE refuses to perform any operations that require a working entropy source. We cannot use the existing EFI driver blacklisting mechanism to unload the broken driver, since the RngDxe driver is integrated into the DxeCore image. Work around the broken driver by checking for consecutive identical results returned by EFI_RNG_PROTOCOL and falling back to the original timer-based entropy source. Signed-off-by: Michael Brown --- src/interface/efi/efi_entropy.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) (limited to 'src/interface/efi') diff --git a/src/interface/efi/efi_entropy.c b/src/interface/efi/efi_entropy.c index 2a2fc9054..dca0b6f1d 100644 --- a/src/interface/efi/efi_entropy.c +++ b/src/interface/efi/efi_entropy.c @@ -179,6 +179,7 @@ static int efi_get_noise_ticks ( noise_sample_t *noise ) { * @ret rc Return status code */ static int efi_get_noise_rng ( noise_sample_t *noise ) { + static uint8_t prev[EFI_ENTROPY_RNG_LEN]; uint8_t buf[EFI_ENTROPY_RNG_LEN]; EFI_STATUS efirc; int rc; @@ -196,6 +197,17 @@ static int efi_get_noise_rng ( noise_sample_t *noise ) { return rc; } + /* Fail (and permanently disable the EFI RNG) if we get + * consecutive identical results. + */ + if ( memcmp ( buf, prev, sizeof ( buf ) ) == 0 ) { + DBGC ( &tick, "ENTROPY detected broken EFI RNG:\n" ); + DBGC_HDA ( &tick, 0, buf, sizeof ( buf ) ); + efirng = NULL; + return -EIO; + } + memcpy ( prev, buf, sizeof ( prev ) ); + /* Reduce random bytes to a single noise sample. This seems * like overkill, but we have no way of knowing how much * entropy is actually present in the bytes returned by the -- cgit v1.2.3-55-g7522 From 2ae5d4338661b65c63eb5cb1a96e5b803fe7d620 Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Tue, 30 Jun 2020 16:32:59 +0100 Subject: [efi] Raise TPL during driver entry point As per commit c89a446 ("[efi] Run at TPL_CALLBACK to protect against UEFI timers") we expect to run at TPL_CALLBACK almost all of the time. Various code paths rely on this assumption. Code paths that need to temporarily lower the TPL (e.g. for entropy gathering) will restore it to TPL_CALLBACK. The entropy gathering code will be run during DRBG initialisation, which happens during the call to startup(). In the case of iPXE compiled as an EFI application this code will run within the scope of efi_snp_claim() and so will execute at TPL_CALLBACK as expected. In the case of iPXE compiled as an EFI driver the code will incorrectly run at TPL_APPLICATION since there is nothing within the EFI driver entry point that raises (and restores) the TPL. The net effect is that a build that includes the entropy-gathering code (e.g. a build with HTTPS enabled) will return from the driver entry point at TPL_CALLBACK, which causes a system lockup. Fix by raising and restoring the TPL within the EFI driver entry point. Debugged-by: Ignat Korchagin Signed-off-by: Michael Brown --- src/interface/efi/efidrvprefix.c | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'src/interface/efi') diff --git a/src/interface/efi/efidrvprefix.c b/src/interface/efi/efidrvprefix.c index f9bfa4c1d..ac7d94374 100644 --- a/src/interface/efi/efidrvprefix.c +++ b/src/interface/efi/efidrvprefix.c @@ -34,6 +34,8 @@ FILE_LICENCE ( GPL2_OR_LATER ); */ EFI_STATUS EFIAPI _efidrv_start ( EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *systab ) { + EFI_BOOT_SERVICES *bs; + EFI_TPL saved_tpl; EFI_STATUS efirc; /* Initialise stack cookie */ @@ -43,10 +45,17 @@ EFI_STATUS EFIAPI _efidrv_start ( EFI_HANDLE image_handle, if ( ( efirc = efi_init ( image_handle, systab ) ) != 0 ) return efirc; + /* Raise TPL */ + bs = efi_systab->BootServices; + saved_tpl = bs->RaiseTPL ( TPL_CALLBACK ); + /* Initialise iPXE environment */ initialise(); startup(); + /* Restore TPL */ + bs->RestoreTPL ( saved_tpl ); + return 0; } -- cgit v1.2.3-55-g7522 From fcdd9c0982e6c5a435fefdcc9444242fd4fbdd8b Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Thu, 9 Jul 2020 13:56:50 +0100 Subject: [efi] Distribute available entropy within stack cookie Several of the values used to compute a stack cookie (in the absence of a viable entropy source) will tend to have either all-zeroes or all-ones in the higher order bits. Rotate the values in order to distribute the (minimal) available entropy more evenly. Suggested-by: Pete Beck Signed-off-by: Michael Brown --- src/interface/efi/efi_init.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) (limited to 'src/interface/efi') diff --git a/src/interface/efi/efi_init.c b/src/interface/efi/efi_init.c index df46bb17b..284c39b35 100644 --- a/src/interface/efi/efi_init.c +++ b/src/interface/efi/efi_init.c @@ -22,6 +22,7 @@ FILE_LICENCE ( GPL2_OR_LATER ); #include #include #include +#include #include #include #include @@ -105,19 +106,29 @@ static void * efi_find_table ( EFI_GUID *guid ) { */ __attribute__ (( noinline )) unsigned long efi_stack_cookie ( EFI_HANDLE handle ) { + unsigned long cookie = 0; + unsigned int rotation = ( 8 * sizeof ( cookie ) / 4 ); /* There is no viable source of entropy available at this * point. Construct a value that is at least likely to vary * between platforms and invocations. - * - * Ensure that the value contains a NUL byte, to act as a + */ + cookie ^= ( ( unsigned long ) handle ); + cookie = roll ( cookie, rotation ); + cookie ^= ( ( unsigned long ) &handle ); + cookie = roll ( cookie, rotation ); + cookie ^= profile_timestamp(); + cookie = roll ( cookie, rotation ); + cookie ^= build_id; + + /* Ensure that the value contains a NUL byte, to act as a * runaway string terminator. Construct the NUL using a shift * rather than a mask, to avoid losing valuable entropy in the - * low-order bits. + * lower-order bits. */ - return ( ( ( ( unsigned long ) handle ) ^ - ( ( unsigned long ) &handle ) ^ - profile_timestamp() ^ build_id ) << 8 ); + cookie <<= 8; + + return cookie; } /** -- cgit v1.2.3-55-g7522 From b6eecb182e07a82bae2a5f8c28dadb1bea409090 Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Thu, 9 Jul 2020 14:20:53 +0100 Subject: [efi] Ensure NUL byte is at lowest address within stack cookie The NUL byte included within the stack cookie to act as a string terminator should be placed at the lowest byte address within the stack cookie, in order to avoid potentially including the stack cookie value within an accidentally unterminated string. Suggested-by: Pete Beck Signed-off-by: Michael Brown --- src/interface/efi/efi_init.c | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 'src/interface/efi') diff --git a/src/interface/efi/efi_init.c b/src/interface/efi/efi_init.c index 284c39b35..e1041a5ec 100644 --- a/src/interface/efi/efi_init.c +++ b/src/interface/efi/efi_init.c @@ -21,6 +21,7 @@ FILE_LICENCE ( GPL2_OR_LATER ); #include #include +#include #include #include #include @@ -128,6 +129,13 @@ efi_stack_cookie ( EFI_HANDLE handle ) { */ cookie <<= 8; + /* Ensure that the NUL byte is placed at the bottom of the + * stack cookie, to avoid potential disclosure via an + * unterminated string. + */ + if ( __BYTE_ORDER == __BIG_ENDIAN ) + cookie >>= 8; + return cookie; } -- cgit v1.2.3-55-g7522 From 1832f8a9b02d00977ca1e079bd9e0369763df14c Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Tue, 14 Jul 2020 18:24:02 +0100 Subject: [efi] Claim SNP devices early in efi_download_start() Claiming the SNP devices has the side effect of raising the TPL to iPXE's normal operating level of TPL_CALLBACK (see the commit message for c89a446 ("[efi] Run at TPL_CALLBACK to protect against UEFI timers") for details). This must happen before executing any code that relies upon the TPL having been raised to TPL_CALLBACK. The call to efi_snp_claim() in efi_download_start() currently happens only after the call to xfer_open(). Calling xfer_open() will typically result in a retry timer being started, which will result in a call to currticks() in order to initialise the timer. The call to currticks() will drop to TPL_APPLICATION and restore to TPL_CALLBACK in order to allow a timer tick to occur. Since this call happened before the call to efi_snp_claim(), the restored TPL is incorrect. This in turn results in efi_snp_claim() recording the incorrect original TPL, causing efi_snp_release() to eventually restore the incorrect TPL, causing the system to lock up when ExitBootServices() is called at TPL_CALLBACK. Fix by moving the call to efi_snp_claim() to the start of efi_download_start(). Debugged-by: Jarrod Johnson Debugged-by: He He4 Huang Debugged-by: James Wang Signed-off-by: Michael Brown --- src/interface/efi/efi_download.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'src/interface/efi') diff --git a/src/interface/efi/efi_download.c b/src/interface/efi/efi_download.c index 1218852e2..8d12bd57c 100644 --- a/src/interface/efi/efi_download.c +++ b/src/interface/efi/efi_download.c @@ -138,8 +138,11 @@ efi_download_start ( IPXE_DOWNLOAD_PROTOCOL *This __unused, struct efi_download_file *file; int rc; + efi_snp_claim(); + file = malloc ( sizeof ( struct efi_download_file ) ); if ( file == NULL ) { + efi_snp_release(); return EFI_OUT_OF_RESOURCES; } @@ -147,10 +150,10 @@ efi_download_start ( IPXE_DOWNLOAD_PROTOCOL *This __unused, rc = xfer_open ( &file->xfer, LOCATION_URI_STRING, Url ); if ( rc ) { free ( file ); + efi_snp_release(); return EFIRC ( rc ); } - efi_snp_claim(); file->pos = 0; file->data_callback = DataCallback; file->finish_callback = FinishCallback; -- cgit v1.2.3-55-g7522 From b76052335788d0ad2c4b0bded116c3b02dd4bbc2 Mon Sep 17 00:00:00 2001 From: Ignat Korchagin Date: Tue, 21 Jul 2020 22:27:08 +0100 Subject: [efi] Check the status code from AllocatePool() According to the latest UEFI specification (Version 2.8 Errata B) p. 7.2: "Buffer: A pointer to a pointer to the allocated buffer if the call succeeds; undefined otherwise." So implementations are obliged neither to return NULL, if the allocation fails, nor to preserve the contents of the pointer. Make the logic more reliable by checking the status code from AllocatePool() instead of checking the returned pointer for NULL Signed-off-by: Ignat Korchagin Modified-by: Michael Brown Signed-off-by: Michael Brown --- src/interface/efi/efi_snp_hii.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) (limited to 'src/interface/efi') diff --git a/src/interface/efi/efi_snp_hii.c b/src/interface/efi/efi_snp_hii.c index 651bef040..1e681a429 100644 --- a/src/interface/efi/efi_snp_hii.c +++ b/src/interface/efi/efi_snp_hii.c @@ -247,16 +247,17 @@ static int efi_snp_hii_append ( struct efi_snp_device *snpdev __unused, const char *key, const char *value, wchar_t **results ) { EFI_BOOT_SERVICES *bs = efi_systab->BootServices; + EFI_STATUS efirc; size_t len; void *new; /* Allocate new string */ len = ( ( *results ? ( wcslen ( *results ) + 1 /* "&" */ ) : 0 ) + strlen ( key ) + 1 /* "=" */ + strlen ( value ) + 1 /* NUL */ ); - bs->AllocatePool ( EfiBootServicesData, ( len * sizeof ( wchar_t ) ), - &new ); - if ( ! new ) - return -ENOMEM; + if ( ( efirc = bs->AllocatePool ( EfiBootServicesData, + ( len * sizeof ( wchar_t ) ), + &new ) ) != 0 ) + return -EEFI ( efirc ); /* Populate string */ efi_snprintf ( new, len, "%ls%s%s=%s", ( *results ? *results : L"" ), -- cgit v1.2.3-55-g7522 From fd47fa8fe1a3542621d5c97af349b3ae38146585 Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Mon, 3 Aug 2020 14:42:18 +0100 Subject: [efi] Match EDK2 numbering for USB ports The various USB specifications all use one-based numbering for ports. This scheme is applied consistently across the various relevant specifications, covering both port numbers that appear on the wire (i.e. downstream hub port numbers) and port numbers that exist only logically (i.e. root hub port numbers). The UEFI specification is ambiguous about the port numbers as used for the ParentPortNumber field within a USB_DEVICE_PATH structure. As of UEFI specification version 2.8 errata B: - section 10.3.4.5 just states "USB Parent Port Number" with no indication of being zero-based or one-based - section 17.1.1 notes that for the EFI_USB2_HC_PROTOCOL, references to PortNumber parameters are zero-based for root hub ports - section 17.1.1 also mentions a TranslatorPortNumber used by EFI_USB2_HC_PROTOCOL, with no indication of being zero-based or one-based - there are no other mentions of USB port numbering schemes. Experimentation and inspection of the EDK2 codebase reveals that at least the EDK2 reference implementation will use zero-based numbering for both root and non-root hub ports when populating a USB_DEVICE_PATH structure (though will inconsistently use one-based numbering for the TranslatorPortNumber parameter). Use zero-based numbering for both root and non-root hub ports when constructing a USB_DEVICE_PATH in order to match the behaviour of the EDK2 implementation. Signed-off-by: Michael Brown --- src/interface/efi/efi_usb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/interface/efi') diff --git a/src/interface/efi/efi_usb.c b/src/interface/efi/efi_usb.c index 48274f1d6..6f49b369e 100644 --- a/src/interface/efi/efi_usb.c +++ b/src/interface/efi/efi_usb.c @@ -1100,7 +1100,7 @@ static int efi_usb_install ( struct efi_usb_device *usbdev, usbpath->Header.Type = MESSAGING_DEVICE_PATH; usbpath->Header.SubType = MSG_USB_DP; usbpath->Header.Length[0] = sizeof ( *usbpath ); - usbpath->ParentPortNumber = usb->port->address; + usbpath->ParentPortNumber = ( usb->port->address - 1 ); } /* Add to list of interfaces */ -- cgit v1.2.3-55-g7522 From c63e61df75c815da1dc93196bbb6a931743b736f Mon Sep 17 00:00:00 2001 From: Michael Brown Date: Mon, 3 Aug 2020 15:26:25 +0100 Subject: [efi] Use device path to locate filesystem from which we were loaded The file:/ URI syntax may be used to refer to local files on the filesystem from which the iPXE binary was loaded. This is currently implemented by directly using the DeviceHandle recorded in our EFI_LOADED_IMAGE_PROTOCOL. This mechanism will fail when a USB-enabled build of iPXE is loaded from USB storage and subsequently installs its own USB host controller drivers, since doing so will disconnect and reconnect the existing USB storage drivers and thereby invalidate the original storage device handle. Fix by recording the device path for the loaded image's DeviceHandle at initialisation time and later using the recorded device path to locate the appropriate device handle. Signed-off-by: Michael Brown --- src/interface/efi/efi_init.c | 40 ++++++++++++++++++++++++++++++++++++++++ src/interface/efi/efi_local.c | 15 +++++++++++++-- 2 files changed, 53 insertions(+), 2 deletions(-) (limited to 'src/interface/efi') diff --git a/src/interface/efi/efi_init.c b/src/interface/efi/efi_init.c index e1041a5ec..70212b184 100644 --- a/src/interface/efi/efi_init.c +++ b/src/interface/efi/efi_init.c @@ -26,6 +26,7 @@ FILE_LICENCE ( GPL2_OR_LATER ); #include #include #include +#include #include /** Image handle passed to entry point */ @@ -34,6 +35,9 @@ EFI_HANDLE efi_image_handle; /** Loaded image protocol for this image */ EFI_LOADED_IMAGE_PROTOCOL *efi_loaded_image; +/** Device path for the loaded image's device handle */ +EFI_DEVICE_PATH_PROTOCOL *efi_loaded_image_path; + /** System table passed to entry point * * We construct the symbol name efi_systab via the PLATFORM macro. @@ -152,6 +156,9 @@ EFI_STATUS efi_init ( EFI_HANDLE image_handle, struct efi_protocol *prot; struct efi_config_table *tab; void *loaded_image; + void *device_path; + void *device_path_copy; + size_t device_path_len; EFI_STATUS efirc; int rc; @@ -230,6 +237,33 @@ EFI_STATUS efi_init ( EFI_HANDLE image_handle, DBGC ( systab, "EFI image base address %p\n", efi_loaded_image->ImageBase ); + /* Get loaded image's device handle's device path */ + if ( ( efirc = bs->OpenProtocol ( efi_loaded_image->DeviceHandle, + &efi_device_path_protocol_guid, + &device_path, image_handle, NULL, + EFI_OPEN_PROTOCOL_GET_PROTOCOL ) ) != 0 ) { + rc = -EEFI ( efirc ); + DBGC ( systab, "EFI could not get loaded image's device path: " + "%s", strerror ( rc ) ); + goto err_no_device_path; + } + + /* Make a copy of the loaded image's device handle's device + * path, since the device handle itself may become invalidated + * when we load our own drivers. + */ + device_path_len = ( efi_devpath_len ( device_path ) + + sizeof ( EFI_DEVICE_PATH_PROTOCOL ) ); + if ( ( efirc = bs->AllocatePool ( EfiBootServicesData, device_path_len, + &device_path_copy ) ) != 0 ) { + rc = -EEFI ( efirc ); + goto err_alloc_device_path; + } + memcpy ( device_path_copy, device_path, device_path_len ); + efi_loaded_image_path = device_path_copy; + DBGC ( systab, "EFI image device path %s\n", + efi_devpath_text ( efi_loaded_image_path ) ); + /* EFI is perfectly capable of gracefully shutting down any * loaded devices if it decides to fall back to a legacy boot. * For no particularly comprehensible reason, it doesn't @@ -261,6 +295,9 @@ EFI_STATUS efi_init ( EFI_HANDLE image_handle, err_driver_install: bs->CloseEvent ( efi_shutdown_event ); err_create_event: + bs->FreePool ( efi_loaded_image_path ); + err_alloc_device_path: + err_no_device_path: err_no_loaded_image: err_missing_table: err_missing_protocol: @@ -291,6 +328,9 @@ static EFI_STATUS EFIAPI efi_unload ( EFI_HANDLE image_handle __unused ) { /* Uninstall exit boot services event */ bs->CloseEvent ( efi_shutdown_event ); + /* Free copy of loaded image's device handle's device path */ + bs->FreePool ( efi_loaded_image_path ); + DBGC ( systab, "EFI image unloaded\n" ); return 0; diff --git a/src/interface/efi/efi_local.c b/src/interface/efi/efi_local.c index bd010ad2e..79ea822f3 100644 --- a/src/interface/efi/efi_local.c +++ b/src/interface/efi/efi_local.c @@ -307,6 +307,7 @@ static int efi_local_open_volume ( struct efi_local *local, EFI_GUID *protocol = &efi_simple_file_system_protocol_guid; int ( * check ) ( struct efi_local *local, EFI_HANDLE device, EFI_FILE_PROTOCOL *root, const char *volume ); + EFI_DEVICE_PATH_PROTOCOL *path; EFI_FILE_PROTOCOL *root; EFI_HANDLE *handles; EFI_HANDLE device; @@ -328,8 +329,18 @@ static int efi_local_open_volume ( struct efi_local *local, } check = efi_local_check_volume_name; } else { - /* Use our loaded image's device handle */ - handles = &efi_loaded_image->DeviceHandle; + /* Locate filesystem from which we were loaded */ + path = efi_loaded_image_path; + if ( ( efirc = bs->LocateDevicePath ( protocol, &path, + &device ) ) != 0 ) { + rc = -EEFI ( efirc ); + DBGC ( local, "LOCAL %p could not locate file system " + "on %s: %s\n", local, + efi_devpath_text ( efi_loaded_image_path ), + strerror ( rc ) ); + return rc; + } + handles = &device; num_handles = 1; check = NULL; } -- cgit v1.2.3-55-g7522