Message-ID: <egtwyk2rp3mtnw2ry6npq5xjfhjvtnymbxy66zevtdi7yvaav4@gcnmrmtqro4b>
Date: Fri, 13 Sep 2024 15:21:07 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Eric Biggers <ebiggers@...nel.org>
Cc: "Gaurav Kashyap (QUIC)" <quic_gaurkash@...cinc.com>, 
	Neil Armstrong <neil.armstrong@...aro.org>, Bartosz Golaszewski <brgl@...ev.pl>, 
	Jens Axboe <axboe@...nel.dk>, Jonathan Corbet <corbet@....net>, 
	Alasdair Kergon <agk@...hat.com>, Mike Snitzer <snitzer@...nel.org>, 
	Mikulas Patocka <mpatocka@...hat.com>, Adrian Hunter <adrian.hunter@...el.com>, 
	Asutosh Das <quic_asutoshd@...cinc.com>, Ritesh Harjani <ritesh.list@...il.com>, 
	Ulf Hansson <ulf.hansson@...aro.org>, Alim Akhtar <alim.akhtar@...sung.com>, 
	Avri Altman <avri.altman@....com>, Bart Van Assche <bvanassche@....org>, 
	"James E.J. Bottomley" <James.Bottomley@...senpartnership.com>, "Martin K. Petersen" <martin.petersen@...cle.com>, 
	"Theodore Y. Ts'o" <tytso@....edu>, Jaegeuk Kim <jaegeuk@...nel.org>, 
	Alexander Viro <viro@...iv.linux.org.uk>, Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>, 
	Bjorn Andersson <andersson@...nel.org>, Konrad Dybcio <konradybcio@...nel.org>, 
	"manivannan.sadhasivam@...aro.org" <manivannan.sadhasivam@...aro.org>, "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>, 
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
	"dm-devel@...ts.linux.dev" <dm-devel@...ts.linux.dev>, "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>, 
	"linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>, "linux-fscrypt@...r.kernel.org" <linux-fscrypt@...r.kernel.org>, 
	"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>, "linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>, 
	"bartosz.golaszewski" <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH v6 09/17] soc: qcom: ice: add HWKM support to the ICE
 driver

