| 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: <Y5iQyefMnpxJsBFw@yilunxu-OptiPlex-7050>
Date: Tue, 13 Dec 2022 22:48:41 +0800
From: Xu Yilun <yilun.xu@...el.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: linux-fpga@...r.kernel.org, Wu Hao <hao.wu@...el.com>,
Tom Rix <trix@...hat.com>, Moritz Fischer <mdf@...nel.org>,
Lee Jones <lee@...nel.org>,
Matthew Gerlach <matthew.gerlach@...ux.intel.com>,
Russ Weight <russell.h.weight@...el.com>,
Tianfei zhang <tianfei.zhang@...el.com>,
Mark Brown <broonie@...nel.org>,
Greg KH <gregkh@...uxfoundation.org>,
Marco Pagani <marpagan@...hat.com>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 8/8] fpga: m10bmc-sec: Add support for N6000
On 2022-12-13 at 12:18:50 +0200, Ilpo Järvinen wrote:
> On Tue, 13 Dec 2022, Xu Yilun wrote:
>
> > On 2022-12-11 at 12:39:13 +0200, Ilpo Järvinen wrote:
> > > Add support for PMCI-based flash access path and N6000 sec update
> > > support. Access to flash staging area is different for N6000 from that
> > > of the SPI interfaced counterparts.
> > >
> > > Introduce intel_m10bmc_flash_bulk_ops to allow interface specific
> > > differentiations for the flash access path for sec update and make
> > > m10bmc_sec_read/write() in sec update driver to use the new operations.
> > >
> > > Co-developed-by: Tianfei zhang <tianfei.zhang@...el.com>
> > > Signed-off-by: Tianfei zhang <tianfei.zhang@...el.com>
> > > Co-developed-by: Russ Weight <russell.h.weight@...el.com>
> > > Signed-off-by: Russ Weight <russell.h.weight@...el.com>
> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> > > ---
> > > drivers/fpga/intel-m10-bmc-sec-update.c | 65 ++++++++++-
> > > drivers/mfd/intel-m10-bmc-pmci.c | 145 ++++++++++++++++++++++++
> > > include/linux/mfd/intel-m10-bmc.h | 14 +++
> > > 3 files changed, 223 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/fpga/intel-m10-bmc-sec-update.c b/drivers/fpga/intel-m10-bmc-sec-update.c
> > > index 9922027856a4..885e38f13897 100644
> > > --- a/drivers/fpga/intel-m10-bmc-sec-update.c
> > > +++ b/drivers/fpga/intel-m10-bmc-sec-update.c
> > > @@ -14,6 +14,20 @@
> > > #include <linux/platform_device.h>
> > > #include <linux/slab.h>
> > >
> > > +#define M10BMC_PMCI_FLASH_MUX_CTRL 0x1d0
> > > +#define FLASH_MUX_SELECTION GENMASK(2, 0)
> > > +#define FLASH_MUX_IDLE 0
> > > +#define FLASH_MUX_NIOS 1
> > > +#define FLASH_MUX_HOST 2
> > > +#define FLASH_MUX_PFL 4
> > > +#define get_flash_mux(mux) FIELD_GET(FLASH_MUX_SELECTION, mux)
> > > +
> > > +#define FLASH_NIOS_REQUEST BIT(4)
> > > +#define FLASH_HOST_REQUEST BIT(5)
> > > +
> > > +#define M10_FLASH_INT_US 1
> > > +#define M10_FLASH_TIMEOUT_US 10000
> > > +
> > > struct m10bmc_sec {
> > > struct device *dev;
> > > struct intel_m10bmc *m10bmc;
> > > @@ -21,6 +35,7 @@ struct m10bmc_sec {
> > > char *fw_name;
> > > u32 fw_name_id;
> > > bool cancel_request;
> > > + struct mutex flash_mutex;
> > > };
> > >
> > > static DEFINE_XARRAY_ALLOC(fw_upload_xa);
> > > @@ -31,6 +46,24 @@ static DEFINE_XARRAY_ALLOC(fw_upload_xa);
> > > #define REH_MAGIC GENMASK(15, 0)
> > > #define REH_SHA_NUM_BYTES GENMASK(31, 16)
> > >
> > > +static int m10bmc_sec_set_flash_host_mux(struct intel_m10bmc *m10bmc, bool request)
> > > +{
> > > + u32 ctrl;
> > > + int ret;
> > > +
> > > + ret = regmap_update_bits(m10bmc->regmap, M10BMC_PMCI_FLASH_MUX_CTRL,
> > > + FLASH_HOST_REQUEST,
> > > + FIELD_PREP(FLASH_HOST_REQUEST, request));
> > > + if (ret)
> > > + return ret;
> > > +
> > > + return regmap_read_poll_timeout(m10bmc->regmap,
> > > + M10BMC_PMCI_FLASH_MUX_CTRL, ctrl,
> > > + request ? (get_flash_mux(ctrl) == FLASH_MUX_HOST) :
> > > + (get_flash_mux(ctrl) != FLASH_MUX_HOST),
> > > + M10_FLASH_INT_US, M10_FLASH_TIMEOUT_US);
> > > +}
> > > +
> > > static int m10bmc_sec_write(struct m10bmc_sec *sec, const u8 *buf, u32 offset, u32 size)
> > > {
> > > struct intel_m10bmc *m10bmc = sec->m10bmc;
> > > @@ -41,6 +74,15 @@ static int m10bmc_sec_write(struct m10bmc_sec *sec, const u8 *buf, u32 offset, u
> > > u32 leftover_tmp = 0;
> > > int ret;
> > >
> > > + if (sec->m10bmc->flash_bulk_ops) {
> > > + mutex_lock(&sec->flash_mutex);
> > > + /* On write, firmware manages flash MUX */
> > > + ret = sec->m10bmc->flash_bulk_ops->write(m10bmc, buf, offset, size);
> > > + mutex_unlock(&sec->flash_mutex);
> > > +
> > > + return ret;
> > > + }
> > > +
> > > if (WARN_ON_ONCE(stride > sizeof(leftover_tmp)))
> > > return -EINVAL;
> > >
> > > @@ -69,7 +111,21 @@ static int m10bmc_sec_read(struct m10bmc_sec *sec, u8 *buf, u32 addr, u32 size)
> > > u32 leftover_offset = read_count * stride;
> > > u32 leftover_size = size - leftover_offset;
> > > u32 leftover_tmp;
> > > - int ret;
> > > + int ret, ret2;
> > > +
> > > + if (sec->m10bmc->flash_bulk_ops) {
> > > + mutex_lock(&sec->flash_mutex);
> > > + ret = m10bmc_sec_set_flash_host_mux(m10bmc, true);
> > > + if (ret)
> > > + goto mux_fail;
> >
> > If the flash host mux fail, we still need to un-mux it?
>
> It seemed safer to attempt to set it back after the code tried to alter
> the MUX setting. I don't see how it could be harmful. Likely we're in the
> deep end in that case anyway so setting it back might just fails too
> (which is harmless sans the small extra delay) or just confirms that the
> value wasn't changed.
Fine. It's good to me.
>
> --
> i.
>
> > > + ret = sec->m10bmc->flash_bulk_ops->read(m10bmc, buf, addr, size);
> > > +mux_fail:
> > > + ret2 = m10bmc_sec_set_flash_host_mux(m10bmc, false);
> > > + mutex_unlock(&sec->flash_mutex);
> > > + if (ret)
> > > + return ret;
> > > + return ret2;
> > > + }
> > >
> > > if (WARN_ON_ONCE(stride > sizeof(leftover_tmp)))
> > > return -EINVAL;
> > > @@ -611,6 +667,8 @@ static int m10bmc_sec_probe(struct platform_device *pdev)
> > > if (ret)
> > > return ret;
> > >
> > > + mutex_init(&sec->flash_mutex);
> > > +
> > > len = scnprintf(buf, SEC_UPDATE_LEN_MAX, "secure-update%d",
> > > sec->fw_name_id);
> > > sec->fw_name = kmemdup_nul(buf, len, GFP_KERNEL);
> > > @@ -633,6 +691,7 @@ static int m10bmc_sec_probe(struct platform_device *pdev)
> > > fw_uploader_fail:
> > > kfree(sec->fw_name);
> > > fw_name_fail:
> > > + mutex_destroy(&sec->flash_mutex);
> > > xa_erase(&fw_upload_xa, sec->fw_name_id);
> > > return ret;
> > > }
> >
Powered by blists - more mailing lists