[PATCH v2 01/18] test_firmware: Test platform fw loading on non-EFI systems

[Scott Branden]

Looks good.

On 2020-07-22 12:30 p.m., Kees Cook wrote:
> On non-EFI systems, it wasn't possible to test the platform firmware
> loader because it will have never set "checked_fw" during __init.
> Instead, allow the test code to override this check. Additionally split
> the declarations into a private header file so it there is greater
> enforcement of the symbol visibility.
>
> Fixes: 548193cba2a7 ("test_firmware: add support for firmware_request_platform")
> Cc: stable at vger.kernel.org
> Signed-off-by: Kees Cook <keescook at chromium.org>
Acked-by: Scott Branden <scott.branden at broadcom.com>
> ---
>   drivers/firmware/efi/embedded-firmware.c | 21 ++++++++++++++++-----
>   drivers/firmware/efi/embedded-firmware.h | 19 +++++++++++++++++++
>   include/linux/efi_embedded_fw.h          | 13 -------------
>   lib/test_firmware.c                      |  5 +++++
>   4 files changed, 40 insertions(+), 18 deletions(-)
>   create mode 100644 drivers/firmware/efi/embedded-firmware.h
>
> diff --git a/drivers/firmware/efi/embedded-firmware.c b/drivers/firmware/efi/embedded-firmware.c
> index a1b199de9006..0fb03cd0a5a2 100644
> --- a/drivers/firmware/efi/embedded-firmware.c
> +++ b/drivers/firmware/efi/embedded-firmware.c
> @@ -14,11 +14,22 @@
>   #include <linux/vmalloc.h>
>   #include <crypto/sha.h>
>   
> +#include "embedded-firmware.h"
> +
> +#ifdef CONFIG_TEST_FIRMWARE
> +# define EFI_EMBEDDED_FW_VISIBILITY
> +#else
> +# define EFI_EMBEDDED_FW_VISIBILITY static
> +#endif
> +
> +EFI_EMBEDDED_FW_VISIBILITY LIST_HEAD(efi_embedded_fw_list);
> +EFI_EMBEDDED_FW_VISIBILITY bool efi_embedded_fw_checked;
> +
>   /* Exported for use by lib/test_firmware.c only */
> -LIST_HEAD(efi_embedded_fw_list);
> +#ifdef CONFIG_TEST_FIRMWARE
>   EXPORT_SYMBOL_GPL(efi_embedded_fw_list);
> -
> -static bool checked_for_fw;
> +EXPORT_SYMBOL_GPL(efi_embedded_fw_checked);
> +#endif
>   
>   static const struct dmi_system_id * const embedded_fw_table[] = {
>   #ifdef CONFIG_TOUCHSCREEN_DMI
> @@ -119,14 +130,14 @@ void __init efi_check_for_embedded_firmwares(void)
>   		}
>   	}
>   
> -	checked_for_fw = true;
> +	efi_embedded_fw_checked = true;
>   }
>   
>   int efi_get_embedded_fw(const char *name, const u8 **data, size_t *size)
>   {
>   	struct efi_embedded_fw *iter, *fw = NULL;
>   
> -	if (!checked_for_fw) {
> +	if (!efi_embedded_fw_checked) {
>   		pr_warn("Warning %s called while we did not check for embedded fw\n",
>   			__func__);
>   		return -ENOENT;
> diff --git a/drivers/firmware/efi/embedded-firmware.h b/drivers/firmware/efi/embedded-firmware.h
> new file mode 100644
> index 000000000000..34113316d068
> --- /dev/null
> +++ b/drivers/firmware/efi/embedded-firmware.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _EFI_EMBEDDED_FW_INTERNAL_H_
> +#define _EFI_EMBEDDED_FW_INTERNAL_H_
> +
> +/*
> + * This struct and efi_embedded_fw_list are private to the efi-embedded fw
> + * implementation they only in separate header for use by lib/test_firmware.c.
> + */
> +struct efi_embedded_fw {
> +	struct list_head list;
> +	const char *name;
> +	const u8 *data;
> +	size_t length;
> +};
> +
> +extern struct list_head efi_embedded_fw_list;
> +extern bool efi_embedded_fw_checked;
> +
> +#endif /* _EFI_EMBEDDED_FW_INTERNAL_H_ */
> diff --git a/include/linux/efi_embedded_fw.h b/include/linux/efi_embedded_fw.h
> index 57eac5241303..4ad5db9f5312 100644
> --- a/include/linux/efi_embedded_fw.h
> +++ b/include/linux/efi_embedded_fw.h
> @@ -7,19 +7,6 @@
>   
>   #define EFI_EMBEDDED_FW_PREFIX_LEN		8
>   
> -/*
> - * This struct and efi_embedded_fw_list are private to the efi-embedded fw
> - * implementation they are in this header for use by lib/test_firmware.c only!
> - */
> -struct efi_embedded_fw {
> -	struct list_head list;
> -	const char *name;
> -	const u8 *data;
> -	size_t length;
> -};
> -
> -extern struct list_head efi_embedded_fw_list;
> -
>   /**
>    * struct efi_embedded_fw_desc - This struct is used by the EFI embedded-fw
>    *                               code to search for embedded firmwares.
> diff --git a/lib/test_firmware.c b/lib/test_firmware.c
> index 9fee2b93a8d1..62af792e151c 100644
> --- a/lib/test_firmware.c
> +++ b/lib/test_firmware.c
> @@ -489,6 +489,7 @@ static ssize_t trigger_request_store(struct device *dev,
>   static DEVICE_ATTR_WO(trigger_request);
>   
>   #ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
> +#include "../drivers/firmware/efi/embedded-firmware.h"
>   static ssize_t trigger_request_platform_store(struct device *dev,
>   					      struct device_attribute *attr,
>   					      const char *buf, size_t count)
> @@ -501,6 +502,7 @@ static ssize_t trigger_request_platform_store(struct device *dev,
>   	};
>   	struct efi_embedded_fw efi_embedded_fw;
>   	const struct firmware *firmware = NULL;
> +	bool saved_efi_embedded_fw_checked;
>   	char *name;
>   	int rc;
>   
> @@ -513,6 +515,8 @@ static ssize_t trigger_request_platform_store(struct device *dev,
>   	efi_embedded_fw.data = (void *)test_data;
>   	efi_embedded_fw.length = sizeof(test_data);
>   	list_add(&efi_embedded_fw.list, &efi_embedded_fw_list);
> +	saved_efi_embedded_fw_checked = efi_embedded_fw_checked;
> +	efi_embedded_fw_checked = true;
>   
>   	pr_info("loading '%s'\n", name);
>   	rc = firmware_request_platform(&firmware, name, dev);
> @@ -530,6 +534,7 @@ static ssize_t trigger_request_platform_store(struct device *dev,
>   	rc = count;
>   
>   out:
> +	efi_embedded_fw_checked = saved_efi_embedded_fw_checked;
>   	release_firmware(firmware);
>   	list_del(&efi_embedded_fw.list);
>   	kfree(name);