[BUG] crypto: export() overran state buffer on test vector

[BUG] crypto: export() overran state buffer on test vector

[Corentin Labbe]

Hello

When working on adding hash support on sun8i-ce, I made a simple version which always fallback.
but booting it lead to this:
[   52.274278] sun8i-ce 1c15000.crypto: Register sha1
[   52.279286] sun8i-ce 1c15000.crypto: sun8i_hash_crainit statesize is 96
[   52.285933] sun8i-ce 1c15000.crypto: Fallback for sha1-sun8i-ce is sha1-ce
[   52.312423] shash_default_export descsize=104
[   52.316021] alg: ahash: sha1-sun8i-ce export() overran state buffer on test vector 0, cfg=\"import/export\" statesize=96
[   52.333189] sun8i-ce 1c15000.crypto: Register sha224
[   52.338387] sun8i-ce 1c15000.crypto: sun8i_hash_crainit statesize is 104
[   52.345097] sun8i-ce 1c15000.crypto: Fallback for sha224-sun8i-ce is sha224-ce
[   52.371865] shash_default_export descsize=112
[   52.375459] alg: ahash: sha224-sun8i-ce export() overran state buffer on test vector 0, cfg=\"import/export\" statesize=104
[   52.393039] sun8i-ce 1c15000.crypto: Register sha256
[   52.398219] sun8i-ce 1c15000.crypto: sun8i_hash_crainit statesize is 104
[   52.404937] sun8i-ce 1c15000.crypto: Fallback for sha256-sun8i-ce is sha256-ce
[   52.431476] shash_default_export descsize=112
[   52.435073] alg: ahash: sha256-sun8i-ce export() overran state buffer on test vector 0, cfg=\"import/export\" statesize=104

For sha1, sha224 and sha256, my driver fail to pass the test.
This is due to the fact that export() (and so shash_async_export/shash_default_export) use crypto_shash_descsize() as length but selftest expect it to be statesize.

Just in case, this is my export code:
int sun8i_hash_crainit(struct crypto_tfm *tfm)
{
        struct sun8i_hash_tfm_ctx *op = crypto_tfm_ctx(tfm);
        struct ahash_alg *alg = __crypto_ahash_alg(tfm->__crt_alg);
        struct sun8i_ce_alg_template *algt;

        memset(op, 0, sizeof(struct sun8i_hash_tfm_ctx));

        crypto_ahash_set_reqsize(__crypto_ahash_cast(tfm), sizeof(struct sun8i_hash_reqctx));

        op->fallback_tfm = crypto_alloc_ahash(crypto_tfm_alg_name(tfm), 0, CRYPTO_ALG_NEED_FALLBACK);
        if (IS_ERR(op->fallback_tfm)) {
                dev_err(algt->ce->dev, "Fallback driver cound no be loaded\n");
                return PTR_ERR(op->fallback_tfm);
        }
        dev_info(op->ce->dev, "%s statesize is %u\n", __func__, algt->alg.hash.halg.statesize);
        dev_info(op->ce->dev, "Fallback for %s is %s\n",
                crypto_tfm_alg_driver_name(tfm),
                crypto_tfm_alg_driver_name(&op->fallback_tfm->base));
        return 0;
}

int sun8i_hash_init(struct ahash_request *areq)
{
        struct sun8i_hash_reqctx *rctx = ahash_request_ctx(areq);
        struct crypto_ahash *tfm = crypto_ahash_reqtfm(areq);
        struct sun8i_hash_tfm_ctx *tfmctx = crypto_ahash_ctx(tfm);

        memset(rctx, 0, sizeof(struct sun8i_hash_reqctx));

        ahash_request_set_tfm(&rctx->fallback_req, tfmctx->fallback_tfm);
        rctx->fallback_req.base.flags = areq->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP;

        return crypto_ahash_init(&rctx->fallback_req);
}

