On Thu, 20 Jun 2024 at 01:30, Gaurav Kashyap (QUIC)
<quic_gaurkash@xxxxxxxxxxx> wrote:
>
> Hello Dmitry
>
> On 06/18/2024 3:17 PM PDT, Dmitry Baryshkov wrote:
> > 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.
> >
>
> What we are looking for with these patches is for per-file/folder encryption using fscrypt policies.
> Cryptsetup to my understanding supports only full-disk , and does not support FBE (File-Based)
I must admit, I mostly used dm-crypt beforehand, so I had to look at
fscrypt now. Some of my previous comments might not be fully
applicable.
> Hence the idea here is that we mount an unencrypted device (with the inlinecrypt option that indicates inline encryption is supported)
> And specify policies (links to keys) for different folders.
>
> > > 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.
> >
>
> Those are capabilities that are read from the storage controller. However, wrapped keys
> Are not a standard in the ICE JEDEC specification, and in most cases, is a value add coming
> from the SoC.
>
> QCOM SOC and firmware currently does not support both kinds of keys in the HWKM mode.
> That is something we are internally working on, but not available yet.
I'd say this is a significant obstacle, at least from my point of
view. I understand that the default might be to use hw-wrapped keys,
but it should be possible for the user to select non-HW keys if the
ability to recover the data is considered to be important. Note, I'm
really pointing to the user here, not to the system integrator. So
using DT property or specifying kernel arguments to switch between
these modes is not really an option.
But I'd really love to hear some feedback from linux-security and/or
linux-fscrypt here.
In my humble opinion the user should be able to specify that the key
is wrapped using the hardware KMK. Then if the hardware has already
started using the other kind of keys, it should be able to respond
with -EINVAL / whatever else. Then the user can evict previously
programmed key and program a desired one.
> > 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 will evaluate this.
> But my initial response is that we currently cannot communicate to our TPM directly from HLOS, but
> goes through QTEE, and I don't think our qtee currently interfaces with the open source tee
> driver. The interface is through QCOM SCM driver.
Note, this is just an API interface, see how it is implemented for the
CAAM hardware.
>
> > > 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
