| 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: <ZGbmfsU6vc6uZ+E+@aschofie-mobl2>
Date: Thu, 18 May 2023 20:01:18 -0700
From: Alison Schofield <alison.schofield@...el.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: Vishal Verma <vishal.l.verma@...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 4/4] tools/testing/cxl: add firmware update emulation to
CXL memdevs
On Thu, May 11, 2023 at 05:18:16PM +0100, Jonathan Cameron wrote:
> On Fri, 21 Apr 2023 21:09:28 -0600
> Vishal Verma <vishal.l.verma@...el.com> wrote:
>
> > Add emulation for the 'Get FW Info', 'Transfer FW', and 'Activate FW'
> > CXL mailbox commands to the cxl_test emulated memdevs to enable
> > end-to-end unit testing of a firmware update flow. For now, only
> > advertise an 'offline activation' capability as that is all the CXL
> > memdev driver currently implements.
> >
> > Add some canned values for the serial number fields, and create a
> > platform device sysfs knob to calculate the sha256sum of the firmware
> > image that was received, so a unit test can compare it with the original
> > file that was uploaded.
> >
> > Signed-off-by: Vishal Verma <vishal.l.verma@...el.com>
> Hi Vishal,
>
> A few trivial comments inline,
>
> Thanks,
>
> Jonathan
>
> > ---
> > tools/testing/cxl/test/mem.c | 191 +++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 191 insertions(+)
> >
> > diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> > index 9263b04d35f7..bc99cc673550 100644
> > --- a/tools/testing/cxl/test/mem.c
> > +++ b/tools/testing/cxl/test/mem.c
> > @@ -7,11 +7,14 @@
> > #include <linux/delay.h>
> > #include <linux/sizes.h>
> > #include <linux/bits.h>
> > +#include <crypto/hash.h>
> > #include <cxlmem.h>
> >
> > #include "trace.h"
> >
> > #define LSA_SIZE SZ_128K
> > +#define FW_SIZE SZ_64M
> > +#define FW_SLOTS 3
> > #define DEV_SIZE SZ_2G
> > #define EFFECT(x) (1U << x)
> >
> > @@ -40,6 +43,18 @@ static struct cxl_cel_entry mock_cel[] = {
> > .opcode = cpu_to_le16(CXL_MBOX_OP_GET_HEALTH_INFO),
> > .effect = cpu_to_le16(0),
> > },
> > + {
> > + .opcode = cpu_to_le16(CXL_MBOX_OP_GET_FW_INFO),
> > + .effect = cpu_to_le16(0),
> > + },
> > + {
> > + .opcode = cpu_to_le16(CXL_MBOX_OP_TRANSFER_FW),
> > + .effect = cpu_to_le16(EFFECT(0) | EFFECT(6)),
>
> Beginning to feel like some defines for each effect might be worth
> adding.
>
> > + },
> > + {
> > + .opcode = cpu_to_le16(CXL_MBOX_OP_ACTIVATE_FW),
> > + .effect = cpu_to_le16(EFFECT(0) | EFFECT(1)),
> > + },
> > };
>
> ...
>
> > +static int mock_transfer_fw(struct cxl_dev_state *cxlds,
> > + struct cxl_mbox_cmd *cmd)
> > +{
> > + struct cxl_mbox_transfer_fw *transfer = cmd->payload_in;
> > + struct cxl_mockmem_data *mdata = dev_get_drvdata(cxlds->dev);
> > + void *fw = mdata->fw;
> > + size_t offset, length;
> > +
> > + offset = le32_to_cpu(transfer->offset) * CXL_FW_TRANSFER_OFFSET_ALIGN;
> > + length = cmd->size_in - sizeof(*transfer);
> > + if (offset + length > FW_SIZE)
> > + return -EINVAL;
> > +
> > + switch (transfer->action) {
> > + case CXL_FW_TRANSFER_ACTION_FULL:
> > + if (offset != 0)
> > + return -EINVAL;
> > + fallthrough;
> > + case CXL_FW_TRANSFER_ACTION_END:
> > + if (transfer->slot == 0 || transfer->slot > FW_SLOTS)
> > + return -EINVAL;
> > + mdata->fw_size = offset + length;
> > + break;
> > + case CXL_FW_TRANSFER_ACTION_START:
> > + case CXL_FW_TRANSFER_ACTION_CONTINUE:
> > + case CXL_FW_TRANSFER_ACTION_ABORT:
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + memcpy(fw + offset, &transfer->data[0], length);
>
> Slight preference for transfer->data
>
What's the story behind that Jonathan?
I imagined kernel developers leaned towards the explicit.
Alison
> > + return 0;
> > +}
> > +
>
> ...
>
> > +static int do_sha256(u8 *data, unsigned int length, u8 *hash)
>
> Can't use the one in include/crypto/sha2.h? Don't think anyone really
> cares about extreme performance here.
>
> > +{
> > + struct crypto_shash *alg;
> > + struct sdesc *sdesc;
> > + size_t size;
> > + int rc;
> > +
> > + alg = crypto_alloc_shash("sha256", 0, 0);
> > + if (IS_ERR(alg))
> > + return PTR_ERR(alg);
> > +
> > + size = sizeof(struct shash_desc) + crypto_shash_descsize(alg);
> > + sdesc = kzalloc(size, GFP_KERNEL);
> > + if (!sdesc) {
> > + rc = -ENOMEM;
> > + goto out_shash;
> > + }
> > +
> > + sdesc->shash.tfm = alg;
> > + rc = crypto_shash_digest(&sdesc->shash, data, length, hash);
> > +
> > + kfree(sdesc);
> > +out_shash:
> > + crypto_free_shash(alg);
> > + return rc;
> > +}
> > +
> > +#define CHECKSUM_SIZE 32
> > +
> > +static ssize_t fw_buf_checksum_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct cxl_mockmem_data *mdata = dev_get_drvdata(dev);
> > + unsigned char *hstr, *hptr;
> > + u8 hash[CHECKSUM_SIZE];
> > + ssize_t written = 0;
> > + int i, rc;
> > +
> > + rc = do_sha256(mdata->fw, mdata->fw_size, &hash[0]);
> > + if (rc) {
> > + dev_err(dev, "error calculating checksum: %d\n", rc);
> > + goto out_free;
> > + }
> > +
> > + hstr = kzalloc((CHECKSUM_SIZE * 2) + 1, GFP_KERNEL);
> > + if (!hstr)
> > + return -ENOMEM;
> > +
> > + hptr = hstr;
> > + for (i = 0; i < CHECKSUM_SIZE; i++)
> > + hptr += sprintf(hptr, "%02x", hash[i]);
> > +
> > + written = sysfs_emit(buf, "%s\n", hstr);
> > +
> > +out_free:
> > + kfree(hstr);
> > + return written;
> > +}
> > +
> > +static DEVICE_ATTR_RO(fw_buf_checksum);
> > +
> > static struct attribute *cxl_mock_mem_attrs[] = {
> > &dev_attr_security_lock.attr,
> > &dev_attr_event_trigger.attr,
> > + &dev_attr_fw_buf_checksum.attr,
> > NULL
> > };
> > ATTRIBUTE_GROUPS(cxl_mock_mem);
> >
>
Powered by blists - more mailing lists