From: Luis Chamberlain <mcgrof@kernel.org>
To: Kees Cook <keescook@chromium.org>, Takashi Iwai <tiwai@suse.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Scott Branden <scott.branden@broadcom.com>,
Mimi Zohar <zohar@linux.ibm.com>, Jessica Yu <jeyu@kernel.org>,
SeongJae Park <sjpark@amazon.de>, KP Singh <kpsingh@chromium.org>,
linux-efi@vger.kernel.org, linux-security-module@vger.kernel.org,
linux-integrity@vger.kernel.org, selinux@vger.kernel.org,
linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 18/19] firmware: Add request_partial_firmware_into_buf()
Date: Wed, 29 Jul 2020 01:17:39 +0000 [thread overview]
Message-ID: <20200729011739.GL4332@42.do-not-panic.com> (raw)
In-Reply-To: <20200724213640.389191-19-keescook@chromium.org>
Long ago Takashi had some points about this strategy breaking
compressed file use. Was that considered?
Luis
On Fri, Jul 24, 2020 at 02:36:39PM -0700, Kees Cook wrote:
> From: Scott Branden <scott.branden@broadcom.com>
>
> Add request_partial_firmware_into_buf() to allow for portions of a
> firmware file to be read into a buffer. This is needed when large firmware
> must be loaded in portions from a file on memory constrained systems.
>
> Signed-off-by: Scott Branden <scott.branden@broadcom.com>
> Co-developed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> drivers/base/firmware_loader/firmware.h | 4 +
> drivers/base/firmware_loader/main.c | 109 +++++++++++++++++++-----
> include/linux/firmware.h | 12 +++
> 3 files changed, 105 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h
> index 7ad5fe52bc72..3f6eda46b3a2 100644
> --- a/drivers/base/firmware_loader/firmware.h
> +++ b/drivers/base/firmware_loader/firmware.h
> @@ -32,6 +32,8 @@
> * @FW_OPT_FALLBACK_PLATFORM: Enable fallback to device fw copy embedded in
> * the platform's main firmware. If both this fallback and the sysfs
> * fallback are enabled, then this fallback will be tried first.
> + * @FW_OPT_PARTIAL: Allow partial read of firmware instead of needing to read
> + * entire file.
> */
> enum fw_opt {
> FW_OPT_UEVENT = BIT(0),
> @@ -41,6 +43,7 @@ enum fw_opt {
> FW_OPT_NOCACHE = BIT(4),
> FW_OPT_NOFALLBACK_SYSFS = BIT(5),
> FW_OPT_FALLBACK_PLATFORM = BIT(6),
> + FW_OPT_PARTIAL = BIT(7),
> };
>
> enum fw_status {
> @@ -68,6 +71,7 @@ struct fw_priv {
> void *data;
> size_t size;
> size_t allocated_size;
> + size_t offset;
> u32 opt_flags;
> #ifdef CONFIG_FW_LOADER_PAGED_BUF
> bool is_paged_buf;
> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> index 814a18cc51bd..7aa22bdc2f60 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -170,10 +170,19 @@ static struct fw_priv *__allocate_fw_priv(const char *fw_name,
> struct firmware_cache *fwc,
> void *dbuf,
> size_t size,
> + size_t offset,
> u32 opt_flags)
> {
> struct fw_priv *fw_priv;
>
> + /* For a partial read, the buffer must be preallocated. */
> + if ((opt_flags & FW_OPT_PARTIAL) && !dbuf)
> + return NULL;
> +
> + /* Only partial reads are allowed to use an offset. */
> + if (offset != 0 && !(opt_flags & FW_OPT_PARTIAL))
> + return NULL;
> +
> fw_priv = kzalloc(sizeof(*fw_priv), GFP_ATOMIC);
> if (!fw_priv)
> return NULL;
> @@ -188,6 +197,7 @@ static struct fw_priv *__allocate_fw_priv(const char *fw_name,
> fw_priv->fwc = fwc;
> fw_priv->data = dbuf;
> fw_priv->allocated_size = size;
> + fw_priv->offset = offset;
> fw_priv->opt_flags = opt_flags;
> fw_state_init(fw_priv);
> #ifdef CONFIG_FW_LOADER_USER_HELPER
> @@ -216,12 +226,17 @@ static int alloc_lookup_fw_priv(const char *fw_name,
> struct fw_priv **fw_priv,
> void *dbuf,
> size_t size,
> + size_t offset,
> u32 opt_flags)
> {
> struct fw_priv *tmp;
>
> spin_lock(&fwc->lock);
> - if (!(opt_flags & FW_OPT_NOCACHE)) {
> + /*
> + * Do not merge requests that are marked to be non-cached or
> + * are performing partial reads.
> + */
> + if (!(opt_flags & (FW_OPT_NOCACHE | FW_OPT_PARTIAL))) {
> tmp = __lookup_fw_priv(fw_name);
> if (tmp) {
> kref_get(&tmp->ref);
> @@ -232,7 +247,7 @@ static int alloc_lookup_fw_priv(const char *fw_name,
> }
> }
>
> - tmp = __allocate_fw_priv(fw_name, fwc, dbuf, size, opt_flags);
> + tmp = __allocate_fw_priv(fw_name, fwc, dbuf, size, offset, opt_flags);
> if (tmp) {
> INIT_LIST_HEAD(&tmp->list);
> if (!(opt_flags & FW_OPT_NOCACHE))
> @@ -439,6 +454,12 @@ static int fw_decompress_xz(struct device *dev, struct fw_priv *fw_priv,
> else
> return fw_decompress_xz_pages(dev, fw_priv, in_size, in_buffer);
> }
> +#else
> +static inline int fw_decompress_xz(struct device *dev, struct fw_priv *fw_priv,
> + size_t in_size, const void *in_buffer)
> +{
> + return -ENOENT;
> +}
> #endif /* CONFIG_FW_LOADER_COMPRESS */
>
> /* direct firmware loading support */
> @@ -485,6 +506,9 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
> return -ENOMEM;
>
> for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
> + size_t file_size = 0;
> + size_t *file_size_ptr = NULL;
> +
> /* skip the unset customized path */
> if (!fw_path[i][0])
> continue;
> @@ -498,9 +522,18 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
>
> fw_priv->size = 0;
>
> + /*
> + * The total file size is only examined when doing a partial
> + * read; the "full read" case needs to fail if the whole
> + * firmware was not completely loaded.
> + */
> + if ((fw_priv->opt_flags & FW_OPT_PARTIAL) && buffer)
> + file_size_ptr = &file_size;
> +
> /* load firmware files from the mount namespace of init */
> - rc = kernel_read_file_from_path_initns(path, 0, &buffer, msize,
> - NULL,
> + rc = kernel_read_file_from_path_initns(path, fw_priv->offset,
> + &buffer, msize,
> + file_size_ptr,
> READING_FIRMWARE);
> if (rc < 0) {
> if (rc != -ENOENT)
> @@ -691,7 +724,7 @@ int assign_fw(struct firmware *fw, struct device *device)
> static int
> _request_firmware_prepare(struct firmware **firmware_p, const char *name,
> struct device *device, void *dbuf, size_t size,
> - u32 opt_flags)
> + size_t offset, u32 opt_flags)
> {
> struct firmware *firmware;
> struct fw_priv *fw_priv;
> @@ -710,7 +743,7 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
> }
>
> ret = alloc_lookup_fw_priv(name, &fw_cache, &fw_priv, dbuf, size,
> - opt_flags);
> + offset, opt_flags);
>
> /*
> * bind with 'priv' now to avoid warning in failure path
> @@ -757,9 +790,10 @@ static void fw_abort_batch_reqs(struct firmware *fw)
> static int
> _request_firmware(const struct firmware **firmware_p, const char *name,
> struct device *device, void *buf, size_t size,
> - u32 opt_flags)
> + size_t offset, u32 opt_flags)
> {
> struct firmware *fw = NULL;
> + bool nondirect = false;
> int ret;
>
> if (!firmware_p)
> @@ -771,18 +805,20 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> }
>
> ret = _request_firmware_prepare(&fw, name, device, buf, size,
> - opt_flags);
> + offset, opt_flags);
> if (ret <= 0) /* error or already assigned */
> goto out;
>
> ret = fw_get_filesystem_firmware(device, fw->priv, "", NULL);
> -#ifdef CONFIG_FW_LOADER_COMPRESS
> - if (ret == -ENOENT)
> +
> + /* Only full reads can support decompression, platform, and sysfs. */
> + if (!(opt_flags & FW_OPT_PARTIAL))
> + nondirect = true;
> +
> + if (ret == -ENOENT && nondirect)
> ret = fw_get_filesystem_firmware(device, fw->priv, ".xz",
> fw_decompress_xz);
> -#endif
> -
> - if (ret == -ENOENT)
> + if (ret == -ENOENT && nondirect)
> ret = firmware_fallback_platform(fw->priv);
>
> if (ret) {
> @@ -790,7 +826,9 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> dev_warn(device,
> "Direct firmware load for %s failed with error %d\n",
> name, ret);
> - ret = firmware_fallback_sysfs(fw, name, device, opt_flags, ret);
> + if (nondirect)
> + ret = firmware_fallback_sysfs(fw, name, device,
> + opt_flags, ret);
> } else
> ret = assign_fw(fw, device);
>
> @@ -833,7 +871,7 @@ request_firmware(const struct firmware **firmware_p, const char *name,
>
> /* Need to pin this module until return */
> __module_get(THIS_MODULE);
> - ret = _request_firmware(firmware_p, name, device, NULL, 0,
> + ret = _request_firmware(firmware_p, name, device, NULL, 0, 0,
> FW_OPT_UEVENT);
> module_put(THIS_MODULE);
> return ret;
> @@ -860,7 +898,7 @@ int firmware_request_nowarn(const struct firmware **firmware, const char *name,
>
> /* Need to pin this module until return */
> __module_get(THIS_MODULE);
> - ret = _request_firmware(firmware, name, device, NULL, 0,
> + ret = _request_firmware(firmware, name, device, NULL, 0, 0,
> FW_OPT_UEVENT | FW_OPT_NO_WARN);
> module_put(THIS_MODULE);
> return ret;
> @@ -884,7 +922,7 @@ int request_firmware_direct(const struct firmware **firmware_p,
> int ret;
>
> __module_get(THIS_MODULE);
> - ret = _request_firmware(firmware_p, name, device, NULL, 0,
> + ret = _request_firmware(firmware_p, name, device, NULL, 0, 0,
> FW_OPT_UEVENT | FW_OPT_NO_WARN |
> FW_OPT_NOFALLBACK_SYSFS);
> module_put(THIS_MODULE);
> @@ -909,7 +947,7 @@ int firmware_request_platform(const struct firmware **firmware,
>
> /* Need to pin this module until return */
> __module_get(THIS_MODULE);
> - ret = _request_firmware(firmware, name, device, NULL, 0,
> + ret = _request_firmware(firmware, name, device, NULL, 0, 0,
> FW_OPT_UEVENT | FW_OPT_FALLBACK_PLATFORM);
> module_put(THIS_MODULE);
> return ret;
> @@ -965,13 +1003,44 @@ request_firmware_into_buf(const struct firmware **firmware_p, const char *name,
> return -EOPNOTSUPP;
>
> __module_get(THIS_MODULE);
> - ret = _request_firmware(firmware_p, name, device, buf, size,
> + ret = _request_firmware(firmware_p, name, device, buf, size, 0,
> FW_OPT_UEVENT | FW_OPT_NOCACHE);
> module_put(THIS_MODULE);
> return ret;
> }
> EXPORT_SYMBOL(request_firmware_into_buf);
>
> +/**
> + * request_partial_firmware_into_buf() - load partial firmware into a previously allocated buffer
> + * @firmware_p: pointer to firmware image
> + * @name: name of firmware file
> + * @device: device for which firmware is being loaded and DMA region allocated
> + * @buf: address of buffer to load firmware into
> + * @size: size of buffer
> + * @offset: offset into file to read
> + *
> + * This function works pretty much like request_firmware_into_buf except
> + * it allows a partial read of the file.
> + */
> +int
> +request_partial_firmware_into_buf(const struct firmware **firmware_p,
> + const char *name, struct device *device,
> + void *buf, size_t size, size_t offset)
> +{
> + int ret;
> +
> + if (fw_cache_is_setup(device, name))
> + return -EOPNOTSUPP;
> +
> + __module_get(THIS_MODULE);
> + ret = _request_firmware(firmware_p, name, device, buf, size, offset,
> + FW_OPT_UEVENT | FW_OPT_NOCACHE |
> + FW_OPT_PARTIAL);
> + module_put(THIS_MODULE);
> + return ret;
> +}
> +EXPORT_SYMBOL(request_partial_firmware_into_buf);
> +
> /**
> * release_firmware() - release the resource associated with a firmware image
> * @fw: firmware resource to release
> @@ -1004,7 +1073,7 @@ static void request_firmware_work_func(struct work_struct *work)
>
> fw_work = container_of(work, struct firmware_work, work);
>
> - _request_firmware(&fw, fw_work->name, fw_work->device, NULL, 0,
> + _request_firmware(&fw, fw_work->name, fw_work->device, NULL, 0, 0,
> fw_work->opt_flags);
> fw_work->cont(fw, fw_work->context);
> put_device(fw_work->device); /* taken in request_firmware_nowait() */
> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> index cb3e2c06ed8a..c15acadc6cf4 100644
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -53,6 +53,9 @@ int request_firmware_direct(const struct firmware **fw, const char *name,
> struct device *device);
> int request_firmware_into_buf(const struct firmware **firmware_p,
> const char *name, struct device *device, void *buf, size_t size);
> +int request_partial_firmware_into_buf(const struct firmware **firmware_p,
> + const char *name, struct device *device,
> + void *buf, size_t size, size_t offset);
>
> void release_firmware(const struct firmware *fw);
> #else
> @@ -102,6 +105,15 @@ static inline int request_firmware_into_buf(const struct firmware **firmware_p,
> return -EINVAL;
> }
>
> +static inline int request_partial_firmware_into_buf
> + (const struct firmware **firmware_p,
> + const char *name,
> + struct device *device,
> + void *buf, size_t size, size_t offset)
> +{
> + return -EINVAL;
> +}
> +
> #endif
>
> int firmware_request_cache(struct device *device, const char *name);
> --
> 2.25.1
>
next prev parent reply other threads:[~2020-07-29 1:17 UTC|newest]
Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-24 21:36 [PATCH v3 00/19] Introduce partial kernel_read_file() support Kees Cook
2020-07-24 21:36 ` [PATCH v3 01/19] test_firmware: Test platform fw loading on non-EFI systems Kees Cook
2020-07-24 21:36 ` [PATCH v3 02/19] selftest/firmware: Add selftest timeout in settings Kees Cook
2020-07-24 21:36 ` [PATCH v3 03/19] firmware_loader: EFI firmware loader must handle pre-allocated buffer Kees Cook
2020-07-25 10:07 ` Greg Kroah-Hartman
2020-07-25 15:50 ` Kees Cook
2020-07-25 17:20 ` Greg Kroah-Hartman
2020-07-24 21:36 ` [PATCH v3 04/19] fs/kernel_read_file: Remove FIRMWARE_PREALLOC_BUFFER enum Kees Cook
2020-07-27 13:35 ` Mimi Zohar
2020-07-24 21:36 ` [PATCH v3 05/19] fs/kernel_read_file: Remove FIRMWARE_EFI_EMBEDDED enum Kees Cook
2020-07-24 21:36 ` [PATCH v3 06/19] fs/kernel_read_file: Split into separate include file Kees Cook
2020-07-27 14:41 ` Mimi Zohar
2020-07-24 21:36 ` [PATCH v3 07/19] fs/kernel_read_file: Split into separate source file Kees Cook
2020-07-27 14:53 ` Mimi Zohar
2020-07-24 21:36 ` [PATCH v3 08/19] fs/kernel_read_file: Remove redundant size argument Kees Cook
2020-07-27 16:29 ` Mimi Zohar
2020-07-24 21:36 ` [PATCH v3 09/19] fs/kernel_read_file: Switch buffer size arg to size_t Kees Cook
2020-07-27 16:29 ` Mimi Zohar
2020-07-24 21:36 ` [PATCH v3 10/19] fs/kernel_read_file: Add file_size output argument Kees Cook
2020-07-27 16:29 ` Mimi Zohar
2020-07-24 21:36 ` [PATCH v3 11/19] LSM: Introduce kernel_post_load_data() hook Kees Cook
2020-07-27 10:49 ` Mimi Zohar
2020-07-28 19:41 ` Kees Cook
2020-07-24 21:36 ` [PATCH v3 12/19] firmware_loader: Use security_post_load_data() Kees Cook
2020-07-27 10:57 ` Mimi Zohar
2020-07-28 19:43 ` Kees Cook
2020-07-29 16:29 ` Mimi Zohar
2020-07-29 18:10 ` Mimi Zohar
2020-07-29 19:13 ` Kees Cook
2020-07-24 21:36 ` [PATCH v3 13/19] module: Call security_kernel_post_load_data() Kees Cook
2020-07-24 21:36 ` [PATCH v3 14/19] LSM: Add "contents" flag to kernel_read_file hook Kees Cook
2020-07-24 21:36 ` [PATCH v3 15/19] IMA: Add support for file reads without contents Kees Cook
2020-07-27 13:23 ` Mimi Zohar
2020-07-28 19:44 ` Kees Cook
2020-07-28 19:56 ` Greg Kroah-Hartman
2020-07-28 20:12 ` Kees Cook
2020-07-24 21:36 ` [PATCH v3 16/19] fs/kernel_file_read: Add "offset" arg for partial reads Kees Cook
2020-07-24 21:36 ` [PATCH v3 17/19] firmware: Store opt_flags in fw_priv Kees Cook
2020-07-24 21:36 ` [PATCH v3 18/19] firmware: Add request_partial_firmware_into_buf() Kees Cook
2020-07-29 1:17 ` Luis Chamberlain [this message]
2020-07-29 6:22 ` Takashi Iwai
2020-07-29 17:43 ` Kees Cook
2020-07-24 21:36 ` [PATCH v3 19/19] test_firmware: Test partial read support Kees Cook
2020-07-25 5:14 ` [PATCH v3 00/19] Introduce partial kernel_read_file() support Scott Branden
2020-07-25 10:05 ` Greg Kroah-Hartman
2020-07-25 15:48 ` Kees Cook
2020-07-27 11:16 ` Mimi Zohar
2020-07-27 19:18 ` Scott Branden
2020-07-28 18:48 ` Mimi Zohar
2020-07-28 19:56 ` Scott Branden
2020-07-29 1:19 ` Luis Chamberlain
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200729011739.GL4332@42.do-not-panic.com \
--to=mcgrof@kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=jeyu@kernel.org \
--cc=keescook@chromium.org \
--cc=kpsingh@chromium.org \
--cc=linux-efi@vger.kernel.org \
--cc=linux-integrity@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=scott.branden@broadcom.com \
--cc=selinux@vger.kernel.org \
--cc=sjpark@amazon.de \
--cc=tiwai@suse.de \
--cc=zohar@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).