| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4c09bd4b-4ad6-8a09-a94c-59dd422b7420@intel.com>
Date: Thu, 10 Feb 2022 18:29:11 -0800
From: Russ Weight <russell.h.weight@...el.com>
To: Luis Chamberlain <mcgrof@...nel.org>
CC: <gregkh@...uxfoundation.org>, <rafael@...nel.org>,
<linux-kernel@...r.kernel.org>, <trix@...hat.com>,
<lgoncalv@...hat.com>, <yilun.xu@...el.com>, <hao.wu@...el.com>,
<matthew.gerlach@...el.com>, <basheer.ahmed.muddebihal@...el.com>,
<tianfei.zhang@...el.com>
Subject: Re: [RFC PATCH 4/5] firmware_loader: Add firmware-upload support
On 2/3/22 2:59 PM, Luis Chamberlain wrote:
> On Thu, Feb 03, 2022 at 02:30:51PM -0700, Russ Weight wrote:
>> Extend the firmware subsystem to support a persistent sysfs interface that
>> userspace may use to initiate a firmware update. For example, FPGA based
>> PCIe cards load firmware and FPGA images from local FLASH when the card
>> boots. The images in FLASH may be updated with new images provided by the
>> user at his/her convenience.
>>
>> A device driver may call fw_upload_register() to expose persistent
>> "loading" and "data" sysfs files. These files are used in the same way as
>> the fallback sysfs "loading" and "data" files. When 0 is written to
>> "loading" to complete the write of firmware data, the data is transferred
>> to the lower-level driver using pre-registered call-back functions. The
>> data transfer is done in the context of a kernel worker thread.
>>
>> Signed-off-by: Russ Weight <russell.h.weight@...el.com>
>> ---
>> .../ABI/testing/sysfs-class-firmware | 32 +++
>> .../driver-api/firmware/fw_upload.rst | 86 +++++++
>> Documentation/driver-api/firmware/index.rst | 1 +
>> drivers/base/firmware_loader/Kconfig | 14 ++
>> drivers/base/firmware_loader/Makefile | 1 +
>> drivers/base/firmware_loader/firmware.h | 6 +
>> drivers/base/firmware_loader/fw_sysfs.c | 50 +++-
>> drivers/base/firmware_loader/fw_sysfs.h | 4 +
>> drivers/base/firmware_loader/fw_upload.c | 229 ++++++++++++++++++
>> drivers/base/firmware_loader/fw_upload.h | 24 ++
>> drivers/base/firmware_loader/main.c | 16 +-
>> include/linux/firmware.h | 72 ++++++
>> 12 files changed, 523 insertions(+), 12 deletions(-)
>> create mode 100644 Documentation/ABI/testing/sysfs-class-firmware
>> create mode 100644 Documentation/driver-api/firmware/fw_upload.rst
>> create mode 100644 drivers/base/firmware_loader/fw_upload.c
>> create mode 100644 drivers/base/firmware_loader/fw_upload.h
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-firmware b/Documentation/ABI/testing/sysfs-class-firmware
>> new file mode 100644
>> index 000000000000..a2e518f0bf8a
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-firmware
>> @@ -0,0 +1,32 @@
>> +What: /sys/class/firmware/.../data
>> +Date: Mar 2022
>> +KernelVersion: 5.18
>> +Contact: Russ Weight <russell.h.weight@...el.com>
>> +Description: The data sysfs file is used for firmware-fallback and for
>> + firmware uploads. Cat a firmware image to this sysfs file
>> + after you echo 1 to the loading sysfs file. When the firmware
>> + image write is complete, echo 0 to the loading sysfs file. This
>> + sequence will signal the completion of the firmware write and
>> + signal the lower-level driver that the firmware data is
>> + available.
>> +
>> +What: /sys/class/firmware/.../loading
>> +Date: Mar 2022
>> +KernelVersion: 5.18
>> +Contact: Russ Weight <russell.h.weight@...el.com>
>> +Description: The loading sysfs file is used for both firmware-fallback and
>> + for firmware uploads. Echo 1 onto the loading file to indicate
>> + you are writing a firmware file to the data sysfs node. Echo
>> + -1 onto this file to abort the data write or echo 0 onto this
>> + file to indicate that the write is complete. For firmware
>> + uploads, the zero value also triggers the transfer of the
>> + firmware data to the lower-level device driver.
>> +
>> +What: /sys/class/firmware/.../timeout
>> +Date: Mar 2022
>> +KernelVersion: 5.18
>> +Contact: Russ Weight <russell.h.weight@...el.com>
>> +Description: This file supports the timeout mechanism for firmware
>> + fallback. This file has no affect on firmware uploads. For
>> + more information on timeouts please see the documentation
>> + for firmware fallback.
>> diff --git a/Documentation/driver-api/firmware/fw_upload.rst b/Documentation/driver-api/firmware/fw_upload.rst
>> new file mode 100644
>> index 000000000000..bf272f627a1f
>> --- /dev/null
>> +++ b/Documentation/driver-api/firmware/fw_upload.rst
>> @@ -0,0 +1,86 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +=============
>> +fw_upload API
>> +=============
>> +
>> +A device driver that registers with the firmware loader will expose
>> +persistent sysfs nodes to enable users to initiate firmware updates for
>> +that device. It is the responsibility of the device driver and/or the
>> +device itself to perform any validation on the data received. Firmware
>> +upload uses the same *loading* and *data* sysfs files described in the
>> +documentation for firmware fallback.
>> +
>> +Register for firmware upload
>> +============================
>> +
>> +A device driver registers for firmware upload by calling fw_upload_register().
>> +Among the parameter list is a name to identify the device under
>> +/sys/class/firmware. A user may initiate a firmware upload by echoing
>> +a 1 to the *loading* sysfs file for the target device. Next, the user writes
>> +the firmware image to the *data* sysfs file. After writing the firmware
>> +data, the user echos 0 to the *loading* sysfs file to signal completion.
>> +Echoing 0 to *loading* also triggers the transfer of the firmware to the
>> +lower-lever device driver in the context of a kernel worker thread.
>> +
>> +To use the fw_upload API, write a driver that implements a set of ops. The
>> +probe function calls fw_upload_register() and the remove function calls
>> +fw_upload_unregister() such as::
>> +
>> + static const struct fw_upload_ops m10bmc_ops = {
>> + .prepare = m10bmc_sec_prepare,
>> + .write = m10bmc_sec_write,
>> + .poll_complete = m10bmc_sec_poll_complete,
>> + .cancel = m10bmc_sec_cancel,
>> + .cleanup = m10bmc_sec_cleanup,
>> + };
>> +
>> + static int m10bmc_sec_probe(struct platform_device *pdev)
>> + {
>> + const char *fw_name, *truncate;
>> + struct m10bmc_sec *sec;
>> + struct fw_upload *fwl;
>> + unsigned int len;
>> +
>> + sec = devm_kzalloc(&pdev->dev, sizeof(*sec), GFP_KERNEL);
>> + if (!sec)
>> + return -ENOMEM;
>> +
>> + sec->dev = &pdev->dev;
>> + sec->m10bmc = dev_get_drvdata(pdev->dev.parent);
>> + dev_set_drvdata(&pdev->dev, sec);
>> +
>> + fw_name = dev_name(sec->dev);
>> + truncate = strstr(fw_name, ".auto");
>> + len = (truncate) ? truncate - fw_name : strlen(fw_name);
>> + sec->fw_name = kmemdup_nul(fw_name, len, GFP_KERNEL);
>> +
>> + fwl = fw_upload_register(sec->dev, sec->fw_name, &m10bmc_ops, sec);
>> + if (IS_ERR(fwl)) {
>> + dev_err(sec->dev, "Firmware Upload driver failed to start\n");
>> + kfree(sec->fw_name);
>> + return PTR_ERR(fwl);
>> + }
>> +
>> + sec->fwl = fwl;
>> + return 0;
>> + }
>> +
>> + static int m10bmc_sec_remove(struct platform_device *pdev)
>> + {
>> + struct m10bmc_sec *sec = dev_get_drvdata(&pdev->dev);
>> +
>> + fw_upload_unregister(sec->fwl);
>> + kfree(sec->fw_name);
>> + return 0;
>> + }
>> +
>> +fw_upload_register
>> +------------------
>> +.. kernel-doc:: drivers/base/firmware_loader/fw_upload.c
>> + :functions: fw_upload_register
>> +
>> +fw_upload_unregister
>> +--------------------
>> +.. kernel-doc:: drivers/base/firmware_loader/fw_upload.c
>> + :functions: fw_upload_unregister
>> diff --git a/Documentation/driver-api/firmware/index.rst b/Documentation/driver-api/firmware/index.rst
>> index 57415d657173..9d2c19dc8e36 100644
>> --- a/Documentation/driver-api/firmware/index.rst
>> +++ b/Documentation/driver-api/firmware/index.rst
>> @@ -8,6 +8,7 @@ Linux Firmware API
>> core
>> efi/index
>> request_firmware
>> + fw_upload
>> other_interfaces
>>
>> .. only:: subproject and html
>> diff --git a/drivers/base/firmware_loader/Kconfig b/drivers/base/firmware_loader/Kconfig
>> index 1bfe18900ed5..cee662f3277b 100644
>> --- a/drivers/base/firmware_loader/Kconfig
>> +++ b/drivers/base/firmware_loader/Kconfig
>> @@ -185,5 +185,19 @@ config FW_CACHE
>>
>> If unsure, say Y.
>>
>> +config FW_UPLOAD
>> + bool "Enable users to initiate firmware updates using sysfs"
>> + select FW_LOADER_SYSFS
>> + select FW_LOADER_PAGED_BUF
>> + help
>> + Enabling this option will allow device drivers to expose a persistent
>> + sysfs interface that allows firmware updates to be initiated from
>> + userspace. For example, FPGA based PCIe cards load firmware and FPGA
>> + images from local FLASH when the card boots. The images in FLASH may
>> + be updated with new images provided by the user. Enable this device
>> + to support cards that rely on user-initiated updates for firmware files.
>> +
>> + If unsure, say N.
>> +
>> endif # FW_LOADER
>> endmenu
>> diff --git a/drivers/base/firmware_loader/Makefile b/drivers/base/firmware_loader/Makefile
>> index 787c833d0c6e..52ef64bd9357 100644
>> --- a/drivers/base/firmware_loader/Makefile
>> +++ b/drivers/base/firmware_loader/Makefile
>> @@ -7,5 +7,6 @@ firmware_class-objs := main.o
>> firmware_class-$(CONFIG_FW_LOADER_USER_HELPER) += fallback.o
>> firmware_class-$(CONFIG_EFI_EMBEDDED_FIRMWARE) += fallback_platform.o
>> firmware_class-$(CONFIG_FW_LOADER_SYSFS) += fw_sysfs.o
>> +firmware_class-$(CONFIG_FW_UPLOAD) += fw_upload.o
>>
>> obj-y += builtin/
>> diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h
>> index 58768d16f8df..4019f9423de8 100644
>> --- a/drivers/base/firmware_loader/firmware.h
>> +++ b/drivers/base/firmware_loader/firmware.h
>> @@ -87,6 +87,7 @@ struct fw_priv {
>> };
>>
>> extern struct mutex fw_lock;
>> +extern struct firmware_cache fw_cache;
>>
>> static inline bool __fw_state_check(struct fw_priv *fw_priv,
>> enum fw_status status)
>> @@ -154,7 +155,12 @@ static inline bool fw_state_is_done(struct fw_priv *fw_priv)
>> return __fw_state_check(fw_priv, FW_STATUS_DONE);
>> }
>>
>> +int alloc_lookup_fw_priv(const char *fw_name, struct firmware_cache *fwc,
>> + struct fw_priv **fw_priv, void *dbuf, size_t size,
>> + size_t offset, u32 opt_flags);
>> int assign_fw(struct firmware *fw, struct device *device);
>> +void free_fw_priv(struct fw_priv *fw_priv);
>> +void fw_state_init(struct fw_priv *fw_priv);
>>
>> #ifdef CONFIG_FW_LOADER
>> bool firmware_is_builtin(const struct firmware *fw);
>> diff --git a/drivers/base/firmware_loader/fw_sysfs.c b/drivers/base/firmware_loader/fw_sysfs.c
>> index 70cb1d67ffb2..9b0cd37c81df 100644
>> --- a/drivers/base/firmware_loader/fw_sysfs.c
>> +++ b/drivers/base/firmware_loader/fw_sysfs.c
>> @@ -6,8 +6,8 @@
>> #include <linux/slab.h>
>> #include <linux/types.h>
>>
>> -#include "firmware.h"
>> #include "fw_sysfs.h"
>> +#include "fw_upload.h"
>>
>> /*
>> * sysfs support for firmware loader
>> @@ -80,6 +80,10 @@ static void fw_dev_release(struct device *dev)
>> {
>> struct fw_sysfs *fw_sysfs = to_fw_sysfs(dev);
>>
>> + if (fw_sysfs->fw_upload_priv) {
>> + free_fw_priv(fw_sysfs->fw_priv);
>> + kfree(fw_sysfs->fw_upload_priv);
>> + }
>> kfree(fw_sysfs);
>> }
>>
>> @@ -165,6 +169,9 @@ static ssize_t firmware_loading_store(struct device *dev,
>> const char *buf, size_t count)
>> {
>> struct fw_sysfs *fw_sysfs = to_fw_sysfs(dev);
>> +#ifdef CONFIG_FW_UPLOAD
>> + struct fw_upload_priv *fwlp;
>> +#endif
>> struct fw_priv *fw_priv;
>> ssize_t written = count;
>> int loading = simple_strtol(buf, NULL, 10);
>> @@ -211,6 +218,42 @@ static ssize_t firmware_loading_store(struct device *dev,
>> written = rc;
>> } else {
>> fw_state_done(fw_priv);
>> +
>> +#ifdef CONFIG_FW_UPLOAD
>> + /*
>> + * For fw_uploads, start a worker thread to upload
>> + * data to the parent driver.
>> + */
>> + if (!fw_sysfs->fw_upload_priv)
>> + break;
>> +
>> + if (!fw_priv->size) {
>> + fw_free_paged_buf(fw_priv);
>> + fw_state_init(fw_sysfs->fw_priv);
>> + break;
>> + }
>> +
>> + fwlp = fw_sysfs->fw_upload_priv;
>> + mutex_lock(&fwlp->lock);
>> +
>> + /* Do not interfere an on-going fw_upload */
>> + if (fwlp->progress != FW_UPLOAD_PROG_IDLE) {
>> + mutex_unlock(&fwlp->lock);
>> + written = -EBUSY;
>> + goto out;
>> + }
>> +
>> + fwlp->progress = FW_UPLOAD_PROG_RECEIVING;
>> + fwlp->err_code = 0;
>> + fwlp->remaining_size = fw_priv->size;
>> + fwlp->data = fw_priv->data;
>> + pr_debug("%s: fw-%s fw_priv=%p data=%p size=%u\n",
>> + __func__, fw_priv->fw_name,
>> + fw_priv, fw_priv->data,
>> + (unsigned int)fw_priv->size);
>> + queue_work(system_long_wq, &fwlp->work);
>> + mutex_unlock(&fwlp->lock);
>> +#endif
> If you're going to split this work up it'd be nice to avoid to have
> #ifdef stuff here.
Yes - I'll encapsulate the code into another function and move it
to the sysfs-upload.c file.
>
>> diff --git a/drivers/base/firmware_loader/fw_upload.c b/drivers/base/firmware_loader/fw_upload.c
> No need for fw prefix, sysfs-upload.[ch] would make it clearer.
OK - I'll switch to sysfs-upload.[ch]
>
>> +EXPORT_SYMBOL_GPL(fw_upload_register);
> firmware_upload_register()
>
>> +EXPORT_SYMBOL_GPL(fw_upload_unregister);
> firmware_upload_unregister()
Sure - I'll change the register/unregister function names.
>
>> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
>> index 3b057dfc8284..9b109f8ff627 100644
>> --- a/include/linux/firmware.h
>> +++ b/include/linux/firmware.h
>> @@ -17,6 +17,56 @@ struct firmware {
>> void *priv;
>> };
>>
>
>> +/* Update progress codes */
>> +#define FW_UPLOAD_PROG_IDLE 0
>> +#define FW_UPLOAD_PROG_RECEIVING 1
>> +#define FW_UPLOAD_PROG_PREPARING 2
>> +#define FW_UPLOAD_PROG_TRANSFERRING 3
>> +#define FW_UPLOAD_PROG_PROGRAMMING 4
>> +#define FW_UPLOAD_PROG_MAX 5
> enums with kdoc allows you to nicely document these as well.
OK. I'll switch to enums. I also realised that the FW_UPLOAD_PROG*
constants can be defined in a smaller scope. I'll move these to
sysfs-upload.h.
>> +
>> +/* Update error progress codes */
>> +#define FW_UPLOAD_ERR_HW_ERROR 1
>> +#define FW_UPLOAD_ERR_TIMEOUT 2
>> +#define FW_UPLOAD_ERR_CANCELED 3
>> +#define FW_UPLOAD_ERR_BUSY 4
>> +#define FW_UPLOAD_ERR_INVALID_SIZE 5
>> +#define FW_UPLOAD_ERR_RW_ERROR 6
>> +#define FW_UPLOAD_ERR_WEAROUT 7
>> +#define FW_UPLOAD_ERR_MAX 8
> enums with kdoc allows you to nicely document these as well.
Thanks,
- Russ
>
> Luis
Powered by blists - more mailing lists