Re: [PATCH 1/4] xen/lib: Export additional sha256 functions

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/05/2025 6:26 am, Frediano Ziglio wrote:
> On Tue, May 6, 2025 at 11:17 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 
> wrote:
>> On 06/05/2025 3:05 pm, Andrew Cooper wrote:
>>> On 06/05/2025 2:56 pm, Frediano Ziglio wrote:
>>>> diff --git a/xen/include/xen/sha2.h b/xen/include/xen/sha2.h
>>>> index 47d97fbf01..ea8bad67e4 100644
>>>> --- a/xen/include/xen/sha2.h
>>>> +++ b/xen/include/xen/sha2.h
>>>> @@ -9,6 +9,16 @@
>>>>
>>>>  #define SHA2_256_DIGEST_SIZE 32
>>>>
>>>> +struct sha2_256_state {
>>>> +    uint32_t state[SHA2_256_DIGEST_SIZE / sizeof(uint32_t)];
>>>> +    uint8_t buf[64];
>>>> +    size_t count; /* Byte count. */
>>>> +};
>>>> +
>>>> +void sha2_256_init(struct sha2_256_state *s);
>>>> +void sha2_256_update(struct sha2_256_state *s, const void *msg,
>>>> +                     size_t len);
>>>> +void sha2_256_final(struct sha2_256_state *s, void *_dst);
>>>>  void sha2_256_digest(uint8_t digest[SHA2_256_DIGEST_SIZE],
>>>>                       const void *msg, size_t len);
>>> sha2_256_digest() is unlike the others as it holds sha2_256_state
>>> internally.  I'd suggest having all of the additions below this point,
>>> which group them more nicely.
>>>
>>> Can fix on commit.  Otherwise LGTM.
>> Not quite.  Now that sha2_256_final() is exported, it should have a
>> proper type for  _dst.  I've folded:
>>
>> diff --git a/xen/include/xen/sha2.h b/xen/include/xen/sha2.h
>> index 0d55fe640431..cb30e8f8d77c 100644
>> --- a/xen/include/xen/sha2.h
>> +++ b/xen/include/xen/sha2.h
>> @@ -21,6 +21,7 @@ struct sha2_256_state {
>>  void sha2_256_init(struct sha2_256_state *s);
>>  void sha2_256_update(struct sha2_256_state *s, const void *msg,
>>                       size_t len);
>> -void sha2_256_final(struct sha2_256_state *s, void *_dst);
>> +void sha2_256_final(struct sha2_256_state *s,
>> +                    uint8_t digest[SHA2_256_DIGEST_SIZE]);
>>
>>  #endif /* XEN_SHA2_H */
>> diff --git a/xen/lib/sha2-256.c b/xen/lib/sha2-256.c
>> index 896a257ed9b7..08ef7011a1c3 100644
>> --- a/xen/lib/sha2-256.c
>> +++ b/xen/lib/sha2-256.c
>> @@ -171,9 +171,9 @@ void sha2_256_update(struct sha2_256_state *s, const
>> void *msg,
>>      memcpy(s->buf + partial, msg, len);
>>  }
>>
>> -void sha2_256_final(struct sha2_256_state *s, void *_dst)
>> +void sha2_256_final(struct sha2_256_state *s, uint8_t
>> digest[SHA2_256_DIGEST_SIZE])
>>  {
>> -    uint32_t *dst = _dst;
>> +    uint32_t *dst = (uint32_t *)digest;
> That is we are never going to support architectures with unaligned
> memory access.

Note how it's written to.

    /* Store state in digest */
    for ( i = 0; i < 8; i++ )
        put_unaligned_be32(s->state[i], &dst[i]);

It works just fine on misaligned buffers.

~Andrew