[PATCH] crypto: x86/aes-gcm - fix always true check for last AAD segment

[PATCH] crypto: x86/aes-gcm - fix always true check for last AAD segment

[Mohamad Raizudeen]

In gcm_process_assoc(), a segment that is not the last one must have its
length rounded down to a multiple of 16 bytes, as required by the
assembly. The check intended to detect a non-last segment,
`if (unlikely(assoclen)) /* Not the last segment yet? */` is always
true, since the loop only runs while assoclen is nonzero.

As a result the last segment was rounded down as well, leading to some
avoidable extra work: an additional memcpy into the temporary buffer and
an additional call into the assembly after the loop. The GCM
authentication tag is unaffected either way, so the self-tests pass and
this went unnoticed, its purely an efficiency issue rather than
correctness one.

Fix this by comparing the length of the current step against the amount
of AAD that remains, so that only a step which cannot consume all of the
remaining bytes is treated as a non-last segment.

Fixes: b06affb1cb580 ("crypto: x86/aes-gcm - add VAES and AVX512 / AVX10 optimized AES-GCM")
Signed-off-by: Mohamad Raizudeen <raizudeen.kerneldev@gmail.com>
---
 arch/x86/crypto/aesni-intel_glue.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
index f522fff9231e..f4aae889635a 100644
--- a/arch/x86/crypto/aesni-intel_glue.c
+++ b/arch/x86/crypto/aesni-intel_glue.c
@@ -1305,7 +1305,7 @@ static void gcm_process_assoc(const struct aes_gcm_key *key, u8 ghash_acc[16],
 			pos = 0;
 		}
 		len = len_this_step;
-		if (unlikely(assoclen)) /* Not the last segment yet? */
+		if (unlikely(len < assoclen)) /* Not the last segment yet? */
 			len = round_down(len, 16);
 		aes_gcm_aad_update(key, ghash_acc, src, len, flags);
 		src += len;
-- 
2.53.0

Re: [PATCH] crypto: x86/aes-gcm - fix always true check for last AAD segment

[Eric Biggers]

On Sat, Sep 26, 2026 at 09:36:14AM +0530, Mohamad Raizudeen wrote:
> In gcm_process_assoc(), a segment that is not the last one must have its
> length rounded down to a multiple of 16 bytes, as required by the
> assembly. The check intended to detect a non-last segment,
> `if (unlikely(assoclen)) /* Not the last segment yet? */` is always
> true, since the loop only runs while assoclen is nonzero.
> 
> As a result the last segment was rounded down as well, leading to some
> avoidable extra work: an additional memcpy into the temporary buffer and
> an additional call into the assembly after the loop. The GCM
> authentication tag is unaffected either way, so the self-tests pass and
> this went unnoticed, its purely an efficiency issue rather than
> correctness one.
> 
> Fix this by comparing the length of the current step against the amount
> of AAD that remains, so that only a step which cannot consume all of the
> remaining bytes is treated as a non-last segment.
> 
> Fixes: b06affb1cb580 ("crypto: x86/aes-gcm - add VAES and AVX512 / AVX10 optimized AES-GCM")
> Signed-off-by: Mohamad Raizudeen <raizudeen.kerneldev@gmail.com>
> ---

Thanks for finding this!  Please use the correct Fixes commit and add Cc
stable:

    Fixes: e9787deff49e ("crypto: x86/aes-gcm - use the new scatterwalk functions")
    Cc: stable@vger.kernel.org

> -		if (unlikely(assoclen)) /* Not the last segment yet? */
> +		if (unlikely(len < assoclen)) /* Not the last segment yet? */
> 			len = round_down(len, 16);

Well, this is not correct either.  Let's just move
'assoclen -= orig_len_this_step;' to near the beginning of the loop
body, restoring the original logic prior to e9787deff49e.

- Eric