Re: [PATCH v5 04/15] soc: qcom: ice: add hwkm support in ice

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 19 Jun 2024 at 01:07, Gaurav Kashyap (QUIC)
<quic_gaurkash@xxxxxxxxxxx> 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@xxxxxxxxxx>
> > > Signed-off-by: Gaurav Kashyap <quic_gaurkash@xxxxxxxxxxx>
> > > ---
> > >  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@xxxxxxxxxx/#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




[Index of Archives]     [Kernel]     [Gnu Classpath]     [Gnu Crypto]     [DM Crypt]     [Netfilter]     [Bugtraq]
  Powered by Linux