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

[Stephan Mueller <smueller@chronox.de>]

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