On Fri, Sep 13, 2024 at 04:57:16AM GMT, Eric Biggers wrote:
> On Fri, Sep 13, 2024 at 07:28:33AM +0300, Dmitry Baryshkov wrote:
> > On Fri, 13 Sept 2024 at 02:17, Eric Biggers <ebiggers@...nel.org> wrote:
> > >
> > > On Thu, Sep 12, 2024 at 10:17:03PM +0000, Gaurav Kashyap (QUIC) wrote:
> > > >
> > > > On Monday, September 9, 2024 11:29 PM PDT, Dmitry Baryshkov wrote:
> > > > > On Tue, 10 Sept 2024 at 03:51, Gaurav Kashyap (QUIC)
> > > > > <quic_gaurkash@...cinc.com> wrote:
> > > > > >
> > > > > > Hello Dmitry and Neil
> > > > > >
> > > > > > On Monday, September 9, 2024 2:44 AM PDT, Dmitry Baryshkov wrote:
> > > > > > > On Mon, Sep 09, 2024 at 10:58:30AM GMT, Neil Armstrong wrote:
> > > > > > > > On 07/09/2024 00:07, Dmitry Baryshkov wrote:
> > > > > > > > > On Fri, Sep 06, 2024 at 08:07:12PM GMT, Bartosz Golaszewski wrote:
> > > > > > > > > > From: Gaurav Kashyap <quic_gaurkash@...cinc.com>
> > > > > > > > > >
> > > > > > > > > > Qualcomm's ICE (Inline Crypto Engine) contains a proprietary
> > > > > > > > > > key management hardware called Hardware Key Manager (HWKM).
> > > > > > > > > > Add
> > > > > > > HWKM
> > > > > > > > > > support to the ICE driver if it is available on the platform.
> > > > > > > > > > HWKM primarily provides hardware wrapped key support where
> > > > > the
> > > > > > > > > > ICE
> > > > > > > > > > (storage) keys are not available in software and instead
> > > > > > > > > > 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, raw keys have to be used without using the HWKM. We
> > > > > > > > > > query the TZ at run-time to find out whether wrapped keys
> > > > > > > > > > support is
> > > > > > > available.
> > > > > > > > > >
> > > > > > > > > > Tested-by: Neil Armstrong <neil.armstrong@...aro.org>
> > > > > > > > > > Signed-off-by: Gaurav Kashyap <quic_gaurkash@...cinc.com>
> > > > > > > > > > Signed-off-by: Bartosz Golaszewski
> > > > > > > > > > <bartosz.golaszewski@...aro.org>
> > > > > > > > > > ---
> > > > > > > > > >   drivers/soc/qcom/ice.c | 152
> > > > > > > +++++++++++++++++++++++++++++++++++++++++++++++--
> > > > > > > > > >   include/soc/qcom/ice.h |   1 +
> > > > > > > > > >   2 files changed, 149 insertions(+), 4 deletions(-)
> > > > > > > > > >
> > > > > > > > > >   int qcom_ice_enable(struct qcom_ice *ice)
> > > > > > > > > >   {
> > > > > > > > > > + int err;
> > > > > > > > > > +
> > > > > > > > > >           qcom_ice_low_power_mode_enable(ice);
> > > > > > > > > >           qcom_ice_optimization_enable(ice);
> > > > > > > > > > - return qcom_ice_wait_bist_status(ice);
> > > > > > > > > > + if (ice->use_hwkm)
> > > > > > > > > > +         qcom_ice_enable_standard_mode(ice);
> > > > > > > > > > +
> > > > > > > > > > + err = qcom_ice_wait_bist_status(ice); if (err)
> > > > > > > > > > +         return err;
> > > > > > > > > > +
> > > > > > > > > > + if (ice->use_hwkm)
> > > > > > > > > > +         qcom_ice_hwkm_init(ice);
> > > > > > > > > > +
> > > > > > > > > > + return err;
> > > > > > > > > >   }
> > > > > > > > > >   EXPORT_SYMBOL_GPL(qcom_ice_enable);
> > > > > > > > > > @@ -150,6 +282,10 @@ int qcom_ice_resume(struct qcom_ice
> > > > > *ice)
> > > > > > > > > >                   return err;
> > > > > > > > > >           }
> > > > > > > > > > + if (ice->use_hwkm) {
> > > > > > > > > > +         qcom_ice_enable_standard_mode(ice);
> > > > > > > > > > +         qcom_ice_hwkm_init(ice); }
> > > > > > > > > >           return qcom_ice_wait_bist_status(ice);
> > > > > > > > > >   }
> > > > > > > > > >   EXPORT_SYMBOL_GPL(qcom_ice_resume);
> > > > > > > > > > @@ -157,6 +293,7 @@ EXPORT_SYMBOL_GPL(qcom_ice_resume);
> > > > > > > > > >   int qcom_ice_suspend(struct qcom_ice *ice)
> > > > > > > > > >   {
> > > > > > > > > >           clk_disable_unprepare(ice->core_clk);
> > > > > > > > > > + ice->hwkm_init_complete = false;
> > > > > > > > > >           return 0;
> > > > > > > > > >   }
> > > > > > > > > > @@ -206,6 +343,12 @@ int qcom_ice_evict_key(struct qcom_ice
> > > > > > > > > > *ice,
> > > > > > > int slot)
> > > > > > > > > >   }
> > > > > > > > > >   EXPORT_SYMBOL_GPL(qcom_ice_evict_key);
> > > > > > > > > > +bool qcom_ice_hwkm_supported(struct qcom_ice *ice) {  return
> > > > > > > > > > +ice->use_hwkm; }
> > > > > > > EXPORT_SYMBOL_GPL(qcom_ice_hwkm_supported);
> > > > > > > > > > +
> > > > > > > > > >   static struct qcom_ice *qcom_ice_create(struct device *dev,
> > > > > > > > > >                                           void __iomem *base)
> > > > > > > > > >   {
> > > > > > > > > > @@ -240,6 +383,7 @@ static struct qcom_ice
> > > > > > > > > > *qcom_ice_create(struct
> > > > > > > device *dev,
> > > > > > > > > >                   engine->core_clk = devm_clk_get_enabled(dev, NULL);
> > > > > > > > > >           if (IS_ERR(engine->core_clk))
> > > > > > > > > >                   return ERR_CAST(engine->core_clk);
> > > > > > > > > > + engine->use_hwkm = qcom_scm_has_wrapped_key_support();
> > > > > > > > >
> > > > > > > > > This still makes the decision on whether to use HW-wrapped keys
> > > > > > > > > on behalf of a user. I suppose this is incorrect. The user must
> > > > > > > > > be able to use raw keys even if HW-wrapped keys are available on
> > > > > > > > > the platform. One of the examples for such use-cases is if a
> > > > > > > > > user prefers to be able to recover stored information in case of
> > > > > > > > > a device failure (such recovery will be impossible if SoC is
> > > > > > > > > damaged and HW-
> > > > > > > wrapped keys are used).
> > > > > > > >
> > > > > > > > Isn't that already the case ? the
> > > > > BLK_CRYPTO_KEY_TYPE_HW_WRAPPED
> > > > > > > size
> > > > > > > > is here to select HW-wrapped key, otherwise the ol' raw key is passed.
> > > > > > > > Just look the next patch.
> > > > > > > >
> > > > > > > > Or did I miss something ?
> > > > > > >
> > > > > > > That's a good question. If use_hwkm is set, ICE gets programmed to
> > > > > > > use hwkm (see qcom_ice_hwkm_init() call above). I'm not sure if it
> > > > > > > is expected to work properly if after such a call we pass raw key.
> > > > > > >
> > > > > >
> > > > > > Once ICE has moved to a HWKM mode, the firmware key programming
> > > > > currently does not support raw keys.
> > > > > > This support is being added for the next Qualcomm chipset in Trustzone to
> > > > > support both at he same time, but that will take another year or two to hit
> > > > > the market.
> > > > > > Until that time, due to TZ (firmware) limitations , the driver can only
> > > > > support one or the other.
> > > > > >
> > > > > > We also cannot keep moving ICE modes, due to the HWKM enablement
> > > > > being a one-time configurable value at boot.
> > > > >
> > > > > So the init of HWKM should be delayed until the point where the user tells if
> > > > > HWKM or raw keys should be used.
> > > >
> > > > Ack.
> > > > I'll work with Bartosz to look into moving to HWKM mode only during the first key program request
> > > >
> > >
> > > That would mean the driver would have to initially advertise support for both
> > > HW-wrapped keys and raw keys, and then it would revoke the support for one of
> > > them later (due to the other one being used).  However, runtime revocation of
> > > crypto capabilities is not supported by the blk-crypto framework
> > > (Documentation/block/inline-encryption.rst), and there is no clear path to
> > > adding such support.  Upper layers may have already checked the crypto
> > > capabilities and decided to use them.  It's too late to find out that the
> > > support was revoked in the middle of an I/O request.  Upper layer code
> > > (blk-crypto, fscrypt, etc.) is not prepared for this.  And even if it was, the
> > > best it could do is cleanly fail the I/O, which is too late as e.g. it may
> > > happen during background writeback and cause user data to be thrown away.
> > 
> > Can we check crypto capabilities when the user sets the key?
> 
> I think you mean when a key is programmed into a keyslot?  That happens during
> I/O, which is too late as I've explained above.
> 
> > Compare this to the actual HSM used to secure communication or
> > storage. It has certain capabilities, which can be enumerated, etc.
> > But then at the time the user sets the key it is perfectly normal to
> > return an error because HSM is out of resources. It might even have
> > spare key slots, but it might be not enough to be able to program the
> > required key (as a really crazy example, consider the HSM having at
> > this time a single spare DES key slot, while the user wants to program
> > 3DES key).
> 
> That isn't how the kernel handles inline encryption keyslots.  They are only
> programmed as needed for I/O.  If they are all in-use by pending I/O requests,
> then the kernel waits for an I/O request to finish and reprograms the keyslot it
> was using.  There is never an error reported due to lack of keyslots.

Does that mean that the I/O can be outstanding for the very long period
of time? Or that if the ICE hardware has just a single keyslot, but
there are two concurrent I/O processes using two different keys, the
framework will be constantly swapping the keys programmed to the HW?

I think it might be prefereable for the drivers and the framework to
support "preprogramming" of the keys, when the key is programmed to the
hardware when it is set by the user.

Another option might be to let the drivers validate the keys being set
by userspace. This way in our case the driver might report that it
supports both raw and wrapped keys, but start rejecting the keys once
it gets notified that the user has programmed other kind of keys. This
way key setup can fail, but the actual I/O can not. WDYT?

> If I/O requests could randomly fail at any time when using inline encryption,
> then no one would use inline encryption because it would not be reliable.

Yes, I agree here.

> 
> > > So, the choice of support for HW-wrapped vs. raw will need to be made ahead of
> > > time, rather than being implicitly set by the first use.  That is most easily
> > > done using a module parameter like qcom_ice.hw_wrapped_keys=1.  Yes, it's a bit
> > > inconvenient, but there's no realistic way around this currently.
> > 
> > This doesn't work for Android usecase. The user isn't able to setup modparams.
> 
> It does work for Android.  The encryption setting that Android uses is
> configured in the build of Android for the device (by the OEM, or by whoever
> made the build in the case of a custom build).  Refer to
> https://source.android.com/docs/security/features/encryption/file-based#enabling-file-based-encryption
> 
> Anyone who can change that can also change the kernel command line.

Ok. I think if the 'validation' or 'notify' proposal is declined, I'll
have to agree to the modparam.

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists