| 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: <20230614151417.00002f46@Huawei.com>
Date: Wed, 14 Jun 2023 15:14:17 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Vishal Verma <vishal.l.verma@...el.com>
CC: Alison Schofield <alison.schofield@...el.com>,
Ira Weiny <ira.weiny@...el.com>,
Dave Jiang <dave.jiang@...el.com>,
Ben Widawsky <bwidawsk@...nel.org>,
Dan Williams <dan.j.williams@...el.com>,
<linux-cxl@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
Davidlohr Bueso <dave@...olabs.net>,
Russ Weight <russell.h.weight@...el.com>
Subject: Re: [PATCH v3 1/4] cxl: add a firmware update mechanism using the
sysfs firmware loader
On Mon, 12 Jun 2023 00:55:56 -0600
Vishal Verma <vishal.l.verma@...el.com> wrote:
> The sysfs based firmware loader mechanism was created to easily allow
> userspace to upload firmware images to FPGA cards. This also happens to
> be pretty suitable to create a user-initiated but kernel-controlled
> firmware update mechanism for CXL devices, using the CXL specified
> mailbox commands.
>
> Since firmware update commands can be long-running, and can be processed
> in the background by the endpoint device, it is desirable to have the
> ability to chunk the firmware transfer down to smaller pieces, so that
> one operation does not monopolize the mailbox, locking out any other
> long running background commands entirely - e.g. security commands like
> 'sanitize' or poison scanning operations.
>
> The firmware loader mechanism allows a natural way to perform this
> chunking, as after each mailbox command, that is restricted to the
> maximum mailbox payload size, the cxl memdev driver relinquishes control
> back to the fw_loader system and awaits the next chunk of data to
> transfer. This opens opportunities for other background commands to
> access the mailbox and send their own slices of background commands.
>
> Add the necessary helpers and state tracking to be able to perform the
> 'Get FW Info', 'Transfer FW', and 'Activate FW' mailbox commands as
> described in the CXL spec. Wire these up to the firmware loader
> callbacks, and register with that system to create the memX/firmware/
> sysfs ABI.
>
> Cc: Davidlohr Bueso <dave@...olabs.net>
> Cc: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> Cc: Russ Weight <russell.h.weight@...el.com>
> Cc: Alison Schofield <alison.schofield@...el.com>
> Cc: Ira Weiny <ira.weiny@...el.com>
> Cc: Dave Jiang <dave.jiang@...el.com>
> Cc: Ben Widawsky <bwidawsk@...nel.org>
> Cc: Dan Williams <dan.j.williams@...el.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@...el.com>
One issue and one trivial comment inline.
With those fixed up and subject to the fact I haven't looked at the firmware
class before...
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 057a43267290..3234885ad804 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -1,6 +1,7 @@
> +
> +static enum fw_upload_err cxl_fw_write(struct fw_upload *fwl, const u8 *data,
> + u32 offset, u32 size, u32 *written)
> +{
> + struct cxl_dev_state *cxlds = fwl->dd_handle;
> + struct cxl_memdev *cxlmd = cxlds->cxlmd;
> + struct cxl_mbox_transfer_fw *transfer;
> + struct cxl_mbox_cmd mbox_cmd;
> + u32 cur_size, remaining;
> + size_t size_in;
> + int rc;
> +
> + *written = 0;
> +
> + /* Offset has to be aligned to 128B (CXL-3.0 8.2.9.3.2 Table 8-57) */
> + if (!IS_ALIGNED(offset, CXL_FW_TRANSFER_ALIGNMENT)) {
> + dev_err(&cxlmd->dev,
> + "misaligned offset for FW transfer slice (%u)\n",
> + offset);
> + return FW_UPLOAD_ERR_RW_ERROR;
> + }
> +
> + /*
> + * Pick transfer size based on cxlds->payload_size
> + * @size must bw 128-byte aligned, ->payload_size is a power of 2
> + * starting at 256 bytes, and sizeof(*transfer) is 128.
> + * These constraints imply that @cur_size will always be 128b aligned.
> + */
> + cur_size = min_t(size_t, size, cxlds->payload_size - sizeof(*transfer));
> +
> + remaining = size - cur_size;
> + size_in = struct_size(transfer, data, cur_size);
> +
> + if (test_and_clear_bit(CXL_FW_CANCEL, cxlds->fw.state))
> + return cxl_fw_do_cancel(fwl);
> +
> + /*
> + * Slot numbers are 1-indexed
> + * cur_slot is the 0-indexed next_slot (i.e. 'cur_slot - 1 + 1')
> + * Check for rollover using modulo, and 1-index it by adding 1
> + */
> + cxlds->fw.next_slot = (cxlds->fw.cur_slot % cxlds->fw.num_slots) + 1;
> +
> + /* Do the transfer via mailbox cmd */
> + transfer = kzalloc(size_in, GFP_KERNEL);
> + if (!transfer)
> + return FW_UPLOAD_ERR_RW_ERROR;
> +
> + transfer->offset = cpu_to_le32(offset / CXL_FW_TRANSFER_ALIGNMENT);
> + memcpy(transfer->data, data + offset, cur_size);
> + if (cxlds->fw.oneshot) {
> + transfer->action = CXL_FW_TRANSFER_ACTION_FULL;
> + transfer->slot = cxlds->fw.next_slot;
> + } else {
> + if (offset == 0) {
> + transfer->action = CXL_FW_TRANSFER_ACTION_INITIATE;
> + } else if (remaining == 0) {
> + transfer->action = CXL_FW_TRANSFER_ACTION_END;
> + transfer->slot = cxlds->fw.next_slot;
> + } else {
> + transfer->action = CXL_FW_TRANSFER_ACTION_CONTINUE;
> + }
> + }
> +
> + mbox_cmd = (struct cxl_mbox_cmd) {
> + .opcode = CXL_MBOX_OP_TRANSFER_FW,
> + .size_in = size_in,
> + .payload_in = transfer,
> + .poll_interval_ms = 1000,
> + .poll_count = 30,
> + };
> +
> + rc = cxl_internal_send_cmd(cxlds, &mbox_cmd);
> + if (rc < 0) {
> + kfree(transfer);
Double free. Drop this one.
> + rc = FW_UPLOAD_ERR_RW_ERROR;
> + goto out_free;
> + }
> +
> + *written = cur_size;
> +
> + /* Activate FW if oneshot or if the last slice was written */
> + if (cxlds->fw.oneshot || remaining == 0) {
> + dev_dbg(&cxlmd->dev, "Activating firmware slot: %d\n",
> + cxlds->fw.next_slot);
> + rc = cxl_mem_activate_fw(cxlds, cxlds->fw.next_slot);
> + if (rc < 0) {
> + dev_err(&cxlmd->dev, "Error activating firmware: %d\n",
> + rc);
> + rc = FW_UPLOAD_ERR_HW_ERROR;
> + goto out_free;
> + }
> + }
> +
> + rc = FW_UPLOAD_ERR_NONE;
> +
> +out_free:
> + kfree(transfer);
> + return rc;
> +}
> static const struct file_operations cxl_memdev_fops = {
> .owner = THIS_MODULE,
> .unlocked_ioctl = cxl_memdev_ioctl,
> @@ -582,6 +891,7 @@ struct cxl_memdev *devm_cxl_add_memdev(struct cxl_dev_state *cxlds)
> rc = devm_add_action_or_reset(cxlds->dev, cxl_memdev_unregister, cxlmd);
> if (rc)
> return ERR_PTR(rc);
> +
What is this stray white space change I see before me? :)
> return cxlmd;
>
> err:
Powered by blists - more mailing lists