Bogus sha1 implementation in crypto4xx

Bogus sha1 implementation in crypto4xx

[Herbert Xu]

Hi:

I just noticed that the sha1 implementation in crypto4xx is
fundamentally broken.  It stores the hash state in the context
of the tfm, instead of the context of the request.

This means that at any one time you can only have one entity
using the tfm, which is infeasible for an asynchronous hash.

So I'm going to disable the sha1 part of crypto4xx until this
is fixed.

This hasn't caused a problem before because we haven't started
using ahash yet, apart from tcrypt which is single-threaded.
I'm currently in the process of converting authenc (hence IPsec)
across, which means that we will soon rely on the fact that
you can have multiple hash operations ongoing at once.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Bogus sha1 implementation in crypto4xx

[Steffen Klassert]

On Tue, Jul 14, 2009 at 09:06:00PM +0800, Herbert Xu wrote:
> 
> This hasn't caused a problem before because we haven't started
> using ahash yet, apart from tcrypt which is single-threaded.
> I'm currently in the process of converting authenc (hence IPsec)
> across, which means that we will soon rely on the fact that
> you can have multiple hash operations ongoing at once.
> 

As I pointed out already, I'm working on converting IPsec to ahash too.
So perhaps we can share the work to do not all the work twice.
I have ahash versions of authenc and ah4/ah6. The authenc ahash version
needs a hack in esp to be able to chain all scatterlist in authenc and I
wanted to look over it again before I post. 

Today I finished with ah4/ah6. This needs still some tests but I could post
these two in the next days if you have not converted them it yet.

Re: Bogus sha1 implementation in crypto4xx

[Herbert Xu]

On Tue, Jul 14, 2009 at 03:46:29PM +0200, Steffen Klassert wrote:
>
> As I pointed out already, I'm working on converting IPsec to ahash too.
> So perhaps we can share the work to do not all the work twice.
> I have ahash versions of authenc and ah4/ah6. The authenc ahash version
> needs a hack in esp to be able to chain all scatterlist in authenc and I
> wanted to look over it again before I post. 
> 
> Today I finished with ah4/ah6. This needs still some tests but I could post
> these two in the next days if you have not converted them it yet.

Sure, go ahead.  I'm still redoing the ahash infrastructure to
properly support finup and export/import.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Bogus sha1 implementation in crypto4xx

[Steffen Klassert]

On Tue, Jul 14, 2009 at 10:43:44PM +0800, Herbert Xu wrote:
> 
> Sure, go ahead.  I'm still redoing the ahash infrastructure to
> properly support finup and export/import.
> 

Ok, for a moment I thought we both working at the same thing :)
I'll finalize my work and send everything in the next days.

Thanks for clarification.

Re: Bogus sha1 implementation in crypto4xx

[Herbert Xu]

On Wed, Jul 15, 2009 at 07:37:33AM +0200, Steffen Klassert wrote:
>
> Ok, for a moment I thought we both working at the same thing :)
> I'll finalize my work and send everything in the next days.

I'm pretty much done with the ahash rework so whenever you're
ready please send the authenc conversion out.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

RE: Bogus sha1 implementation in crypto4xx

[Shasi Pulijala]

Hi Herbert,

>>I just noticed that the sha1 implementation in crypto4xx is
>>fundamentally broken.  It stores the hash state in the context
>>of the tfm, instead of the context of the request.

>>This means that at any one time you can only have one entity
>>using the tfm, which is infeasible for an asynchronous hash.

I am assuming you are talking about the final hash result being stored in the ctx of the tfm and not the intermediate hash state. 
In any case I think we should be safe here since we have a unique packet descriptor per request.
Each packet descriptor has a pointer for SA which holds a pointer for State record. This state record holds the IV and the final hash state.
In the driver’s crypto4xx_build_pd function we get a unique packet descriptor per request and a unique SA pointer location and a unique state record pointer.
The tfm’s context SA is now copied into the packet descriptor’s SA pointer and here we also assign the state record pointer to the SA.

                if (iv_len || ctx->is_hash) {
                                ivlen = iv_len;
                                pd->sa = pd_uinfo->sa_pa;
                                sa = (struct dynamic_sa_ctl *) pd_uinfo->sa_va;
                                if (ctx->direction == DIR_INBOUND)
                                                memcpy(sa, ctx->sa_in, ctx->sa_len * 4);
                                else
                                                memcpy(sa, ctx->sa_out, ctx->sa_len * 4);

                                memcpy((void *) sa + ctx->offset_to_sr_ptr,
                                                &pd_uinfo->sr_pa, 4);

If there exists an IV we then copy it to the state record. 

                                if (iv_len)
                                                crypto4xx_memcpy_le(pd_uinfo->sr_va, iv, iv_len);

The above applies for digest functions for hash algorithms.

For init/update/final operations in the init function we save the initial digest to the tfm ctx. We follow the same the procedure for update as in digest operations as well, where in we create a separate copy of the tfm ctx SA in the unique packet descriptor, and the result of each update function is copied into the tfm ctx SA ‘s initial digest again to be used for the next operation. So even for these operations we should be safe.

I hope we both are seeing the same problem here, if not it would be great if you could explain what's the exact problem that you are seeing.

Thanks,
-Shasi

-----Original Message-----
From: Herbert Xu [mailto:herbert@gondor.apana.org.au] 
Sent: Tuesday, July 14, 2009 6:06 AM
To: Rina Kagan; Shasi Pulijala
Cc: Linux Crypto Mailing List
Subject: Bogus sha1 implementation in crypto4xx

Hi:

I just noticed that the sha1 implementation in crypto4xx is
fundamentally broken.  It stores the hash state in the context
of the tfm, instead of the context of the request.

This means that at any one time you can only have one entity
using the tfm, which is infeasible for an asynchronous hash.

So I'm going to disable the sha1 part of crypto4xx until this
is fixed.

This hasn't caused a problem before because we haven't started
using ahash yet, apart from tcrypt which is single-threaded.
I'm currently in the process of converting authenc (hence IPsec)
across, which means that we will soon rely on the fact that
you can have multiple hash operations ongoing at once.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bogus sha1 implementation in crypto4xx

[Herbert Xu]

On Wed, Jul 29, 2009 at 03:58:10PM -0700, Shasi Pulijala wrote:
>
> I am assuming you are talking about the final hash result being stored in the ctx of the tfm and not the intermediate hash state. 

No I mean the intermediate hash state.

All crypto hash implementations must support

	crypto_ahash_init(req1)
	crypto_ahash_init(req2)

	crypto_ahash_update(req1)
	crypto_ahash_update(req2)

	crypto_ahash_final(req1)
	crypto_ahash_final(req2)

Of course there may be an unlimited number of requests and these
may be invoked from different CPUs.  Therefore you must store the
intermediate hash result in the request itself.

If your particular device does not support the export of the
intermediate state, then you must fall back to using a software
hash for everything up until the last finup/final request.  See
drivers/crypto/padlock-sha.c for an example.  It's shash but the
same principle applies to ahash.

Only the old hash/digest interface is limited to one request per
tfm at any time.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt