Re: [PATCH RFC 1/2] crypto: ti: Add support for SHA224/256/384/512 in DTHE V2 driver

[T Pratham <t-pratham@ti.com>]

Hi Herbert

Thanks for helping out here. I modified import/export (albeit with a few
changes/quirks; discussed below) and all self-tests are passing.

On 03/04/25 14:05, Herbert Xu wrote:
> On Thu, Apr 03, 2025 at 01:58:47PM +0530, T Pratham wrote:
>> I'm so sorry, for it slipped out of my mind that `u8 phash_available`
>> also needs to be restored at import. It's just stores a boolean 0/1. How
>> to go about handling this?
> You should be able to derive that from digestcnt.  IOW if you have
> previously submitted data to the hardware, then phash is available,
> and vice versa.
I am able to derive this from digestcnt. This is working.
>
> Note that if you go down this route (which many drivers do), then
> you're going to need to initialise a zero hash partial state in
> the export function like this:
>
> static int ahash_export_zero(struct ahash_request *req, void *out)
> {
> 	HASH_FBREQ_ON_STACK(fbreq, req);
>  
> 	return crypto_ahash_init(fbreq) ?:
> 	       crypto_ahash_export(fbreq, out);
> }
Although, I was not able to quite understand what you meant to imply
from this snippet. And I was not able to find any references for
HASH_FBREQ_ON_STACK as well. Overall, it was not clear why such a fbreq
is required and where it is being used. Hence I omitted this part
completely, and still passing all tests. Would love to know if you have
any good reason to what you suggested.
> Cheers,

Another thing, the buflen variable ranges from 0 to BLOCK_SIZE, not
(BLOCK_SIZE - 1). This is being used to handle certain quirks of the
hardware together with linux crypto framework, which I am happy to
elaborate further if required. Cutting the digression short, I have to
find a workaround to comply with your import/export changes:

tl;dr: I'm storing buflen - 1 if buflen != 0. To differentiate b/w
buflen = 0 and buflen = 1 in import, I am storing a flag in buf[1] if
buflen is either 0 or 1. Code (simplified for brevity) follows.

static int dthe_sha256_export(struct ahash_request *req, void *out)
{
    [...]
    struct sha256_state *state = out;

    if (buflen > 0) {
        state->count = digestcnt + (buflen - 1);
        memcpy(state->buf, data_buf, buflen);
        if (buflen == 1)
            state->buf[1] = 1;
    } else {
        state->count = digestcnt;
        state->buf[1] = 0;
    }
    memcpy(state->state, phash, phash_size);

    return 0;
}

static int dthe_sha256_import(struct ahash_request *req, const void *in)
{
    [...]
    const struct sha256_state *state = in;

    buflen = state->count & (SHA256_BLOCK_SIZE - 1);
    digestcnt = state->count - buflen;
    if (buflen == 0) {
        if (state->buf[1])
            buflen = 1;
    } else {
        buflen++;
    }
    [...]
    memcpy(phash, state->state, phash_size);
    memcpy(data_buf, state->buf, buflen);
    phash_available = ((digestcnt) ? 1 : 0);

    return 0;
}

I'm not exactly sure what effects, in any, this would have if this is
exported to a software implementation in some extraordinary error case.
But this is what I could think of to handle my case and would like to
know if this is an issue with software implementation. I would also love
to know how you're migrating other drivers which are storing more data
in their states than the struct sha256_state /sha512_state can store.

Regards
T Pratham <t-pratham@ti.com>