| Message ID | 20221006130837.17587-4-pankaj.gupta@nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | Hardware Bound key added to Trusted Key-Ring | expand |
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,
> -----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&data=05%7C01%7Cpankaj.gupta%40nxp.com > %7C33055110772a4d4bb97508daa8317e93%7C686ea1d3bc2b4c6fa92cd99c5c3 > 01635%7C0%7C0%7C638007227793511347%7CUnknown%7CTWFpbGZsb3d8ey > JWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C > 3000%7C%7C%7C&sdata=H0fzzxQhsV%2FyPphBAHBDmzQfTFnjDE7jYstTM > ok%2F09I%3D&reserved=0 > PGP Key: > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgondor.ap > ana.org.au%2F~herbert%2Fpubkey.txt&data=05%7C01%7Cpankaj.gupta%4 > 0nxp.com%7C33055110772a4d4bb97508daa8317e93%7C686ea1d3bc2b4c6fa9 > 2cd99c5c301635%7C0%7C0%7C638007227793667554%7CUnknown%7CTWFpb > GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6M > n0%3D%7C3000%7C%7C%7C&sdata=SclJ9G7jBWhOW%2Fm3Gt0jP1oicqVp > 5ghH%2BDT8Vd5maag%3D&reserved=0
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
> 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/
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,
> -----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
> -----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&data=05%7C01%7Cpankaj.gupta%40nx > p.com%7C4ef27fc922d04350ca9f08daab67a1a3%7C686ea1d3bc2b4c6fa92cd9 > 9c5c301635%7C0%7C0%7C638010758832054902%7CUnknown%7CTWFpbGZs > b3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn > 0%3D%7C3000%7C%7C%7C&sdata=SOguJ9LGhSCDmspbjDIEzkQLk9Bz% > 2FsS0B%2BLNc4gzRo8%3D&reserved=0 > PGP Key: > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgondo > r.apana.org.au%2F~herbert%2Fpubkey.txt&data=05%7C01%7Cpankaj.g > upta%40nxp.com%7C4ef27fc922d04350ca9f08daab67a1a3%7C686ea1d3bc2b > 4c6fa92cd99c5c301635%7C0%7C0%7C638010758832054902%7CUnknown%7C > TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL > CJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=hCzT2fPfJ%2BBNVqN6JR > wMx9zNJkqvdRSLrR68ubhCvN4%3D&reserved=0
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
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
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,
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
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
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
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
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 --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);
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(+)