int sun8i_hash_export(struct ahash_request *areq, void *out)
{
        struct sun8i_hash_reqctx *rctx = ahash_request_ctx(areq);
        struct crypto_ahash *tfm = crypto_ahash_reqtfm(areq);
        struct sun8i_hash_tfm_ctx *tfmctx = crypto_ahash_ctx(tfm);

        ahash_request_set_tfm(&rctx->fallback_req, tfmctx->fallback_tfm);
        rctx->fallback_req.base.flags = areq->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP;
                                                                                
        return crypto_ahash_export(&rctx->fallback_req, out);                   
}

Regards

Re: [BUG] crypto: export() overran state buffer on test vector

[Eric Biggers]

On Thu, Feb 06, 2020 at 09:54:42AM +0100, Corentin Labbe wrote:
> Hello
> 
> When working on adding hash support on sun8i-ce, I made a simple version which always fallback.
> but booting it lead to this:
> [   52.274278] sun8i-ce 1c15000.crypto: Register sha1
> [   52.279286] sun8i-ce 1c15000.crypto: sun8i_hash_crainit statesize is 96
> [   52.285933] sun8i-ce 1c15000.crypto: Fallback for sha1-sun8i-ce is sha1-ce
> [   52.312423] shash_default_export descsize=104
> [   52.316021] alg: ahash: sha1-sun8i-ce export() overran state buffer on test vector 0, cfg=\"import/export\" statesize=96
> [   52.333189] sun8i-ce 1c15000.crypto: Register sha224
> [   52.338387] sun8i-ce 1c15000.crypto: sun8i_hash_crainit statesize is 104
> [   52.345097] sun8i-ce 1c15000.crypto: Fallback for sha224-sun8i-ce is sha224-ce
> [   52.371865] shash_default_export descsize=112
> [   52.375459] alg: ahash: sha224-sun8i-ce export() overran state buffer on test vector 0, cfg=\"import/export\" statesize=104
> [   52.393039] sun8i-ce 1c15000.crypto: Register sha256
> [   52.398219] sun8i-ce 1c15000.crypto: sun8i_hash_crainit statesize is 104
> [   52.404937] sun8i-ce 1c15000.crypto: Fallback for sha256-sun8i-ce is sha256-ce
> [   52.431476] shash_default_export descsize=112
> [   52.435073] alg: ahash: sha256-sun8i-ce export() overran state buffer on test vector 0, cfg=\"import/export\" statesize=104
> 
> For sha1, sha224 and sha256, my driver fail to pass the test.
> This is due to the fact that export() (and so shash_async_export/shash_default_export) use crypto_shash_descsize() as length but selftest expect it to be statesize.

That doesn't appear to actually be the problem.  shash_default_export() does
assume descsize == statesize, but it's only used when that's the case.
See shash_prepare_alg().  

