[PATCH] crypto: aesni: add setkey for driver-gcm-aes-aesni

[PATCH] crypto: aesni: add setkey for driver-gcm-aes-aesni

[Stephan Mueller]

The cipher registered as __driver-gcm-aes-aesni is never intended
to be used directly by any caller. Instead it is a service mechanism to
rfc4106-gcm-aesni.

The kernel crypto API unconditionally calls the registered setkey
function. In case a caller erroneously uses __driver-gcm-aes-aesni a
call to crypto_aead_setkey will cause a NULL pointer dereference without
this patch.

CC: Tadeusz Struk <tadeusz.struk@intel.com>
Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 arch/x86/crypto/aesni-intel_glue.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
index 947c6bf..a278ef9 100644
--- a/arch/x86/crypto/aesni-intel_glue.c
+++ b/arch/x86/crypto/aesni-intel_glue.c
@@ -1012,6 +1012,16 @@ static int rfc4106_decrypt(struct aead_request *req)
 	}
 }
 
+static int __driver_rfc4106_set_key(struct crypto_aead *parent,
+				    const u8 *key, unsigned int key_len)
+{
+	/*
+	 * __driver-gcm-aes-aesni is only a backend for rfc4106-gcm-aesni
+	 * and is never intended to be used as a regular cipher.
+	 */
+	return -EOPNOTSUPP;
+}
+
 static int __driver_rfc4106_encrypt(struct aead_request *req)
 {
 	u8 one_entry_in_sg = 0;
@@ -1366,6 +1376,7 @@ static struct crypto_alg aesni_algs[] = { {
 	.cra_module		= THIS_MODULE,
 	.cra_u = {
 		.aead = {
+			.setkey		= __driver_rfc4106_set_key,
 			.encrypt	= __driver_rfc4106_encrypt,
 			.decrypt	= __driver_rfc4106_decrypt,
 		},
-- 
2.1.0

Re: [PATCH] crypto: aesni: add setkey for driver-gcm-aes-aesni

[Stephan Mueller]

Am Sonntag, 18. Januar 2015, 23:56:03 schrieb Stephan Mueller:

Hi Tadeusz,

> The cipher registered as __driver-gcm-aes-aesni is never intended
> to be used directly by any caller. Instead it is a service mechanism to
> rfc4106-gcm-aesni.
> 
> The kernel crypto API unconditionally calls the registered setkey
> function. In case a caller erroneously uses __driver-gcm-aes-aesni a
> call to crypto_aead_setkey will cause a NULL pointer dereference without
> this patch.

I tested that patch and can confirm that this patch fixes the kernel crash 
triggered through the AF_ALG interface for AEAD ciphers that is currently 
under development reported earlier. 
> 
> CC: Tadeusz Struk <tadeusz.struk@intel.com>
> Signed-off-by: Stephan Mueller <smueller@chronox.de>
> ---
>  arch/x86/crypto/aesni-intel_glue.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/x86/crypto/aesni-intel_glue.c
> b/arch/x86/crypto/aesni-intel_glue.c index 947c6bf..a278ef9 100644
> --- a/arch/x86/crypto/aesni-intel_glue.c
> +++ b/arch/x86/crypto/aesni-intel_glue.c
> @@ -1012,6 +1012,16 @@ static int rfc4106_decrypt(struct aead_request *req)
>  	}
>  }
> 
> +static int __driver_rfc4106_set_key(struct crypto_aead *parent,
> +				    const u8 *key, unsigned int key_len)
> +{
> +	/*
> +	 * __driver-gcm-aes-aesni is only a backend for rfc4106-gcm-aesni
> +	 * and is never intended to be used as a regular cipher.
> +	 */
> +	return -EOPNOTSUPP;
> +}
> +
>  static int __driver_rfc4106_encrypt(struct aead_request *req)
>  {
>  	u8 one_entry_in_sg = 0;
> @@ -1366,6 +1376,7 @@ static struct crypto_alg aesni_algs[] = { {
>  	.cra_module		= THIS_MODULE,
>  	.cra_u = {
>  		.aead = {
> +			.setkey		= __driver_rfc4106_set_key,
>  			.encrypt	= __driver_rfc4106_encrypt,
>  			.decrypt	= __driver_rfc4106_decrypt,
>  		},


-- 
Ciao
Stephan

Re: [PATCH] crypto: aesni: add setkey for driver-gcm-aes-aesni

[Tadeusz Struk]

On 01/18/2015 02:56 PM, Stephan Mueller wrote:
> The cipher registered as __driver-gcm-aes-aesni is never intended
> to be used directly by any caller. Instead it is a service mechanism to
> rfc4106-gcm-aesni.
> 
> The kernel crypto API unconditionally calls the registered setkey
> function. In case a caller erroneously uses __driver-gcm-aes-aesni a
> call to crypto_aead_setkey will cause a NULL pointer dereference without
> this patch.

Acked-by: Tadeusz Struk <tadeusz.struk@intel.com>

Re: [PATCH] crypto: aesni: add setkey for driver-gcm-aes-aesni

[Herbert Xu]

On Sun, Jan 18, 2015 at 11:56:03PM +0100, Stephan Mueller wrote:
> The cipher registered as __driver-gcm-aes-aesni is never intended
> to be used directly by any caller. Instead it is a service mechanism to
> rfc4106-gcm-aesni.
> 
> The kernel crypto API unconditionally calls the registered setkey
> function. In case a caller erroneously uses __driver-gcm-aes-aesni a
> call to crypto_aead_setkey will cause a NULL pointer dereference without
> this patch.
> 
> CC: Tadeusz Struk <tadeusz.struk@intel.com>
> Signed-off-by: Stephan Mueller <smueller@chronox.de>

Rather than adding a bogus setkey function, please fix this mess
properly by moving the top-level setkey function into the __driver
one where it should be.  Compare with how we handle it in the
ablk_helper which is pretty much the same thing.

Thanks,
-- 
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: [PATCH] crypto: aesni: add setkey for driver-gcm-aes-aesni

[Stephan Mueller]

Am Dienstag, 20. Januar 2015, 14:17:04 schrieb Herbert Xu:

Hi Herbert,

>On Sun, Jan 18, 2015 at 11:56:03PM +0100, Stephan Mueller wrote:
>> The cipher registered as __driver-gcm-aes-aesni is never intended
>> to be used directly by any caller. Instead it is a service mechanism
>> to rfc4106-gcm-aesni.
>> 
>> The kernel crypto API unconditionally calls the registered setkey
>> function. In case a caller erroneously uses __driver-gcm-aes-aesni a
>> call to crypto_aead_setkey will cause a NULL pointer dereference
>> without this patch.
>> 
>> CC: Tadeusz Struk <tadeusz.struk@intel.com>
>> Signed-off-by: Stephan Mueller <smueller@chronox.de>
>
>Rather than adding a bogus setkey function, please fix this mess
>properly by moving the top-level setkey function into the __driver
>one where it should be.  Compare with how we handle it in the
>ablk_helper which is pretty much the same thing.

That is a good suggestion. And the modification is quite limited as the 
existing rfc4106_set_key could be used for the __driver with only slight 
modifications.

In that case, however, we should apply the same to rfc4106_set_authsize.

This in turn would then turn the __driver implementation into a full GCM 
implementation. That would mean that we should rename it from __driver 
into gcm(aes) / gcm-aesni. 
>
>Thanks,


Ciao
Stephan

Re: [PATCH] crypto: aesni: add setkey for driver-gcm-aes-aesni

[Herbert Xu]

On Tue, Jan 20, 2015 at 04:35:41AM +0100, Stephan Mueller wrote:
>
> This in turn would then turn the __driver implementation into a full GCM 
> implementation. That would mean that we should rename it from __driver 
> into gcm(aes) / gcm-aesni. 

No you shouldn't because it'll fail in interrupt context where
you cannot use those special instructions.

The whole point of this setup is to use accelerated instructions
where possible, and otherwise fall back to a separate thread
where we can do so safely.

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: [PATCH] crypto: aesni: add setkey for driver-gcm-aes-aesni

[Stephan Mueller]

Am Dienstag, 20. Januar 2015, 14:37:05 schrieb Herbert Xu:

Hi Herbert,

>On Tue, Jan 20, 2015 at 04:35:41AM +0100, Stephan Mueller wrote:
>> This in turn would then turn the __driver implementation into a full
>> GCM implementation. That would mean that we should rename it from
>> __driver into gcm(aes) / gcm-aesni.
>
>No you shouldn't because it'll fail in interrupt context where
>you cannot use those special instructions.

How would the fail manifest itself? If algif_aead would be present, user 
space could use the __driver implementation regardless of a setkey or 
authsize callback by simply calling encrypt/decrypt. Would the error be 
limited to that caller only?
>
>The whole point of this setup is to use accelerated instructions
>where possible, and otherwise fall back to a separate thread
>where we can do so safely.

Thanks for clarification.
>
>Cheers,


Ciao
Stephan

Re: [PATCH] crypto: aesni: add setkey for driver-gcm-aes-aesni

[Herbert Xu]

On Tue, Jan 20, 2015 at 04:54:44AM +0100, Stephan Mueller wrote:
>
> How would the fail manifest itself? If algif_aead would be present, user 
> space could use the __driver implementation regardless of a setkey or 
> authsize callback by simply calling encrypt/decrypt. Would the error be 
> limited to that caller only?

For user-space it'll never fail.  However, for kernel users such
as IPsec it'll fail if the interrupt occurs while in a thread
that's already using the FPU.

But you're right, we probably shouldn't allow these algorithms to
be directly exported to user-space at all, even when they do possess
a setkey function.  In fact, we should ban them from other places
where they might be used too, such as through IPsec.

I'll try to write something up to do this

Thanks,
-- 
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: [PATCH] crypto: aesni: add setkey for driver-gcm-aes-aesni

[Stephan Mueller]

Am Dienstag, 20. Januar 2015, 14:17:04 schrieb Herbert Xu:

Hi Tadeusz,

> On Sun, Jan 18, 2015 at 11:56:03PM +0100, Stephan Mueller wrote:
> > The cipher registered as __driver-gcm-aes-aesni is never intended
> > to be used directly by any caller. Instead it is a service mechanism to
> > rfc4106-gcm-aesni.
> > 
> > The kernel crypto API unconditionally calls the registered setkey
> > function. In case a caller erroneously uses __driver-gcm-aes-aesni a
> > call to crypto_aead_setkey will cause a NULL pointer dereference without
> > this patch.
> > 
> > CC: Tadeusz Struk <tadeusz.struk@intel.com>
> > Signed-off-by: Stephan Mueller <smueller@chronox.de>
> 
> Rather than adding a bogus setkey function, please fix this mess
> properly by moving the top-level setkey function into the __driver
> one where it should be.  Compare with how we handle it in the
> ablk_helper which is pretty much the same thing.

Tadeusz, are you working on that update or shall I have a look?

-- 
Ciao
Stephan

Re: [PATCH] crypto: aesni: add setkey for driver-gcm-aes-aesni

[Tadeusz Struk]

On 01/20/2015 05:25 PM, Stephan Mueller wrote:
>> Rather than adding a bogus setkey function, please fix this mess
>> properly by moving the top-level setkey function into the __driver
>> one where it should be.  Compare with how we handle it in the
>> ablk_helper which is pretty much the same thing.
> 
> Tadeusz, are you working on that update or shall I have a look?
> 
Hi,
No, I thought that the agreement was that we don't want to allow user
space to use these helpers directly, right? Am I missing something?

Re: [PATCH] crypto: aesni: add setkey for driver-gcm-aes-aesni

[Stephan Mueller]

Am Donnerstag, 22. Januar 2015, 10:23:57 schrieb Tadeusz Struk:

Hi Tadeusz,

>On 01/20/2015 05:25 PM, Stephan Mueller wrote:
>>> Rather than adding a bogus setkey function, please fix this mess
>>> properly by moving the top-level setkey function into the __driver
>>> one where it should be.  Compare with how we handle it in the
>>> ablk_helper which is pretty much the same thing.
>> 
>> Tadeusz, are you working on that update or shall I have a look?
>
>Hi,
>No, I thought that the agreement was that we don't want to allow user
>space to use these helpers directly, right? Am I missing something?


That would be correct. But if I understood Herbert correctly, he is 
creating a patch that disables these service ciphers for general usage.

Ciao
Stephan

Re: [PATCH] crypto: aesni: add setkey for driver-gcm-aes-aesni

[Tadeusz Struk]

On 01/22/2015 01:20 PM, Stephan Mueller wrote:
> That would be correct. But if I understood Herbert correctly, he is 
> creating a patch that disables these service ciphers for general usage.

Yes, and this should also implicitly fix the problem with user space.
Thanks,
Tadeusz

Re: [PATCH] crypto: aesni: add setkey for driver-gcm-aes-aesni

[Herbert Xu]

On Thu, Jan 22, 2015 at 10:23:57AM -0800, Tadeusz Struk wrote:
>
> No, I thought that the agreement was that we don't want to allow user
> space to use these helpers directly, right? Am I missing something?

Yes but we should also fix this so that it's a proper aead
algorithm.

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: [PATCH] crypto: aesni: add setkey for driver-gcm-aes-aesni

[Tadeusz Struk]

On 01/22/2015 02:23 PM, Herbert Xu wrote:
> Yes but we should also fix this so that it's a proper aead
> algorithm.
Ok, I'll do that.
Thanks,
Tadeusz