LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] crypto: AES-NI: fix memory usage in GCM decryption
@ 2015-03-08 18:49 Stephan Mueller
  2015-03-10  9:45 ` Herbert Xu
  0 siblings, 1 reply; 4+ messages in thread
From: Stephan Mueller @ 2015-03-08 18:49 UTC (permalink / raw)
  To: Tadeusz Struk, 'Herbert Xu; +Cc: linux-crypto, linux-kernel

The RFC4106 GCM decryption operation tries to overwrite cryptlen memory
in req->dst. As the destination buffer for decryption only needs to hold
the plaintext memory but cryptlen references the input buffer holding
(ciphertext || authentication tag), the assumption of the destination
buffer length in RFC4106 GCM operation leads to a too large size.

This patch simply subtracts the authentication tag size from cryptlen.
The kernel crypto API logic requires the caller to provide the
length of (ciphertext || authentication tag) as cryptlen for the
AEAD decryption operation. Thus, the cipher implementation must
caculate the size of the plaintext output itself and cannot simply use
cryptlen.

Note, this fixes a kernel crash that can be triggered from user space
via AF_ALG(aead)  without it (simply use the libkcapi test application
from [1] and update it to use rfc4106-gcm-aes).

[1] http://www.chronox.de/libkcapi.html

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

diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
index 6893f49..8f7900e8 100644
--- a/arch/x86/crypto/aesni-intel_glue.c
+++ b/arch/x86/crypto/aesni-intel_glue.c
@@ -1160,7 +1160,8 @@ static int __driver_rfc4106_decrypt(struct aead_request *req)
 		scatterwalk_done(&src_sg_walk, 0, 0);
 		scatterwalk_done(&assoc_sg_walk, 0, 0);
 	} else {
-		scatterwalk_map_and_copy(dst, req->dst, 0, req->cryptlen, 1);
+		scatterwalk_map_and_copy(dst, req->dst, 0,
+					 (req->cryptlen - auth_tag_len), 1);
 		kfree(src);
 	}
 	return retval;
-- 
2.1.0



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] crypto: AES-NI: fix memory usage in GCM decryption
  2015-03-08 18:49 [PATCH] crypto: AES-NI: fix memory usage in GCM decryption Stephan Mueller
@ 2015-03-10  9:45 ` Herbert Xu
  2015-03-11  8:01   ` Stephan Mueller
  0 siblings, 1 reply; 4+ messages in thread
From: Herbert Xu @ 2015-03-10  9:45 UTC (permalink / raw)
  To: Stephan Mueller; +Cc: Tadeusz Struk, linux-crypto, linux-kernel

On Sun, Mar 08, 2015 at 07:49:58PM +0100, Stephan Mueller wrote:
> The RFC4106 GCM decryption operation tries to overwrite cryptlen memory
> in req->dst. As the destination buffer for decryption only needs to hold
> the plaintext memory but cryptlen references the input buffer holding
> (ciphertext || authentication tag), the assumption of the destination
> buffer length in RFC4106 GCM operation leads to a too large size.
> 
> This patch simply subtracts the authentication tag size from cryptlen.
> The kernel crypto API logic requires the caller to provide the
> length of (ciphertext || authentication tag) as cryptlen for the
> AEAD decryption operation. Thus, the cipher implementation must
> caculate the size of the plaintext output itself and cannot simply use
> cryptlen.
> 
> Note, this fixes a kernel crash that can be triggered from user space
> via AF_ALG(aead)  without it (simply use the libkcapi test application
> from [1] and update it to use rfc4106-gcm-aes).
> 
> [1] http://www.chronox.de/libkcapi.html
> 
> CC: Tadeusz Struk <tadeusz.struk@intel.com>
> Signed-off-by: Stephan Mueller <smueller@chronox.de>

Good catch.  However, I think the same confusion exists in the
code that allocates src which may also trigger a crash.  So could
you fix that as well?

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] crypto: AES-NI: fix memory usage in GCM decryption
  2015-03-10  9:45 ` Herbert Xu
@ 2015-03-11  8:01   ` Stephan Mueller
  2015-03-11  9:10     ` Herbert Xu
  0 siblings, 1 reply; 4+ messages in thread