> Just in case, this is my export code:
> int sun8i_hash_crainit(struct crypto_tfm *tfm)
> {
>         struct sun8i_hash_tfm_ctx *op = crypto_tfm_ctx(tfm);
>         struct ahash_alg *alg = __crypto_ahash_alg(tfm->__crt_alg);
>         struct sun8i_ce_alg_template *algt;
> 
>         memset(op, 0, sizeof(struct sun8i_hash_tfm_ctx));
> 
>         crypto_ahash_set_reqsize(__crypto_ahash_cast(tfm), sizeof(struct sun8i_hash_reqctx));
> 
>         op->fallback_tfm = crypto_alloc_ahash(crypto_tfm_alg_name(tfm), 0, CRYPTO_ALG_NEED_FALLBACK);
>         if (IS_ERR(op->fallback_tfm)) {
>                 dev_err(algt->ce->dev, "Fallback driver cound no be loaded\n");
>                 return PTR_ERR(op->fallback_tfm);
>         }
>         dev_info(op->ce->dev, "%s statesize is %u\n", __func__, algt->alg.hash.halg.statesize);
>         dev_info(op->ce->dev, "Fallback for %s is %s\n",
>                 crypto_tfm_alg_driver_name(tfm),
>                 crypto_tfm_alg_driver_name(&op->fallback_tfm->base));
>         return 0;
> }
> 
> int sun8i_hash_init(struct ahash_request *areq)
> {
>         struct sun8i_hash_reqctx *rctx = ahash_request_ctx(areq);
>         struct crypto_ahash *tfm = crypto_ahash_reqtfm(areq);
>         struct sun8i_hash_tfm_ctx *tfmctx = crypto_ahash_ctx(tfm);
> 
>         memset(rctx, 0, sizeof(struct sun8i_hash_reqctx));
> 
>         ahash_request_set_tfm(&rctx->fallback_req, tfmctx->fallback_tfm);
>         rctx->fallback_req.base.flags = areq->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP;
> 
>         return crypto_ahash_init(&rctx->fallback_req);
> }
> 
> int sun8i_hash_export(struct ahash_request *areq, void *out)
> {
>         struct sun8i_hash_reqctx *rctx = ahash_request_ctx(areq);
>         struct crypto_ahash *tfm = crypto_ahash_reqtfm(areq);
>         struct sun8i_hash_tfm_ctx *tfmctx = crypto_ahash_ctx(tfm);
> 
>         ahash_request_set_tfm(&rctx->fallback_req, tfmctx->fallback_tfm);
>         rctx->fallback_req.base.flags = areq->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP;
>                                                                                 
>         return crypto_ahash_export(&rctx->fallback_req, out);                   
> }

It seems the actual problem is that you're doing the export using a fallback
algorithm, which may have a statesize different from the one you're setting.

But I'm not sure what you should do here, since the correct statesize can only
be known when a tfm is allocated, not when the algorithm is registered.

Possibly statesize needs to be made a property of the tfm (struct crypto_ahash
and crypto_shash) rather than the algorithm (struct hash_alg_common).

But are you sure you actually need a fallback algorithm for any hash algorithms
in your driver in the first place?

- Eric

Re: [BUG] crypto: export() overran state buffer on test vector

[Corentin Labbe]

