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

From: Stephan Mueller
Date: Wed Mar 11 2015 - 04:48:57 EST


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@xxxxxxxxx>
> > Signed-off-by: Stephan Mueller <smueller@xxxxxxxxxx>
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/