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: 55+ 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-26 3:00 ` kernel test robot
2020-07-27 21:24 ` Sasha Levin
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-27 21:24 ` Sasha Levin
2020-07-24 21:36 ` [PATCH v3 05/19] fs/kernel_read_file: Remove FIRMWARE_EFI_EMBEDDED enum Kees Cook
2020-07-27 21:24 ` Sasha Levin
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.