On Thu, Feb 06, 2020 at 10:57:19PM -0800, Eric Biggers wrote:
> On Thu, Feb 06, 2020 at 09:54:42AM +0100, Corentin Labbe wrote:
> > Hello
> > 
> > When working on adding hash support on sun8i-ce, I made a simple version which always fallback.
> > but booting it lead to this:
> > [   52.274278] sun8i-ce 1c15000.crypto: Register sha1
> > [   52.279286] sun8i-ce 1c15000.crypto: sun8i_hash_crainit statesize is 96
> > [   52.285933] sun8i-ce 1c15000.crypto: Fallback for sha1-sun8i-ce is sha1-ce
> > [   52.312423] shash_default_export descsize=104
> > [   52.316021] alg: ahash: sha1-sun8i-ce export() overran state buffer on test vector 0, cfg=\"import/export\" statesize=96
> > [   52.333189] sun8i-ce 1c15000.crypto: Register sha224
> > [   52.338387] sun8i-ce 1c15000.crypto: sun8i_hash_crainit statesize is 104
> > [   52.345097] sun8i-ce 1c15000.crypto: Fallback for sha224-sun8i-ce is sha224-ce
> > [   52.371865] shash_default_export descsize=112
> > [   52.375459] alg: ahash: sha224-sun8i-ce export() overran state buffer on test vector 0, cfg=\"import/export\" statesize=104
> > [   52.393039] sun8i-ce 1c15000.crypto: Register sha256
> > [   52.398219] sun8i-ce 1c15000.crypto: sun8i_hash_crainit statesize is 104
> > [   52.404937] sun8i-ce 1c15000.crypto: Fallback for sha256-sun8i-ce is sha256-ce
> > [   52.431476] shash_default_export descsize=112
> > [   52.435073] alg: ahash: sha256-sun8i-ce export() overran state buffer on test vector 0, cfg=\"import/export\" statesize=104
> > 
> > For sha1, sha224 and sha256, my driver fail to pass the test.
> > This is due to the fact that export() (and so shash_async_export/shash_default_export) use crypto_shash_descsize() as length but selftest expect it to be statesize.
> 
> That doesn't appear to actually be the problem.  shash_default_export() does
> assume descsize == statesize, but it's only used when that's the case.
> See shash_prepare_alg().  
> 
> > Just in case, this is my export code:
> > int sun8i_hash_crainit(struct crypto_tfm *tfm)
> > {
> >         struct sun8i_hash_tfm_ctx *op = crypto_tfm_ctx(tfm);
> >         struct ahash_alg *alg = __crypto_ahash_alg(tfm->__crt_alg);
> >         struct sun8i_ce_alg_template *algt;
> > 
> >         memset(op, 0, sizeof(struct sun8i_hash_tfm_ctx));
> > 
> >         crypto_ahash_set_reqsize(__crypto_ahash_cast(tfm), sizeof(struct sun8i_hash_reqctx));
> > 
> >         op->fallback_tfm = crypto_alloc_ahash(crypto_tfm_alg_name(tfm), 0, CRYPTO_ALG_NEED_FALLBACK);
> >         if (IS_ERR(op->fallback_tfm)) {
> >                 dev_err(algt->ce->dev, "Fallback driver cound no be loaded\n");
> >                 return PTR_ERR(op->fallback_tfm);
> >         }
> >         dev_info(op->ce->dev, "%s statesize is %u\n", __func__, algt->alg.hash.halg.statesize);
> >         dev_info(op->ce->dev, "Fallback for %s is %s\n",
> >                 crypto_tfm_alg_driver_name(tfm),
> >                 crypto_tfm_alg_driver_name(&op->fallback_tfm->base));
> >         return 0;
> > }
> > 
> > int sun8i_hash_init(struct ahash_request *areq)
> > {
> >         struct sun8i_hash_reqctx *rctx = ahash_request_ctx(areq);
> >         struct crypto_ahash *tfm = crypto_ahash_reqtfm(areq);
> >         struct sun8i_hash_tfm_ctx *tfmctx = crypto_ahash_ctx(tfm);
> > 
> >         memset(rctx, 0, sizeof(struct sun8i_hash_reqctx));
> > 
> >         ahash_request_set_tfm(&rctx->fallback_req, tfmctx->fallback_tfm);
> >         rctx->fallback_req.base.flags = areq->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP;
> > 
> >         return crypto_ahash_init(&rctx->fallback_req);
> > }
> > 
> > int sun8i_hash_export(struct ahash_request *areq, void *out)
> > {
> >         struct sun8i_hash_reqctx *rctx = ahash_request_ctx(areq);
> >         struct crypto_ahash *tfm = crypto_ahash_reqtfm(areq);
> >         struct sun8i_hash_tfm_ctx *tfmctx = crypto_ahash_ctx(tfm);
> > 
> >         ahash_request_set_tfm(&rctx->fallback_req, tfmctx->fallback_tfm);
> >         rctx->fallback_req.base.flags = areq->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP;
> >                                                                                 
> >         return crypto_ahash_export(&rctx->fallback_req, out);                   
> > }
> 
> It seems the actual problem is that you're doing the export using a fallback
> algorithm, which may have a statesize different from the one you're setting.
> 

Should not be a problem since all stuff before export is done by the fallback.

> But I'm not sure what you should do here, since the correct statesize can only
> be known when a tfm is allocated, not when the algorithm is registered.
> 
> Possibly statesize needs to be made a property of the tfm (struct crypto_ahash
> and crypto_shash) rather than the algorithm (struct hash_alg_common).
> 
> But are you sure you actually need a fallback algorithm for any hash algorithms
> in your driver in the first place?
> 

