On 29/04/25 14:19, Herbert Xu wrote:
> [...]
> +
> +static int s390_hmac_export_sha256(struct shash_desc *desc, void *out)
> +{
> + struct s390_kmac_sha2_ctx *ctx = shash_desc_ctx(desc);
> + u64 total = ctx->buflen[0];
> + union {
> + u8 *u8;
> + u64 *u64;
> + } p = { .u8 = out };
> + unsigned int remain;
> + u64 hashed;
> + int err = 0;
> +
> + hashed = round_down(total, SHA256_BLOCK_SIZE);
> + remain = total - hashed;
> +
> + if (!hashed)
> + err = s390_hmac_export_zero(desc, out);
> + else
> + memcpy(p.u8, ctx->param, SHA256_DIGEST_SIZE);
> +
> + p.u8 += SHA256_DIGEST_SIZE;
> + put_unaligned(total, p.u64++);
> +
> + memcpy(p.u8, ctx->buf, remain);
> +
> + return err;
> +}
Why do pointer increment with different types through a union which is un-intuitive to understand and prone to easy errors in future. It is easy to mix up the layout of the data being stored. Why not just typecast void * to a struct exposing different fields? Same with sha512.
> +
> + [...]
> +
> +static int s390_hmac_export_sha512(struct shash_desc *desc, void *out)
> +{
> + struct s390_kmac_sha2_ctx *ctx = shash_desc_ctx(desc);
> + u64 total_hi = ctx->buflen[1];
> + u64 total = ctx->buflen[0];
Can use uniform naming here. total_hi and total_lo.
Regards
T Pratham <t-pratham@xxxxxx>
