| 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
| ||
|
Date: Wed, 19 Jun 2024 01:16:38 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: "Gaurav Kashyap (QUIC)" <quic_gaurkash@...cinc.com>
Cc: "linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
"linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>, "andersson@...nel.org" <andersson@...nel.org>,
"ebiggers@...gle.com" <ebiggers@...gle.com>,
"neil.armstrong@...aro.org" <neil.armstrong@...aro.org>,
"srinivas.kandagatla" <srinivas.kandagatla@...aro.org>,
"krzysztof.kozlowski+dt@...aro.org" <krzysztof.kozlowski+dt@...aro.org>,
"conor+dt@...nel.org" <conor+dt@...nel.org>, "robh+dt@...nel.org" <robh+dt@...nel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>, kernel <kernel@...cinc.com>,
"linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"Om Prakash Singh (QUIC)" <quic_omprsing@...cinc.com>,
"Bao D. Nguyen (QUIC)" <quic_nguyenb@...cinc.com>,
"bartosz.golaszewski" <bartosz.golaszewski@...aro.org>,
"konrad.dybcio@...aro.org" <konrad.dybcio@...aro.org>,
"ulf.hansson@...aro.org" <ulf.hansson@...aro.org>, "jejb@...ux.ibm.com" <jejb@...ux.ibm.com>,
"martin.petersen@...cle.com" <martin.petersen@...cle.com>, "mani@...nel.org" <mani@...nel.org>,
"davem@...emloft.net" <davem@...emloft.net>,
"herbert@...dor.apana.org.au" <herbert@...dor.apana.org.au>, Prasad Sodagudi <psodagud@...cinc.com>,
Sonal Gupta <sonalg@...cinc.com>
Subject: Re: [PATCH v5 04/15] soc: qcom: ice: add hwkm support in ice
On Wed, 19 Jun 2024 at 01:07, Gaurav Kashyap (QUIC)
<quic_gaurkash@...cinc.com> wrote:
>
> Hello Dmitry,
>
> On 06/17/2024 12:55 AM PDT, Dmitry Baryshkov wrote:
> > On Sun, Jun 16, 2024 at 05:50:59PM GMT, Gaurav Kashyap wrote:
> > > Qualcomm's ICE (Inline Crypto Engine) contains a proprietary key
> > > management hardware called Hardware Key Manager (HWKM).
> > > This patch integrates HWKM support in ICE when it is available. HWKM
> > > primarily provides hardware wrapped key support where the ICE
> > > (storage) keys are not available in software and protected in
> > > hardware.
> > >
> > > When HWKM software support is not fully available (from Trustzone),
> > > there can be a scenario where the ICE hardware supports HWKM, but it
> > > cannot be used for wrapped keys. In this case, standard keys have to
> > > be used without using HWKM. Hence, providing a toggle controlled by a
> > > devicetree entry to use HWKM or not.
> > >
> > > Tested-by: Neil Armstrong <neil.armstrong@...aro.org>
> > > Signed-off-by: Gaurav Kashyap <quic_gaurkash@...cinc.com>
> > > ---
> > > drivers/soc/qcom/ice.c | 153
> > +++++++++++++++++++++++++++++++++++++++--
> > > include/soc/qcom/ice.h | 1 +
> > > 2 files changed, 150 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c index
> > > 6f941d32fffb..d5e74cf2946b 100644
> > > --- a/drivers/soc/qcom/ice.c
> > > +++ b/drivers/soc/qcom/ice.c
> > > @@ -26,6 +26,40 @@
> > > #define QCOM_ICE_REG_FUSE_SETTING 0x0010
> > > #define QCOM_ICE_REG_BIST_STATUS 0x0070
> > > #define QCOM_ICE_REG_ADVANCED_CONTROL 0x1000
> > > +#define QCOM_ICE_REG_CONTROL 0x0
> > > +/* QCOM ICE HWKM registers */
> > > +#define QCOM_ICE_REG_HWKM_TZ_KM_CTL 0x1000
> > > +#define QCOM_ICE_REG_HWKM_TZ_KM_STATUS 0x1004
> > > +#define QCOM_ICE_REG_HWKM_BANK0_BANKN_IRQ_STATUS 0x2008
> > > +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_0 0x5000
> > > +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_1 0x5004
> > > +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_2 0x5008
> > > +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_3 0x500C
> > > +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_4 0x5010
> > > +
> > > +/* QCOM ICE HWKM reg vals */
> > > +#define QCOM_ICE_HWKM_BIST_DONE_V1 BIT(16)
> > > +#define QCOM_ICE_HWKM_BIST_DONE_V2 BIT(9)
> > > +#define QCOM_ICE_HWKM_BIST_DONE(ver)
> > QCOM_ICE_HWKM_BIST_DONE_V##ver
> > > +
> > > +#define QCOM_ICE_HWKM_CRYPTO_BIST_DONE_V1 BIT(14)
> > > +#define QCOM_ICE_HWKM_CRYPTO_BIST_DONE_V2 BIT(7)
> > > +#define QCOM_ICE_HWKM_CRYPTO_BIST_DONE(v)
> > QCOM_ICE_HWKM_CRYPTO_BIST_DONE_V##v
> > > +
> > > +#define QCOM_ICE_HWKM_BOOT_CMD_LIST1_DONE BIT(2)
> > > +#define QCOM_ICE_HWKM_BOOT_CMD_LIST0_DONE BIT(1)
> > > +#define QCOM_ICE_HWKM_KT_CLEAR_DONE BIT(0)
> > > +
> > > +#define QCOM_ICE_HWKM_BIST_VAL(v)
> > (QCOM_ICE_HWKM_BIST_DONE(v) | \
> > > + QCOM_ICE_HWKM_CRYPTO_BIST_DONE(v) | \
> > > + QCOM_ICE_HWKM_BOOT_CMD_LIST1_DONE | \
> > > + QCOM_ICE_HWKM_BOOT_CMD_LIST0_DONE | \
> > > + QCOM_ICE_HWKM_KT_CLEAR_DONE)
> > > +
> > > +#define QCOM_ICE_HWKM_V1_STANDARD_MODE_VAL (BIT(0) | BIT(1)
> > | BIT(2))
> > > +#define QCOM_ICE_HWKM_V2_STANDARD_MODE_MASK
> > GENMASK(31, 1) #define
> > > +QCOM_ICE_HWKM_DISABLE_CRC_CHECKS_VAL (BIT(1) | BIT(2))
> > > +#define QCOM_ICE_HWKM_RSP_FIFO_CLEAR_VAL BIT(3)
> > >
> > > /* BIST ("built-in self-test") status flags */
> > > #define QCOM_ICE_BIST_STATUS_MASK GENMASK(31, 28)
> > > @@ -34,6 +68,9 @@
> > > #define QCOM_ICE_FORCE_HW_KEY0_SETTING_MASK 0x2 #define
> > > QCOM_ICE_FORCE_HW_KEY1_SETTING_MASK 0x4
> > >
> > > +#define QCOM_ICE_HWKM_REG_OFFSET 0x8000
> > > +#define HWKM_OFFSET(reg) ((reg) +
> > QCOM_ICE_HWKM_REG_OFFSET)
> > > +
> > > #define qcom_ice_writel(engine, val, reg) \
> > > writel((val), (engine)->base + (reg))
> > >
> > > @@ -46,6 +83,9 @@ struct qcom_ice {
> > > struct device_link *link;
> > >
> > > struct clk *core_clk;
> > > + u8 hwkm_version;
> > > + bool use_hwkm;
> > > + bool hwkm_init_complete;
> > > };
> > >
> > > static bool qcom_ice_check_supported(struct qcom_ice *ice) @@ -63,8
> > > +103,21 @@ static bool qcom_ice_check_supported(struct qcom_ice *ice)
> > > return false;
> > > }
> > >
> > > - dev_info(dev, "Found QC Inline Crypto Engine (ICE) v%d.%d.%d\n",
> > > - major, minor, step);
> > > + if (major >= 4 || (major == 3 && minor == 2 && step >= 1))
> > > + ice->hwkm_version = 2;
> > > + else if (major == 3 && minor == 2)
> > > + ice->hwkm_version = 1;
> > > + else
> > > + ice->hwkm_version = 0;
> > > +
> > > + if (ice->hwkm_version == 0)
> > > + ice->use_hwkm = false;
> > > +
> > > + dev_info(dev, "Found QC Inline Crypto Engine (ICE) v%d.%d.%d,
> > HWKM v%d\n",
> > > + major, minor, step, ice->hwkm_version);
> > > +
> > > + if (!ice->use_hwkm)
> > > + dev_info(dev, "QC ICE HWKM (Hardware Key Manager) not
> > > + used/supported");
> > >
> > > /* If fuses are blown, ICE might not work in the standard way. */
> > > regval = qcom_ice_readl(ice, QCOM_ICE_REG_FUSE_SETTING); @@
> > > -113,27 +166,106 @@ static void qcom_ice_optimization_enable(struct
> > qcom_ice *ice)
> > > * fails, so we needn't do it in software too, and (c) properly testing
> > > * storage encryption requires testing the full storage stack anyway,
> > > * and not relying on hardware-level self-tests.
> > > + *
> > > + * However, we still care about if HWKM BIST failed (when supported)
> > > + as
> > > + * important functionality would fail later, so disable hwkm on failure.
> > > */
> > > static int qcom_ice_wait_bist_status(struct qcom_ice *ice) {
> > > u32 regval;
> > > + u32 bist_done_val;
> > > int err;
> > >
> > > err = readl_poll_timeout(ice->base + QCOM_ICE_REG_BIST_STATUS,
> > > regval, !(regval & QCOM_ICE_BIST_STATUS_MASK),
> > > 50, 5000);
> > > - if (err)
> > > + if (err) {
> > > dev_err(ice->dev, "Timed out waiting for ICE self-test
> > > to complete\n");
> > > + return err;
> > > + }
> > >
> > > + if (ice->use_hwkm) {
> > > + bist_done_val = ice->hwkm_version == 1 ?
> > > + QCOM_ICE_HWKM_BIST_VAL(1) :
> > > + QCOM_ICE_HWKM_BIST_VAL(2);
> > > + if (qcom_ice_readl(ice,
> > > +
> > HWKM_OFFSET(QCOM_ICE_REG_HWKM_TZ_KM_STATUS)) !=
> > > + bist_done_val) {
> > > + dev_err(ice->dev, "HWKM BIST error\n");
> > > + ice->use_hwkm = false;
> > > + err = -ENODEV;
> > > + }
> > > + }
> > > return err;
> > > }
> > >
> > > +static void qcom_ice_enable_standard_mode(struct qcom_ice *ice) {
> > > + u32 val = 0;
> > > +
> > > + /*
> > > + * When ICE is in standard (hwkm) mode, it supports HW wrapped
> > > + * keys, and when it is in legacy mode, it only supports standard
> > > + * (non HW wrapped) keys.
> >
> > I can't say this is very logical.
> >
> > standard mode => HW wrapped keys
> > legacy mode => standard keys
> >
> > Consider changing the terms.
> >
>
> Ack, will make this clearer
>
> > > + *
> > > + * Put ICE in standard mode, ICE defaults to legacy mode.
> > > + * Legacy mode - ICE HWKM slave not supported.
> > > + * Standard mode - ICE HWKM slave supported.
> >
> > s/slave/some other term/
> >
> Ack - will address this.
>
> > Is it possible to use both kind of keys when working on standard mode?
> > If not, it should be the user who selects what type of keys to be used.
> > Enforcing this via DT is not a way to go.
> >
>
> Unfortunately, that support is not there yet. When you say user, do you mean to have it as a filesystem
> mount option?
During cryptsetup time. When running e.g. cryptsetup I, as a user,
would like to be able to use either a hardware-wrapped key or a
standard key.
> The way the UFS/EMMC crypto layer is designed currently is that, this information
> is needed when the modules are loaded.
>
> https://lore.kernel.org/all/20231104211259.17448-2-ebiggers@kernel.org/#Z31drivers:ufs:core:ufshcd-crypto.c
I see that the driver lists capabilities here. E.g. that it supports
HW-wrapped keys. But the line doesn't specify that standard keys are
not supported.
Also, I'd have expected that hw-wrapped keys are handled using trusted
keys mechanism (see security/keys/trusted-keys/). Could you please
point out why that's not the case?
> I am thinking of a way now to do this with DT, but without having a new vendor property.
> Is it acceptable to use the addressable range as the deciding factor? Say use legacy mode of ICE
> when the addressable size is 0x8000 and use HWKM mode of ICE when the addressable size is
> 0x10000.
Definitely, this is a NAK. It's a very unobvious hack. You have been
asked to use compatible strings to detect whether HW keys are
supported or not.
--
With best wishes
Dmitry
Powered by blists - more mailing lists