My goal is to do like n2-crypto/rk3288crypto/etc..., fallback for init/update/final/finup and only do stuff with digest().
So I have just exactly copied what they do.

Regards

Re: [BUG] crypto: export() overran state buffer on test vector

[Herbert Xu]

On Fri, Feb 07, 2020 at 11:46:59AM +0100, Corentin Labbe wrote:
>
> My goal is to do like n2-crypto/rk3288crypto/etc..., fallback for init/update/final/finup and only do stuff with digest().
> So I have just exactly copied what they do.

n2 at least is totally broken wrt import/export.  The other ones
would work provided that the fallback have the same statesize as
the generic sha implementations.

Are you not using the standard state sizes?

This should probably be switched over to lib/crypto or at least
shash.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [BUG] crypto: export() overran state buffer on test vector

[Corentin Labbe]

On Sat, Feb 08, 2020 at 04:57:13PM +0800, Herbert Xu wrote:
> On Fri, Feb 07, 2020 at 11:46:59AM +0100, Corentin Labbe wrote:
> >
> > My goal is to do like n2-crypto/rk3288crypto/etc..., fallback for init/update/final/finup and only do stuff with digest().
> > So I have just exactly copied what they do.
> 
> n2 at least is totally broken wrt import/export.  The other ones
> would work provided that the fallback have the same statesize as
> the generic sha implementations.
> 

This behavour happen only on arm64, so it is why probably nobody (rockchip/n2) found it.

> Are you not using the standard state sizes?

I use the standard size (statesize = sizeof(struct shaxxx_state))

As a quick workaround, By simply adding (+ 8), all test pass.

> 
> This should probably be switched over to lib/crypto or at least
> shash.
> 

Do you mean that I should abandon ahash as a fallback ?

Re: [BUG] crypto: export() overran state buffer on test vector

[Herbert Xu]

On Tue, Feb 11, 2020 at 08:21:18PM +0100, Corentin Labbe wrote:
> 
> Do you mean that I should abandon ahash as a fallback ?

Perhaps switching to shash is not as straightforward because of
SG handling.  But if you're getting problems with statesize
perhaps at least force a sync ahash algorithm might be a good
idea.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [BUG] crypto: export() overran state buffer on test vector

[Corentin Labbe]

On Wed, Feb 12, 2020 at 10:06:28AM +0800, Herbert Xu wrote:
> On Tue, Feb 11, 2020 at 08:21:18PM +0100, Corentin Labbe wrote:
> > 
> > Do you mean that I should abandon ahash as a fallback ?
> 
> Perhaps switching to shash is not as straightforward because of
> SG handling.  But if you're getting problems with statesize
> perhaps at least force a sync ahash algorithm might be a good
> idea.
> 

I just found the problem, it happen with ARM CE which has a bigger state_size (sha256_ce_state) of 4bytes.

Since my driver didnt use statesize at all, I detect this on cra_init() like this:
if (algt->alg.hash.halg.statesize < crypto_ahash_statesize(op->fallback_tfm))
	algt->alg.hash.halg.statesize = crypto_ahash_statesize(op->fallback_tfm);

Regards

Re: [BUG] crypto: export() overran state buffer on test vector

[Herbert Xu]

On Wed, Feb 12, 2020 at 07:57:49PM +0100, Corentin Labbe wrote:
>
> I just found the problem, it happen with ARM CE which has a bigger state_size (sha256_ce_state) of 4bytes.
> 
> Since my driver didnt use statesize at all, I detect this on cra_init() like this:
> if (algt->alg.hash.halg.statesize < crypto_ahash_statesize(op->fallback_tfm))
> 	algt->alg.hash.halg.statesize = crypto_ahash_statesize(op->fallback_tfm);

Thanks for finding this.

I think this can be fixed by simply adding export/imort functions
that exported the sha state without the extra finalize field which
is never used for the exported state (it's only used as an internal
function parameter).

We should also add some tests to ensure that shash SHA algorithms
all use the same geometry for export/import.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt