| Message ID | 22281096.OZkxDCz0xu@tachyon.chronox.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
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,
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.
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,
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;
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(-)