From: Stephan Mueller @ 2015-03-11  8:01 UTC (permalink / raw)
  To: Herbert Xu; +Cc: Tadeusz Struk, linux-crypto, linux-kernel

Am Dienstag, 10. März 2015, 20:45:43 schrieb Herbert Xu:

Hi Herbert,

> On Sun, Mar 08, 2015 at 07:49:58PM +0100, Stephan Mueller wrote:
> > The RFC4106 GCM decryption operation tries to overwrite cryptlen memory
> > in req->dst. As the destination buffer for decryption only needs to hold
> > the plaintext memory but cryptlen references the input buffer holding
> > (ciphertext || authentication tag), the assumption of the destination
> > buffer length in RFC4106 GCM operation leads to a too large size.
> > 
> > This patch simply subtracts the authentication tag size from cryptlen.
> > The kernel crypto API logic requires the caller to provide the
> > length of (ciphertext || authentication tag) as cryptlen for the
> > AEAD decryption operation. Thus, the cipher implementation must
> > caculate the size of the plaintext output itself and cannot simply use
> > cryptlen.
> > 
> > Note, this fixes a kernel crash that can be triggered from user space
> > via AF_ALG(aead)  without it (simply use the libkcapi test application
> > from [1] and update it to use rfc4106-gcm-aes).
> > 
> > [1] http://www.chronox.de/libkcapi.html
> > 
> > CC: Tadeusz Struk <tadeusz.struk@intel.com>
> > Signed-off-by: Stephan Mueller <smueller@chronox.de>
> 
> Good catch.  However, I think the same confusion exists in the
> code that allocates src which may also trigger a crash.  So could
> you fix that as well?

If you are referring to __driver_rfc4106_decrypt, I do not see that confusion 
when src is allocated. src is allocated in the first else branch. src is of 
size cryptlen + assoc len which implies (ciphertext || tag || AAD). As this 
buffer receives a copy of the input data provided by the caller via req->src 
which holds ciphertext || tag, I think the allocation and size of src is 
correct. Note the last line in the else branch: dst = src which means that we 
have an in-place decryption.

However, I think there is an error in the calculation of the AAD pointer 
offset. That offset is currently calculated as:

assoc = (src + req->cryptlen + auth_tag_len);

But instead, it should be:

assoc = (src + req->cryptlen);

as our buffer is only cryptlen + assolen in size.

If you concur, I will write a patch and test it.

If you are referring to the __driver_rfc4106_encrypt function, I cannot find 
an error either for the allocation of src. Although src only holds the 
plaintext, the final dst=src implies that the same buffer shall hold the 
result of the encrypt operation. As the result has the size of ciphertext || 
tag || AAD, I think the buffer has the right size. Note, the calculation of 
the assoc pointer offset is correct IMHO as the room for the newly generated 
auth tag must be preserved. Also, src is sufficiently large to hold the result 
of the encryption operation.


In addition, I just saw an issue with my patch: instead of calculating the 
length of the plaintext with (req->cryptlen - auth_tag_len), I instead should 
use tempCipherLen that already holds the size of the plaintext.

-- 
Ciao
Stephan

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] crypto: AES-NI: fix memory usage in GCM decryption
  2015-03-11  8:01   ` Stephan Mueller
@ 2015-03-11  9:10     ` Herbert Xu
  0 siblings, 0 replies; 4+ messages in thread
From: Herbert Xu @ 2015-03-11  9:10 UTC (permalink / raw)
  To: Stephan Mueller; +Cc: Tadeusz Struk, linux-crypto, linux-kernel

On Wed, Mar 11, 2015 at 09:01:02AM +0100, Stephan Mueller wrote:
>
> However, I think there is an error in the calculation of the AAD pointer 
> offset. That offset is currently calculated as:
> 
> assoc = (src + req->cryptlen + auth_tag_len);
> 
> But instead, it should be:
> 
> assoc = (src + req->cryptlen);
> 
> as our buffer is only cryptlen + assolen in size.
> 
> If you concur, I will write a patch and test it.

Yes that's the problem I was referring to.

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2015-03-11  9:10 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-08 18:49 [PATCH] crypto: AES-NI: fix memory usage in GCM decryption Stephan Mueller
2015-03-10  9:45 ` Herbert Xu
2015-03-11  8:01   ` Stephan Mueller
2015-03-11  9:10     ` Herbert Xu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).