[PATCH] crypto: AES-NI: fix memory usage in GCM decryption

[PATCH] crypto: AES-NI: fix memory usage in GCM decryption

[Stephan Mueller]

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

Re: [PATCH] crypto: AES-NI: fix memory usage in GCM decryption

[Herbert Xu]

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

Re: [PATCH] crypto: AES-NI: fix memory usage in GCM decryption

[Stephan Mueller]

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

Re: [PATCH] crypto: AES-NI: fix memory usage in GCM decryption

[Herbert Xu]

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