Re: [PATCH 04/35] crypto: Add aesenc_SB_SR

From: Richard Henderson <richard.henderson@linaro.org>
To: Ard Biesheuvel <ardb@kernel.org>
Cc: qemu-devel@nongnu.org, berrange@redhat.com, qemu-arm@nongnu.org,
	qemu-ppc@nongnu.org, qemu-riscv@nongnu.org, pbonzini@redhat.com
Subject: Re: [PATCH 04/35] crypto: Add aesenc_SB_SR
Date: Sat, 3 Jun 2023 08:24:20 -0700	[thread overview]
Message-ID: <d7268399-c424-fd84-9323-df0d2c15db3d@linaro.org> (raw)
In-Reply-To: <CAMj1kXFuESELf9gjsBBKOtZKLxM2cyuzBecQm4+KHT4XtiRLhA@mail.gmail.com>

On 6/3/23 06:15, Ard Biesheuvel wrote:
>> diff --git a/crypto/aes.c b/crypto/aes.c
>> index 1309a13e91..708838315a 100644
>> --- a/crypto/aes.c
>> +++ b/crypto/aes.c
>> @@ -29,6 +29,7 @@
>>    */
>>   #include "qemu/osdep.h"
>>   #include "crypto/aes.h"
>> +#include "crypto/aes-round.h"
>>
>>   typedef uint32_t u32;
>>   typedef uint8_t u8;
>> @@ -1251,6 +1252,52 @@ static const u32 rcon[] = {
>>           0x1B000000, 0x36000000, /* for 128-bit blocks, Rijndael never uses more than 10 rcon values */
>>   };
>>
>> +/* Perform SubBytes + ShiftRows. */
>> +static inline void
>> +aesenc_SB_SR_swap(AESState *r, const AESState *st, bool swap)
>> +{
>> +    const int swap_b = swap ? 15 : 0;
>> +    uint8_t t;
>> +
>> +    /* These four indexes are not swizzled. */
>> +    r->b[swap_b ^ 0x0] = AES_sbox[st->b[swap_b ^ AES_SH_0]];
>> +    r->b[swap_b ^ 0x4] = AES_sbox[st->b[swap_b ^ AES_SH_4]];
>> +    r->b[swap_b ^ 0x8] = AES_sbox[st->b[swap_b ^ AES_SH_8]];
>> +    r->b[swap_b ^ 0xc] = AES_sbox[st->b[swap_b ^ AES_SH_C]];
>> +
>> +    /* Otherwise, break cycles. */
>> +
> 
> This is only needed it r == st, right?

Yes.  This is, perhaps, where using symbolic AES_SH_X while assuming knowledge of the 
value does not aid understanding.

r~

> 
>> +    t = AES_sbox[st->b[swap_b ^ AES_SH_D]];
>> +    r->b[swap_b ^ 0x1] = AES_sbox[st->b[swap_b ^ AES_SH_1]];
>> +    r->b[swap_b ^ 0x5] = AES_sbox[st->b[swap_b ^ AES_SH_5]];
>> +    r->b[swap_b ^ 0x9] = AES_sbox[st->b[swap_b ^ AES_SH_9]];
>> +    r->b[swap_b ^ 0xd] = t;
>> +
>> +    t = AES_sbox[st->b[swap_b ^ AES_SH_A]];
>> +    r->b[swap_b ^ 0x2] = AES_sbox[st->b[swap_b ^ AES_SH_2]];
>> +    r->b[swap_b ^ 0xa] = t;
>> +
>> +    t = AES_sbox[st->b[swap_b ^ AES_SH_E]];
>> +    r->b[swap_b ^ 0x6] = AES_sbox[st->b[swap_b ^ AES_SH_6]];
>> +    r->b[swap_b ^ 0xe] = t;
>> +
>> +    t = AES_sbox[st->b[swap_b ^ AES_SH_7]];
>> +    r->b[swap_b ^ 0x3] = AES_sbox[st->b[swap_b ^ AES_SH_3]];
>> +    r->b[swap_b ^ 0xf] = AES_sbox[st->b[swap_b ^ AES_SH_F]];
>> +    r->b[swap_b ^ 0xb] = AES_sbox[st->b[swap_b ^ AES_SH_B]];
>> +    r->b[swap_b ^ 0x7] = t;
>> +}
>> +
>> +void aesenc_SB_SR_gen(AESState *r, const AESState *st)
>> +{
>> +    aesenc_SB_SR_swap(r, st, false);
>> +}
>> +
>> +void aesenc_SB_SR_genrev(AESState *r, const AESState *st)
>> +{
>> +    aesenc_SB_SR_swap(r, st, true);
>> +}
>> +
>>   /**
>>    * Expand the cipher key into the encryption key schedule.
>>    */
>> --
>> 2.34.1
>>