2023-03-10 14:06:05

by Amadeusz Sławiński

[permalink] [raw]
Subject: [PATCH v4] firmware_loader: Add debug message with checksum for FW file

Enable dynamic-debug logging of firmware filenames and SHA256 checksums
to clearly identify the firmware files that are loaded by the system.

Example output:
[ 34.944619] firmware_class:_request_firmware: i915 0000:00:02.0: Loaded FW: i915/kbl_dmc_ver1_04.bin, sha256: 2cde41c3e5ad181423bcc3e98ff9c49f743c88f18646af4d0b3c3a9664b831a1
[ 48.155884] firmware_class:_request_firmware: snd_soc_avs 0000:00:1f.3: Loaded FW: intel/avs/cnl/dsp_basefw.bin, sha256: 43f6ac1b066e9bd0423d914960fbbdccb391af27d2b1da1085eee3ea8df0f357
[ 49.579540] firmware_class:_request_firmware: snd_soc_avs 0000:00:1f.3: Loaded FW: intel/avs/rt274-tplg.bin, sha256: 4b3580da96dc3d2c443ba20c6728d8b665fceb3ed57223c3a57582bbad8e2413
[ 49.798196] firmware_class:_request_firmware: snd_soc_avs 0000:00:1f.3: Loaded FW: intel/avs/hda-8086280c-tplg.bin, sha256: 5653172579b2be1b51fd69f5cf46e2bac8d63f2a1327924311c13b2f1fe6e601
[ 49.859627] firmware_class:_request_firmware: snd_soc_avs 0000:00:1f.3: Loaded FW: intel/avs/dmic-tplg.bin, sha256: 00fb7fbdb74683333400d7e46925dae60db448b88638efcca0b30215db9df63f

Reviewed-by: Cezary Rojewski <[email protected]>
Signed-off-by: Amadeusz Sławiński <[email protected]>
---

Changes in v4:
* update menuconfig prompt and help message (Russ)

Changes in v3:
* add DYNAMIC_DEBUG and FW_LOADER as dependencies before option can be
enabled (kernel test robot)

Changes in v2:
* allocate buffers (Greg)
* introduce CONFIG_ option to allow for CONFIG_CRYPTO and CONFIG_CRYPTO_SHA256
dependencies without introducing circular dependency (Greg)
* add new line between includes and function name (Cezary)

---
drivers/base/firmware_loader/Kconfig | 13 ++++++++
drivers/base/firmware_loader/main.c | 48 +++++++++++++++++++++++++++-
2 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/drivers/base/firmware_loader/Kconfig b/drivers/base/firmware_loader/Kconfig
index 5166b323a0f8..6520e8c9cb38 100644
--- a/drivers/base/firmware_loader/Kconfig
+++ b/drivers/base/firmware_loader/Kconfig
@@ -3,6 +3,7 @@ menu "Firmware loader"

config FW_LOADER
tristate "Firmware loading facility" if EXPERT
+ select FW_LOADER_DEBUG if DYNAMIC_DEBUG
default y
help
This enables the firmware loading facility in the kernel. The kernel
@@ -24,6 +25,18 @@ config FW_LOADER
You also want to be sure to enable this built-in if you are going to
enable built-in firmware (CONFIG_EXTRA_FIRMWARE).

+config FW_LOADER_DEBUG
+ bool "Log filenames and checksums for loaded firmware"
+ depends on DYNAMIC_DEBUG
+ depends on FW_LOADER
+ depends on CRYPTO
+ depends on CRYPTO_SHA256
+ default FW_LOADER
+ help
+ Select this option to use dynamic debug to log firmware filenames and
+ SHA256 checksums to the kernel log for each firmware file that is
+ loaded.
+
if FW_LOADER

config FW_LOADER_PAGED_BUF
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index 017c4cdb219e..b2c292ca95e8 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -791,6 +791,50 @@ static void fw_abort_batch_reqs(struct firmware *fw)
mutex_unlock(&fw_lock);
}

