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

[Eric Biggers <ebiggers@kernel.org>]

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