diff mbox series

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

Message ID 20221006130837.17587-4-pankaj.gupta@nxp.com
State New
Headers show
Series [v0,1/8] hw-bound-key: introducing the generic structure | expand

Commit Message

Pankaj Gupta Oct. 6, 2022, 1:08 p.m. UTC
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(+)

Comments

Pankaj Gupta Oct. 10, 2022, 11:15 a.m. UTC | #1
> -----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
Jason A. Donenfeld Oct. 10, 2022, 3:15 p.m. UTC | #2
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
David Gstir Oct. 10, 2022, 9:35 p.m. UTC | #3
> 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/
Herbert Xu Oct. 11, 2022, 9:03 a.m. UTC | #4
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,
Pankaj Gupta Oct. 11, 2022, 11:05 a.m. UTC | #5
> -----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
Pankaj Gupta Oct. 11, 2022, 11:32 a.m. UTC | #6
> -----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
Jason A. Donenfeld Oct. 11, 2022, 8:01 p.m. UTC | #7
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
Jarkko Sakkinen Oct. 12, 2022, 8:57 a.m. UTC | #8
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
Herbert Xu Oct. 12, 2022, 9:06 a.m. UTC | #9
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,
Jason Gunthorpe Oct. 14, 2022, 7:19 p.m. UTC | #10
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
Eric Biggers Oct. 20, 2022, 4:26 a.m. UTC | #11
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
Jason Gunthorpe Oct. 20, 2022, 7:23 p.m. UTC | #12
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
Eric Biggers Oct. 20, 2022, 9:28 p.m. UTC | #13
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
Jason Gunthorpe Oct. 20, 2022, 11:42 p.m. UTC | #14
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
diff mbox series

Patch

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);