+#if defined(CONFIG_FW_LOADER_DEBUG)
+#include <crypto/hash.h>
+#include <crypto/sha2.h>
+
+static void fw_log_firmware_info(const struct firmware *fw, const char *name, struct device *device)
+{
+ struct shash_desc *shash;
+ struct crypto_shash *alg;
+ u8 *sha256buf;
+ char *outbuf;
+
+ alg = crypto_alloc_shash("sha256", 0, 0);
+ if (!alg)
+ return;
+
+ sha256buf = kmalloc(SHA256_DIGEST_SIZE, GFP_KERNEL);
+ outbuf = kmalloc(SHA256_BLOCK_SIZE + 1, GFP_KERNEL);
+ shash = kmalloc(sizeof(*shash) + crypto_shash_descsize(alg), GFP_KERNEL);
+ if (!sha256buf || !outbuf || !shash)
+ goto out_free;
+
+ shash->tfm = alg;
+
+ if (crypto_shash_digest(shash, fw->data, fw->size, sha256buf) < 0)
+ goto out_shash;
+
+ for (int i = 0; i < SHA256_DIGEST_SIZE; i++)
+ sprintf(&outbuf[i * 2], "%02x", sha256buf[i]);
+ outbuf[SHA256_BLOCK_SIZE] = 0;
+ dev_dbg(device, "Loaded FW: %s, sha256: %s\n", name, outbuf);
+
+out_shash:
+ crypto_free_shash(alg);
+out_free:
+ kfree(shash);
+ kfree(outbuf);
+ kfree(sha256buf);
+}
+#else
+static void fw_log_firmware_info(const struct firmware *fw, const char *name,
+ struct device *device)
+{}
+#endif
+
/* called from request_firmware() and request_firmware_work_func() */
static int
_request_firmware(const struct firmware **firmware_p, const char *name,
@@ -861,11 +905,13 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
revert_creds(old_cred);
put_cred(kern_cred);

- out:
+out:
if (ret < 0) {
fw_abort_batch_reqs(fw);
release_firmware(fw);
fw = NULL;
+ } else {
+ fw_log_firmware_info(fw, name, device);
}

*firmware_p = fw;
--
2.34.1



2023-03-10 17:31:09

by Russ Weight

[permalink] [raw]
Subject: Re: [PATCH v4] firmware_loader: Add debug message with checksum for FW file



On 3/10/23 06:04, Amadeusz Sławiński wrote:
> Enable dynamic-debug logging of firmware filenames and SHA256 checksums
> to clearly identify the firmware files that are loaded by the system.
>
> Example output:
> [ 34.944619] firmware_class:_request_firmware: i915 0000:00:02.0: Loaded FW: i915/kbl_dmc_ver1_04.bin, sha256: 2cde41c3e5ad181423bcc3e98ff9c49f743c88f18646af4d0b3c3a9664b831a1
> [ 48.155884] firmware_class:_request_firmware: snd_soc_avs 0000:00:1f.3: Loaded FW: intel/avs/cnl/dsp_basefw.bin, sha256: 43f6ac1b066e9bd0423d914960fbbdccb391af27d2b1da1085eee3ea8df0f357
> [ 49.579540] firmware_class:_request_firmware: snd_soc_avs 0000:00:1f.3: Loaded FW: intel/avs/rt274-tplg.bin, sha256: 4b3580da96dc3d2c443ba20c6728d8b665fceb3ed57223c3a57582bbad8e2413
> [ 49.798196] firmware_class:_request_firmware: snd_soc_avs 0000:00:1f.3: Loaded FW: intel/avs/hda-8086280c-tplg.bin, sha256: 5653172579b2be1b51fd69f5cf46e2bac8d63f2a1327924311c13b2f1fe6e601
> [ 49.859627] firmware_class:_request_firmware: snd_soc_avs 0000:00:1f.3: Loaded FW: intel/avs/dmic-tplg.bin, sha256: 00fb7fbdb74683333400d7e46925dae60db448b88638efcca0b30215db9df63f
>
> Reviewed-by: Cezary Rojewski <[email protected]>

Reviewed-by: Russ Weight <[email protected]>
> Signed-off-by: Amadeusz Sławiński <[email protected]>
> ---
>
> Changes in v4:
> * update menuconfig prompt and help message (Russ)
>
> Changes in v3:
> * add DYNAMIC_DEBUG and FW_LOADER as dependencies before option can be
> enabled (kernel test robot)
>
> Changes in v2:
> * allocate buffers (Greg)
> * introduce CONFIG_ option to allow for CONFIG_CRYPTO and CONFIG_CRYPTO_SHA256
> dependencies without introducing circular dependency (Greg)
> * add new line between includes and function name (Cezary)
>
> ---
> drivers/base/firmware_loader/Kconfig | 13 ++++++++
> drivers/base/firmware_loader/main.c | 48 +++++++++++++++++++++++++++-
> 2 files changed, 60 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/firmware_loader/Kconfig b/drivers/base/firmware_loader/Kconfig
> index 5166b323a0f8..6520e8c9cb38 100644
> --- a/drivers/base/firmware_loader/Kconfig
> +++ b/drivers/base/firmware_loader/Kconfig
> @@ -3,6 +3,7 @@ menu "Firmware loader"
>
> config FW_LOADER
> tristate "Firmware loading facility" if EXPERT
> + select FW_LOADER_DEBUG if DYNAMIC_DEBUG
> default y
> help
> This enables the firmware loading facility in the kernel. The kernel
> @@ -24,6 +25,18 @@ config FW_LOADER
> You also want to be sure to enable this built-in if you are going to
> enable built-in firmware (CONFIG_EXTRA_FIRMWARE).
>
> +config FW_LOADER_DEBUG
> + bool "Log filenames and checksums for loaded firmware"
> + depends on DYNAMIC_DEBUG
> + depends on FW_LOADER
> + depends on CRYPTO
> + depends on CRYPTO_SHA256
> + default FW_LOADER
> + help
> + Select this option to use dynamic debug to log firmware filenames and
> + SHA256 checksums to the kernel log for each firmware file that is
> + loaded.
> +
> if FW_LOADER
>
> config FW_LOADER_PAGED_BUF
> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> index 017c4cdb219e..b2c292ca95e8 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -791,6 +791,50 @@ static void fw_abort_batch_reqs(struct firmware *fw)
> mutex_unlock(&fw_lock);
> }
>
> +#if defined(CONFIG_FW_LOADER_DEBUG)
> +#include <crypto/hash.h>
> +#include <crypto/sha2.h>
> +
> +static void fw_log_firmware_info(const struct firmware *fw, const char *name, struct device *device)
> +{
> + struct shash_desc *shash;
> + struct crypto_shash *alg;
> + u8 *sha256buf;
> + char *outbuf;
> +
> + alg = crypto_alloc_shash("sha256", 0, 0);
> + if (!alg)
> + return;
> +
> + sha256buf = kmalloc(SHA256_DIGEST_SIZE, GFP_KERNEL);
> + outbuf = kmalloc(SHA256_BLOCK_SIZE + 1, GFP_KERNEL);
> + shash = kmalloc(sizeof(*shash) + crypto_shash_descsize(alg), GFP_KERNEL);
> + if (!sha256buf || !outbuf || !shash)
> + goto out_free;
> +
> + shash->tfm = alg;
> +
> + if (crypto_shash_digest(shash, fw->data, fw->size, sha256buf) < 0)
> + goto out_shash;
> +
> + for (int i = 0; i < SHA256_DIGEST_SIZE; i++)
> + sprintf(&outbuf[i * 2], "%02x", sha256buf[i]);
> + outbuf[SHA256_BLOCK_SIZE] = 0;
> + dev_dbg(device, "Loaded FW: %s, sha256: %s\n", name, outbuf);
> +
> +out_shash:
> + crypto_free_shash(alg);
> +out_free:
> + kfree(shash);
> + kfree(outbuf);
> + kfree(sha256buf);
> +}
> +#else
> +static void fw_log_firmware_info(const struct firmware *fw, const char *name,
> + struct device *device)
> +{}
> +#endif
> +
> /* called from request_firmware() and request_firmware_work_func() */
> static int
> _request_firmware(const struct firmware **firmware_p, const char *name,
> @@ -861,11 +905,13 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> revert_creds(old_cred);
> put_cred(kern_cred);
>
> - out:
> +out:
> if (ret < 0) {
> fw_abort_batch_reqs(fw);
> release_firmware(fw);
> fw = NULL;
> + } else {
> + fw_log_firmware_info(fw, name, device);
> }
>
> *firmware_p = fw;


2023-03-17 14:15:14

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH v4] firmware_loader: Add debug message with checksum for FW file

