[PATCH v0 3/8] crypto: hbk flags & info added to the tfm

Pankaj Gupta posted 8 patches 3 years, 2 months ago
[PATCH v0 3/8] crypto: hbk flags & info added to the tfm
Posted by Pankaj Gupta 3 years, 2 months ago
Consumer of the kernel crypto api, after allocating
the transformation (tfm), sets the:
- flag 'is_hbk'
- structure 'struct hw_bound_key_info hbk_info'
based on the type of key, the consumer is using.

This helps:

- This helps to influence the core processing logic
  for the encapsulated algorithm.
- This flag is set by the consumer after allocating
  the tfm and before calling the function crypto_xxx_setkey().

Signed-off-by: Pankaj Gupta <pankaj.gupta@nxp.com>
---
 include/linux/crypto.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/crypto.h b/include/linux/crypto.h
index 2324ab6f1846..cd476f8a1cb4 100644
--- a/include/linux/crypto.h
+++ b/include/linux/crypto.h
@@ -19,6 +19,7 @@
 #include <linux/refcount.h>
 #include <linux/slab.h>
 #include <linux/completion.h>
+#include <linux/hw_bound_key.h>
 
 /*
  * Autoloaded crypto modules should only use a prefixed name to avoid allowing
@@ -639,6 +640,10 @@ struct crypto_tfm {
 
 	u32 crt_flags;
 
+	unsigned int is_hbk;
+
+	struct hw_bound_key_info hbk_info;
+
 	int node;
 	
 	void (*exit)(struct crypto_tfm *tfm);
-- 
2.17.1
Re: [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
Posted by Jarkko Sakkinen 3 years, 2 months ago
What are "hbk flags & info" and "the tfm"?

There can be multiple instances of struct crypto_tfm in
the kernel.

Maybe "crypto: Add hbk_info and is_hbk to struct crypto_tfm" ?

On Thu, Oct 06, 2022 at 06:38:32PM +0530, Pankaj Gupta wrote:
> Consumer of the kernel crypto api, after allocating
> the transformation (tfm), sets the:
> - flag 'is_hbk'
> - structure 'struct hw_bound_key_info hbk_info'
> based on the type of key, the consumer is using.
> 
> This helps:
> 
> - This helps to influence the core processing logic
>   for the encapsulated algorithm.
> - This flag is set by the consumer after allocating
>   the tfm and before calling the function crypto_xxx_setkey().

I don't really get "this helps part".



> 
> Signed-off-by: Pankaj Gupta <pankaj.gupta@nxp.com>
> ---
>  include/linux/crypto.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/crypto.h b/include/linux/crypto.h
> index 2324ab6f1846..cd476f8a1cb4 100644
> --- a/include/linux/crypto.h
> +++ b/include/linux/crypto.h
> @@ -19,6 +19,7 @@
>  #include <linux/refcount.h>
>  #include <linux/slab.h>
>  #include <linux/completion.h>
> +#include <linux/hw_bound_key.h>
>  
>  /*
>   * Autoloaded crypto modules should only use a prefixed name to avoid allowing
> @@ -639,6 +640,10 @@ struct crypto_tfm {
>  
>  	u32 crt_flags;
>  
> +	unsigned int is_hbk;

Not sure why not just use bool as type here.

> +
> +	struct hw_bound_key_info hbk_info;
> +
>  	int node;
>  	
>  	void (*exit)(struct crypto_tfm *tfm);
> -- 
> 2.17.1
> 

BR, Jarkko
Re: [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
Posted by Herbert Xu 3 years, 2 months ago
On Thu, Oct 06, 2022 at 06:38:32PM +0530, Pankaj Gupta wrote:
> Consumer of the kernel crypto api, after allocating
> the transformation (tfm), sets the:
> - flag 'is_hbk'
> - structure 'struct hw_bound_key_info hbk_info'
> based on the type of key, the consumer is using.
> 
> This helps:
> 
> - This helps to influence the core processing logic
>   for the encapsulated algorithm.
> - This flag is set by the consumer after allocating
>   the tfm and before calling the function crypto_xxx_setkey().
> 
> Signed-off-by: Pankaj Gupta <pankaj.gupta@nxp.com>
> ---
>  include/linux/crypto.h | 5 +++++
>  1 file changed, 5 insertions(+)

Nack.  You still have not provided a convincing argument why
this is necessary since there are plenty of existing drivers in
the kernel already providing similar features.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
RE: [EXT] Re: [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
Posted by Pankaj Gupta 3 years, 2 months ago

> -----Original Message-----
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Sent: Friday, October 7, 2022 12:28 PM
> To: Pankaj Gupta <pankaj.gupta@nxp.com>
> Cc: jarkko@kernel.org; a.fatoum@pengutronix.de; gilad@benyossef.com;
> Jason@zx2c4.com; jejb@linux.ibm.com; zohar@linux.ibm.com;
> dhowells@redhat.com; sumit.garg@linaro.org; david@sigma-star.at;
> michael@walle.cc; john.ernberg@actia.se; jmorris@namei.org;
> serge@hallyn.com; davem@davemloft.net; j.luebbe@pengutronix.de;
> ebiggers@kernel.org; richard@nod.at; keyrings@vger.kernel.org; linux-
> crypto@vger.kernel.org; linux-integrity@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-security-module@vger.kernel.org; Sahil Malhotra
> <sahil.malhotra@nxp.com>; Kshitiz Varshney <kshitiz.varshney@nxp.com>;
> Horia Geanta <horia.geanta@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
> Subject: [EXT] Re: [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
> 
> Caution: EXT Email
> 
> On Thu, Oct 06, 2022 at 06:38:32PM +0530, Pankaj Gupta wrote:
> > Consumer of the kernel crypto api, after allocating the transformation
> > (tfm), sets the:
> > - flag 'is_hbk'
> > - structure 'struct hw_bound_key_info hbk_info'
> > based on the type of key, the consumer is using.
> >
> > This helps:
> >
> > - This helps to influence the core processing logic
> >   for the encapsulated algorithm.
> > - This flag is set by the consumer after allocating
> >   the tfm and before calling the function crypto_xxx_setkey().
> >
> > Signed-off-by: Pankaj Gupta <pankaj.gupta@nxp.com>
> > ---
> >  include/linux/crypto.h | 5 +++++
> >  1 file changed, 5 insertions(+)
> 
> Nack.  You still have not provided a convincing argument why this is necessary
> since there are plenty of existing drivers in the kernel already providing similar
> features.
> 
CAAM is used as a trusted source for trusted keyring. CAAM can expose these keys either as plain key or HBK(hardware bound key- managed by the hardware only and never visible in plain outside of hardware).

Thus, Keys that are inside CAAM-backed-trusted-keyring, can either be plain key or HBK. So the trusted-key-payload requires additional flag & info(key-encryption-protocol)  to help differentiate it from each other. Now when CAAM trusted-key is presented to the kernel crypto framework, the additional information associated with the key, needs to be passed to the hardware driver. Currently the kernel keyring and kernel crypto frameworks are associated for plain key, but completely dis-associated for HBK. This patch addresses this problem.

Similar capabilities (trusted source), are there in other crypto accelerators on NXP SoC(s). Having hardware specific crypto algorithm name, does not seems to be a scalable solution.

> Cheers,
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page:
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgondor.ap
> ana.org.au%2F~herbert%2F&amp;data=05%7C01%7Cpankaj.gupta%40nxp.com
> %7C33055110772a4d4bb97508daa8317e93%7C686ea1d3bc2b4c6fa92cd99c5c3
> 01635%7C0%7C0%7C638007227793511347%7CUnknown%7CTWFpbGZsb3d8ey
> JWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C
> 3000%7C%7C%7C&amp;sdata=H0fzzxQhsV%2FyPphBAHBDmzQfTFnjDE7jYstTM
> ok%2F09I%3D&amp;reserved=0
> PGP Key:
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgondor.ap
> ana.org.au%2F~herbert%2Fpubkey.txt&amp;data=05%7C01%7Cpankaj.gupta%4
> 0nxp.com%7C33055110772a4d4bb97508daa8317e93%7C686ea1d3bc2b4c6fa9
> 2cd99c5c301635%7C0%7C0%7C638007227793667554%7CUnknown%7CTWFpb
> GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6M
> n0%3D%7C3000%7C%7C%7C&amp;sdata=SclJ9G7jBWhOW%2Fm3Gt0jP1oicqVp
> 5ghH%2BDT8Vd5maag%3D&amp;reserved=0
Re: [EXT] Re: [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
Posted by Jason A. Donenfeld 3 years, 2 months ago
On Mon, Oct 10, 2022 at 11:15:00AM +0000, Pankaj Gupta wrote:
> > Nack.  You still have not provided a convincing argument why this is necessary
> > since there are plenty of existing drivers in the kernel already providing similar
> > features.
> > 
> CAAM is used as a trusted source for trusted keyring. CAAM can expose
> these keys either as plain key or HBK(hardware bound key- managed by
> the hardware only and never visible in plain outside of hardware).
> 
> Thus, Keys that are inside CAAM-backed-trusted-keyring, can either be
> plain key or HBK. So the trusted-key-payload requires additional flag
> & info(key-encryption-protocol)  to help differentiate it from each
> other. Now when CAAM trusted-key is presented to the kernel crypto
> framework, the additional information associated with the key, needs
> to be passed to the hardware driver. Currently the kernel keyring and
> kernel crypto frameworks are associated for plain key, but completely
> dis-associated for HBK. This patch addresses this problem.
> 
> Similar capabilities (trusted source), are there in other crypto
> accelerators on NXP SoC(s). Having hardware specific crypto algorithm
> name, does not seems to be a scalable solution.

Do you mean to say that other drivers that use hardware-backed keys do
so by setting "cra_name" to something particular? Like instead of "aes"
it'd be "aes-but-special-for-this-driver"? If so, that would seem to
break the design of the crypto API. Which driver did you see that does
this? Or perhaps, more generally, what are the drivers that Herbert is
talking about when he mentions the "plenty of existing drivers" that
already do this?

Jason
Re: [EXT] [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
Posted by David Gstir 3 years, 2 months ago

> On 10.10.2022, at 17:15, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> 
> On Mon, Oct 10, 2022 at 11:15:00AM +0000, Pankaj Gupta wrote:
>>> Nack.  You still have not provided a convincing argument why this is necessary
>>> since there are plenty of existing drivers in the kernel already providing similar
>>> features.
>>> 
>> CAAM is used as a trusted source for trusted keyring. CAAM can expose
>> these keys either as plain key or HBK(hardware bound key- managed by
>> the hardware only and never visible in plain outside of hardware).
>> 
>> Thus, Keys that are inside CAAM-backed-trusted-keyring, can either be
>> plain key or HBK. So the trusted-key-payload requires additional flag
>> & info(key-encryption-protocol)  to help differentiate it from each
>> other. Now when CAAM trusted-key is presented to the kernel crypto
>> framework, the additional information associated with the key, needs
>> to be passed to the hardware driver. Currently the kernel keyring and
>> kernel crypto frameworks are associated for plain key, but completely
>> dis-associated for HBK. This patch addresses this problem.
>> 
>> Similar capabilities (trusted source), are there in other crypto
>> accelerators on NXP SoC(s). Having hardware specific crypto algorithm
>> name, does not seems to be a scalable solution.
> 
> Do you mean to say that other drivers that use hardware-backed keys do
> so by setting "cra_name" to something particular? Like instead of "aes"
> it'd be "aes-but-special-for-this-driver"? If so, that would seem to
> break the design of the crypto API. Which driver did you see that does
> this? Or perhaps, more generally, what are the drivers that Herbert is
> talking about when he mentions the "plenty of existing drivers" that
> already do this?

I believe what Herbert means are drivers registered with the cipher name 
prefix “p”. E.g. [1] registers multiple “paes” variants. There was a
previous patch set for CAAM where this was suggested as well [2].

- David

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/crypto/ccree/cc_cipher.c#n1011
[2] https://lore.kernel.org/linux-crypto/20200716073610.GA28215@gondor.apana.org.au/
RE: [EXT] Re: [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
Posted by Pankaj Gupta 3 years, 2 months ago
> -----Original Message-----
> From: Jason A. Donenfeld <Jason@zx2c4.com>
> Sent: Monday, October 10, 2022 8:46 PM
> To: Pankaj Gupta <pankaj.gupta@nxp.com>
> Cc: 'Herbert Xu' <herbert@gondor.apana.org.au>; jarkko@kernel.org;
> a.fatoum@pengutronix.de; gilad@benyossef.com; jejb@linux.ibm.com;
> zohar@linux.ibm.com; dhowells@redhat.com; sumit.garg@linaro.org;
> david@sigma-star.at; michael@walle.cc; john.ernberg@actia.se;
> jmorris@namei.org; serge@hallyn.com; davem@davemloft.net;
> j.luebbe@pengutronix.de; ebiggers@kernel.org; richard@nod.at;
> keyrings@vger.kernel.org; linux-crypto@vger.kernel.org; linux-
> integrity@vger.kernel.org; linux-kernel@vger.kernel.org; linux-security-
> module@vger.kernel.org; Sahil Malhotra <sahil.malhotra@nxp.com>; Kshitiz
> Varshney <kshitiz.varshney@nxp.com>; Horia Geanta
> <horia.geanta@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
> Subject: Re: [EXT] Re: [PATCH v0 3/8] crypto: hbk flags & info added to the
> tfm
> 
> Caution: EXT Email
> 
> On Mon, Oct 10, 2022 at 11:15:00AM +0000, Pankaj Gupta wrote:
> > > Nack.  You still have not provided a convincing argument why this is
> > > necessary since there are plenty of existing drivers in the kernel
> > > already providing similar features.
> > >
> > CAAM is used as a trusted source for trusted keyring. CAAM can expose
> > these keys either as plain key or HBK(hardware bound key- managed by
> > the hardware only and never visible in plain outside of hardware).
> >
> > Thus, Keys that are inside CAAM-backed-trusted-keyring, can either be
> > plain key or HBK. So the trusted-key-payload requires additional flag
> > & info(key-encryption-protocol)  to help differentiate it from each
> > other. Now when CAAM trusted-key is presented to the kernel crypto
> > framework, the additional information associated with the key, needs
> > to be passed to the hardware driver. Currently the kernel keyring and
> > kernel crypto frameworks are associated for plain key, but completely
> > dis-associated for HBK. This patch addresses this problem.
> >
> > Similar capabilities (trusted source), are there in other crypto
> > accelerators on NXP SoC(s). Having hardware specific crypto algorithm
> > name, does not seems to be a scalable solution.
> 
> Do you mean to say that other drivers that use hardware-backed keys do so
> by setting "cra_name" to something particular? 

Yes.

> Like instead of "aes"
> it'd be "aes-but-special-for-this-driver"?

For example: ARM-Crypto-Cell prepends 'p':
- xts(paes) for xts(aes)
- xts(paes) for xts(aes)...etc.

 > If so, that would seem to break the
> design of the crypto API. Which driver did you see that does this?  Or perhaps,
> more generally, what are the drivers that Herbert is talking about when he
> mentions the "plenty of existing drivers" that already do this?
I could find this driver " drivers/crypto/ccree/".
Reference file name is " drivers/crypto/ccree/cc_cipher.c"

Likewise, CAAM being a trust source, new cra_name would be need to deal with CAAM generated HBKs too.
We need to come up with something unique like: for eg:   p(xts(aes)) for xts(aes)             
   
Another trust source from NXP called DCP(drivers/crypto/mxs-dcp.c),  new cra_name would be needed for that too.
There are work in progress for other trust sources from NXP.

So, our approach is too provide generic solution, which can be extended to any trust source generating HBK.


> 
> Jason
Re: [EXT] Re: [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
Posted by Herbert Xu 3 years, 2 months ago
On Mon, Oct 10, 2022 at 09:15:48AM -0600, Jason A. Donenfeld wrote:
>
> Do you mean to say that other drivers that use hardware-backed keys do
> so by setting "cra_name" to something particular? Like instead of "aes"
> it'd be "aes-but-special-for-this-driver"? If so, that would seem to
> break the design of the crypto API. Which driver did you see that does
> this? Or perhaps, more generally, what are the drivers that Herbert is
> talking about when he mentions the "plenty of existing drivers" that
> already do this?

Grep for paes for the existing drivers that support this.  I don't
have anything against this feature per se, but the last thing we
want is a proliferation of different ways of doing the same thing.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [EXT] Re: [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
Posted by Jason A. Donenfeld 3 years, 2 months ago
On Tue, Oct 11, 2022 at 05:03:31PM +0800, Herbert Xu wrote:
> On Mon, Oct 10, 2022 at 09:15:48AM -0600, Jason A. Donenfeld wrote:
> >
> > Do you mean to say that other drivers that use hardware-backed keys do
> > so by setting "cra_name" to something particular? Like instead of "aes"
> > it'd be "aes-but-special-for-this-driver"? If so, that would seem to
> > break the design of the crypto API. Which driver did you see that does
> > this? Or perhaps, more generally, what are the drivers that Herbert is
> > talking about when he mentions the "plenty of existing drivers" that
> > already do this?
> 
> Grep for paes for the existing drivers that support this.  I don't
> have anything against this feature per se, but the last thing we
> want is a proliferation of different ways of doing the same thing.

I've got no stake in this, but isn't the whole idea that if you specify
"aes" you get AES, and if you specify "cbc(aes)" you get AES-CBC, and so
forth? And so leaking implementation details into the algorithm name
feels like it breaks the abstraction a bit.

Rather, drivers that do AES should be called "aes". For this hardware
key situation, I guess that means keys have a type (in-memory vs
hardware-resident). Then, a crypto operation takes an "algorithm" and a
"key", and the abstraction then picks the best implementation that's
compatible with both the "algorithm" and the "key".

I haven't looked carefully, but I assume that's more or less what this
patchset does.

If you don't want a proliferation of different ways of doing the same
thing, maybe the requirement should be that the author of this series
also converts the existing "paes" kludge to use the new thing he's
proposing?

Or maybe the "paes" kludge is better for other reasons? I don't know
really. Just my 2¢, but feel free to disregard, as I really have nothing
to do with this change.

Jason
Re: [EXT] Re: [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
Posted by Herbert Xu 3 years, 2 months ago
On Tue, Oct 11, 2022 at 02:01:45PM -0600, Jason A. Donenfeld wrote:
>
> I've got no stake in this, but isn't the whole idea that if you specify
> "aes" you get AES, and if you specify "cbc(aes)" you get AES-CBC, and so
> forth? And so leaking implementation details into the algorithm name
> feels like it breaks the abstraction a bit.

Well, keys stored in hardware are fundamentally incompatible with
the algorithm/implementation model.  The whole point of having
algorithms with multiple implementations (e.g., drivers) is that
they all provide exactly the same functionality and could be
substituted at will.

This completely breaks down with hardware keys because by definition
the key is stored in a specific piece of hardware so it will only
work with a particular driver.  IOW it almost never makes sense
to allocate "aes" if you have a hardware key, you almost always
want to allocate "aes-mydriver" instead.

> Rather, drivers that do AES should be called "aes". For this hardware
> key situation, I guess that means keys have a type (in-memory vs
> hardware-resident). Then, a crypto operation takes an "algorithm" and a
> "key", and the abstraction then picks the best implementation that's
> compatible with both the "algorithm" and the "key".

No the key is already in a specific hardware bound to some driver.
The user already knows where the key is and therefore they know
which driver it is.

> If you don't want a proliferation of different ways of doing the same
> thing, maybe the requirement should be that the author of this series
> also converts the existing "paes" kludge to use the new thing he's
> proposing?

Yes that would definitely be a good idea.  We should also talk to the
people who added paes in the first place, i.e., s390.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [EXT] Re: [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
Posted by Jason Gunthorpe 3 years, 2 months ago
On Wed, Oct 12, 2022 at 05:06:16PM +0800, Herbert Xu wrote:

> > Rather, drivers that do AES should be called "aes". For this hardware
> > key situation, I guess that means keys have a type (in-memory vs
> > hardware-resident). Then, a crypto operation takes an "algorithm" and a
> > "key", and the abstraction then picks the best implementation that's
> > compatible with both the "algorithm" and the "key".
> 
> No the key is already in a specific hardware bound to some driver.
> The user already knows where the key is and therefore they know
> which driver it is.

Do they?

We have HW that can do HW resident keys as as well, in our case it is
plugged into the storage system with fscrypt and all the crypto
operations are being done "inline" as the data is DMA'd into/out of
the storage. So, no crypto API here.

I would say the user knows about the key and its binding in the sense
they loaded a key into the storage device and mounted a fscrypt
filesystem from that storage device - but the kernel may not know this
explicitly.

> > If you don't want a proliferation of different ways of doing the same
> > thing, maybe the requirement should be that the author of this series
> > also converts the existing "paes" kludge to use the new thing he's
> > proposing?
> 
> Yes that would definitely be a good idea.  We should also talk to the
> people who added paes in the first place, i.e., s390.

Yes, it would be nice to see a comprehensive understand on how HW
resident keys can be modeled in the keyring. Almost every computer now
has a TPM that is also quite capable of doing operations with these
kinds of keys. Seeing the whole picture, including how we generate and
load/save/provision these things seems important.

Jason
Re: [EXT] Re: [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
Posted by Eric Biggers 3 years, 1 month ago
Hi Jason,

On Fri, Oct 14, 2022 at 04:19:41PM -0300, Jason Gunthorpe wrote:
> On Wed, Oct 12, 2022 at 05:06:16PM +0800, Herbert Xu wrote:
> 
> > > Rather, drivers that do AES should be called "aes". For this hardware
> > > key situation, I guess that means keys have a type (in-memory vs
> > > hardware-resident). Then, a crypto operation takes an "algorithm" and a
> > > "key", and the abstraction then picks the best implementation that's
> > > compatible with both the "algorithm" and the "key".
> > 
> > No the key is already in a specific hardware bound to some driver.
> > The user already knows where the key is and therefore they know
> > which driver it is.
> 
> Do they?
> 
> We have HW that can do HW resident keys as as well, in our case it is
> plugged into the storage system with fscrypt and all the crypto
> operations are being done "inline" as the data is DMA'd into/out of
> the storage. So, no crypto API here.
> 
> I would say the user knows about the key and its binding in the sense
> they loaded a key into the storage device and mounted a fscrypt
> filesystem from that storage device - but the kernel may not know this
> explicitly.

Are you referring to the support for hardware-wrapped inline crypto keys?  It
isn't upstream yet, but my latest patchset is at
https://lore.kernel.org/linux-fscrypt/20220927014718.125308-2-ebiggers@kernel.org/T/#u.
There's also a version of it used by some Android devices already.  Out of
curiosity, are you using it in an Android device, or have you adopted it in some
other downstream?

Anyway, that feature does indeed use a boolean flag to indicate whether the key
is hardware-wrapped or not.  And yes, it doesn't use the crypto API.  Nor does
it use the keyrings subsystem, for that matter.

However, the design of hardware-wrapped inline crypto keys is that keys are
scoped to a particular block device (or a set of block devices), which are
assumed to have only one version of wrapped keys.  That makes the boolean flag
work, as it's always unambiguous what the keys mean.

I don't think that would work as well for the crypto API, which is a bit more
general.  In the crypto API, there can be an arbitrary number of crypto drivers,
each of which has its own version of hardware-wrapped (bound) keys.  So maybe
the existing design that is based on algorithm names is fine.

> > > If you don't want a proliferation of different ways of doing the same
> > > thing, maybe the requirement should be that the author of this series
> > > also converts the existing "paes" kludge to use the new thing he's
> > > proposing?
> > 
> > Yes that would definitely be a good idea.  We should also talk to the
> > people who added paes in the first place, i.e., s390.
> 
> Yes, it would be nice to see a comprehensive understand on how HW
> resident keys can be modeled in the keyring.

Note that the keyrings subsystem is not as useful as it might seem.  It sounds
like something you want (you have keys, and there is a subsystem called
"keyrings", so it should be used, right?), but often it isn't.  fscrypt has
mostly moved away from using it, as it caused lots of problems.  I would caution
against assuming that it needs to be part of any solution.

- Eric
Re: [EXT] Re: [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
Posted by Jason Gunthorpe 3 years, 1 month ago
On Wed, Oct 19, 2022 at 09:26:05PM -0700, Eric Biggers wrote:

> Are you referring to the support for hardware-wrapped inline crypto keys?  It
> isn't upstream yet, but my latest patchset is at
> https://lore.kernel.org/linux-fscrypt/20220927014718.125308-2-ebiggers@kernel.org/T/#u.
> There's also a version of it used by some Android devices already.  Out of
> curiosity, are you using it in an Android device, or have you adopted it in some
> other downstream?

Unrelated to Android, similar functionality, but slightly different
ultimate purpose. We are going to be sending a fscrypt patch series
for mlx5 and nvme soonish.

> > Yes, it would be nice to see a comprehensive understand on how HW
> > resident keys can be modeled in the keyring.
> 
> Note that the keyrings subsystem is not as useful as it might seem.  It sounds
> like something you want (you have keys, and there is a subsystem called
> "keyrings", so it should be used, right?), but often it isn't.  fscrypt has
> mostly moved away from using it, as it caused lots of problems.  I would caution
> against assuming that it needs to be part of any solution.

That sounds disappointing that we are now having parallel ways for the
admin to manipulate kernel owned keys.

Jason
Re: [EXT] Re: [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
Posted by Eric Biggers 3 years, 1 month ago
On Thu, Oct 20, 2022 at 04:23:53PM -0300, Jason Gunthorpe wrote:
> On Wed, Oct 19, 2022 at 09:26:05PM -0700, Eric Biggers wrote:
> 
> > Are you referring to the support for hardware-wrapped inline crypto keys?  It
> > isn't upstream yet, but my latest patchset is at
> > https://lore.kernel.org/linux-fscrypt/20220927014718.125308-2-ebiggers@kernel.org/T/#u.
> > There's also a version of it used by some Android devices already.  Out of
> > curiosity, are you using it in an Android device, or have you adopted it in some
> > other downstream?
> 
> Unrelated to Android, similar functionality, but slightly different
> ultimate purpose. We are going to be sending a fscrypt patch series
> for mlx5 and nvme soonish.

That's interesting, though also slightly scary in that it sounds like you've
already shipped some major fscrypt changes without review!

> > > Yes, it would be nice to see a comprehensive understand on how HW
> > > resident keys can be modeled in the keyring.
> > 
> > Note that the keyrings subsystem is not as useful as it might seem.  It sounds
> > like something you want (you have keys, and there is a subsystem called
> > "keyrings", so it should be used, right?), but often it isn't.  fscrypt has
> > mostly moved away from using it, as it caused lots of problems.  I would caution
> > against assuming that it needs to be part of any solution.
> 
> That sounds disappointing that we are now having parallel ways for the
> admin to manipulate kernel owned keys.

Well, the keyrings subsystem never worked properly for fscrypt anyway.  At most,
it's only useful for providing the key to the filesystem initially (by passing a
key ID to FS_IOC_ADD_ENCRYPTION_KEY, instead of the key bytes), similar to what
dm-crypt allows.  After that, the keyrings subsystem plays no role.

I'm open to making FS_IOC_ADD_ENCRYPTION_KEY accept other 'struct key' types,
like "trusted" which has been discussed before and which dm-crypt supports.

Just don't assume that just because you have a key, that you automatically
*need* the keyrings subsystem.  Normally just passing the key bytes in the ioctl
works just as well and is much simpler.  Same for dm-crypt, which normally takes
the key bytes in the device-mapper table parameters...

- Eric
Re: [EXT] Re: [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
Posted by Jason Gunthorpe 3 years, 1 month ago
On Thu, Oct 20, 2022 at 02:28:36PM -0700, Eric Biggers wrote:
> On Thu, Oct 20, 2022 at 04:23:53PM -0300, Jason Gunthorpe wrote:
> > On Wed, Oct 19, 2022 at 09:26:05PM -0700, Eric Biggers wrote:
> > 
> > > Are you referring to the support for hardware-wrapped inline crypto keys?  It
> > > isn't upstream yet, but my latest patchset is at
> > > https://lore.kernel.org/linux-fscrypt/20220927014718.125308-2-ebiggers@kernel.org/T/#u.
> > > There's also a version of it used by some Android devices already.  Out of
> > > curiosity, are you using it in an Android device, or have you adopted it in some
> > > other downstream?
> > 
> > Unrelated to Android, similar functionality, but slightly different
> > ultimate purpose. We are going to be sending a fscrypt patch series
> > for mlx5 and nvme soonish.
> 
> That's interesting, though also slightly scary in that it sounds like you've
> already shipped some major fscrypt changes without review!

Heh, says the Android guy :)

Fortunately nothing major, we are enterprise focused, we need stuff in
real distros - we know know how to do it.

> > That sounds disappointing that we are now having parallel ways for the
> > admin to manipulate kernel owned keys.
> 
> Well, the keyrings subsystem never worked properly for fscrypt anyway.  At most,
> it's only useful for providing the key to the filesystem initially (by passing a
> key ID to FS_IOC_ADD_ENCRYPTION_KEY, instead of the key bytes), similar to what
> dm-crypt allows.  After that, the keyrings subsystem plays no role.

Sure, but loading the key into the keyring should allow many different
options, including things like TPM PCR secured keys (eg like
bitlocker) - we shouldn't allow user space the ability to see the key
data at all.

Duplicating this in every subsystem makes no sense, there is a
reasonable role for the keyring to play in solving these kinds of
problems for everything.

Jason
RE: [EXT] Re: [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
Posted by Pankaj Gupta 3 years, 2 months ago

> -----Original Message-----
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Sent: Tuesday, October 11, 2022 2:34 PM
> To: Jason A. Donenfeld <Jason@zx2c4.com>
> Cc: Pankaj Gupta <pankaj.gupta@nxp.com>; jarkko@kernel.org;
> a.fatoum@pengutronix.de; gilad@benyossef.com; jejb@linux.ibm.com;
> zohar@linux.ibm.com; dhowells@redhat.com; sumit.garg@linaro.org;
> david@sigma-star.at; michael@walle.cc; john.ernberg@actia.se;
> jmorris@namei.org; serge@hallyn.com; davem@davemloft.net;
> j.luebbe@pengutronix.de; ebiggers@kernel.org; richard@nod.at;
> keyrings@vger.kernel.org; linux-crypto@vger.kernel.org; linux-
> integrity@vger.kernel.org; linux-kernel@vger.kernel.org; linux-security-
> module@vger.kernel.org; Sahil Malhotra <sahil.malhotra@nxp.com>; Kshitiz
> Varshney <kshitiz.varshney@nxp.com>; Horia Geanta
> <horia.geanta@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
> Subject: Re: [EXT] Re: [PATCH v0 3/8] crypto: hbk flags & info added to the
> tfm
> 
> Caution: EXT Email
> 
> On Mon, Oct 10, 2022 at 09:15:48AM -0600, Jason A. Donenfeld wrote:
> >
> > Do you mean to say that other drivers that use hardware-backed keys do
> > so by setting "cra_name" to something particular? Like instead of "aes"
> > it'd be "aes-but-special-for-this-driver"? If so, that would seem to
> > break the design of the crypto API. Which driver did you see that does
> > this? Or perhaps, more generally, what are the drivers that Herbert is
> > talking about when he mentions the "plenty of existing drivers" that
> > already do this?
> 
> Grep for paes for the existing drivers that support this.  I don't have anything
> against this feature per se, but the last thing we want is a proliferation of
> different ways of doing the same thing.

Our goal is to have a generic solution, which can be extended to any driver dealing with:
- Generating HBK and adding to trusted keyring.
- Using the trusted keyring's HBK for crypto operation.

With this framework in place, driver specific custom changes can be avoided, bridging the interface-gap of:
kernel-keyring <-> kernel-crypto-layer.

Thanks.
> 
> Cheers,
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page:
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgondo
> r.apana.org.au%2F~herbert%2F&amp;data=05%7C01%7Cpankaj.gupta%40nx
> p.com%7C4ef27fc922d04350ca9f08daab67a1a3%7C686ea1d3bc2b4c6fa92cd9
> 9c5c301635%7C0%7C0%7C638010758832054902%7CUnknown%7CTWFpbGZs
> b3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn
> 0%3D%7C3000%7C%7C%7C&amp;sdata=SOguJ9LGhSCDmspbjDIEzkQLk9Bz%
> 2FsS0B%2BLNc4gzRo8%3D&amp;reserved=0
> PGP Key:
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgondo
> r.apana.org.au%2F~herbert%2Fpubkey.txt&amp;data=05%7C01%7Cpankaj.g
> upta%40nxp.com%7C4ef27fc922d04350ca9f08daab67a1a3%7C686ea1d3bc2b
> 4c6fa92cd99c5c301635%7C0%7C0%7C638010758832054902%7CUnknown%7C
> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL
> CJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=hCzT2fPfJ%2BBNVqN6JR
> wMx9zNJkqvdRSLrR68ubhCvN4%3D&amp;reserved=0