On Fri, Mar 10, 2023 at 03:04:59PM +0100, Amadeusz Sławiński wrote:
> Enable dynamic-debug logging of firmware filenames and SHA256 checksums
> to clearly identify the firmware files that are loaded by the system.
>
> Example output:
> [ 34.944619] firmware_class:_request_firmware: i915 0000:00:02.0: Loaded FW: i915/kbl_dmc_ver1_04.bin, sha256: 2cde41c3e5ad181423bcc3e98ff9c49f743c88f18646af4d0b3c3a9664b831a1
> [ 48.155884] firmware_class:_request_firmware: snd_soc_avs 0000:00:1f.3: Loaded FW: intel/avs/cnl/dsp_basefw.bin, sha256: 43f6ac1b066e9bd0423d914960fbbdccb391af27d2b1da1085eee3ea8df0f357
> [ 49.579540] firmware_class:_request_firmware: snd_soc_avs 0000:00:1f.3: Loaded FW: intel/avs/rt274-tplg.bin, sha256: 4b3580da96dc3d2c443ba20c6728d8b665fceb3ed57223c3a57582bbad8e2413
> [ 49.798196] firmware_class:_request_firmware: snd_soc_avs 0000:00:1f.3: Loaded FW: intel/avs/hda-8086280c-tplg.bin, sha256: 5653172579b2be1b51fd69f5cf46e2bac8d63f2a1327924311c13b2f1fe6e601
> [ 49.859627] firmware_class:_request_firmware: snd_soc_avs 0000:00:1f.3: Loaded FW: intel/avs/dmic-tplg.bin, sha256: 00fb7fbdb74683333400d7e46925dae60db448b88638efcca0b30215db9df63f
>
> Reviewed-by: Cezary Rojewski <[email protected]>
> Signed-off-by: Amadeusz Sławiński <[email protected]>
> ---
>
> Changes in v4:
> * update menuconfig prompt and help message (Russ)
>
> Changes in v3:
> * add DYNAMIC_DEBUG and FW_LOADER as dependencies before option can be
> enabled (kernel test robot)
>
> Changes in v2:
> * allocate buffers (Greg)
> * introduce CONFIG_ option to allow for CONFIG_CRYPTO and CONFIG_CRYPTO_SHA256
> dependencies without introducing circular dependency (Greg)
> * add new line between includes and function name (Cezary)
>
> ---
> drivers/base/firmware_loader/Kconfig | 13 ++++++++
> drivers/base/firmware_loader/main.c | 48 +++++++++++++++++++++++++++-
> 2 files changed, 60 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/firmware_loader/Kconfig b/drivers/base/firmware_loader/Kconfig
> index 5166b323a0f8..6520e8c9cb38 100644
> --- a/drivers/base/firmware_loader/Kconfig
> +++ b/drivers/base/firmware_loader/Kconfig
> @@ -3,6 +3,7 @@ menu "Firmware loader"
>
> config FW_LOADER
> tristate "Firmware loading facility" if EXPERT
> + select FW_LOADER_DEBUG if DYNAMIC_DEBUG

Why the select? that prevents anyone from actually choosing this if
they want to or not. It also prevents them from disabling this option
if they want to, while still keeping DYNAMIC_DEBUG enabled.

So please don't make this change.

thanks,

greg k-h

2023-03-17 14:47:06

by Amadeusz Sławiński

[permalink] [raw]
Subject: Re: [PATCH v4] firmware_loader: Add debug message with checksum for FW file

On 3/17/2023 3:15 PM, Greg Kroah-Hartman wrote:
> On Fri, Mar 10, 2023 at 03:04:59PM +0100, Amadeusz Sławiński wrote:
>> Enable dynamic-debug logging of firmware filenames and SHA256 checksums
>> to clearly identify the firmware files that are loaded by the system.
>>
>> Example output:
>> [ 34.944619] firmware_class:_request_firmware: i915 0000:00:02.0: Loaded FW: i915/kbl_dmc_ver1_04.bin, sha256: 2cde41c3e5ad181423bcc3e98ff9c49f743c88f18646af4d0b3c3a9664b831a1
>> [ 48.155884] firmware_class:_request_firmware: snd_soc_avs 0000:00:1f.3: Loaded FW: intel/avs/cnl/dsp_basefw.bin, sha256: 43f6ac1b066e9bd0423d914960fbbdccb391af27d2b1da1085eee3ea8df0f357
>> [ 49.579540] firmware_class:_request_firmware: snd_soc_avs 0000:00:1f.3: Loaded FW: intel/avs/rt274-tplg.bin, sha256: 4b3580da96dc3d2c443ba20c6728d8b665fceb3ed57223c3a57582bbad8e2413
>> [ 49.798196] firmware_class:_request_firmware: snd_soc_avs 0000:00:1f.3: Loaded FW: intel/avs/hda-8086280c-tplg.bin, sha256: 5653172579b2be1b51fd69f5cf46e2bac8d63f2a1327924311c13b2f1fe6e601
>> [ 49.859627] firmware_class:_request_firmware: snd_soc_avs 0000:00:1f.3: Loaded FW: intel/avs/dmic-tplg.bin, sha256: 00fb7fbdb74683333400d7e46925dae60db448b88638efcca0b30215db9df63f
>>
>> Reviewed-by: Cezary Rojewski <[email protected]>
>> Signed-off-by: Amadeusz Sławiński <[email protected]>
>> ---
>>
>> Changes in v4:
>> * update menuconfig prompt and help message (Russ)
>>
>> Changes in v3:
>> * add DYNAMIC_DEBUG and FW_LOADER as dependencies before option can be
>> enabled (kernel test robot)
>>
>> Changes in v2:
>> * allocate buffers (Greg)
>> * introduce CONFIG_ option to allow for CONFIG_CRYPTO and CONFIG_CRYPTO_SHA256
>> dependencies without introducing circular dependency (Greg)
>> * add new line between includes and function name (Cezary)
>>
>> ---
>> drivers/base/firmware_loader/Kconfig | 13 ++++++++
>> drivers/base/firmware_loader/main.c | 48 +++++++++++++++++++++++++++-
>> 2 files changed, 60 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/firmware_loader/Kconfig b/drivers/base/firmware_loader/Kconfig
>> index 5166b323a0f8..6520e8c9cb38 100644
>> --- a/drivers/base/firmware_loader/Kconfig
>> +++ b/drivers/base/firmware_loader/Kconfig
>> @@ -3,6 +3,7 @@ menu "Firmware loader"
>>
>> config FW_LOADER
>> tristate "Firmware loading facility" if EXPERT
>> + select FW_LOADER_DEBUG if DYNAMIC_DEBUG
>
> Why the select? that prevents anyone from actually choosing this if
> they want to or not. It also prevents them from disabling this option
> if they want to, while still keeping DYNAMIC_DEBUG enabled.
>
> So please don't make this change.
>
Indeed it seems unnecessary, I removed it in v5.

Thanks,

Amadeusz