Crypto test results unused?

Crypto test results unused?

[Krzysztof Halasa]

Hi,

is the core crypto code supposed to "kill" algorithms which fail the
test?

On little-endian IXP4xx 3 hardware-assisted algorithms fail (due to
apparently unrelated bug which I will take care of). It seems the kernel
is still using these failing algorithms (my debugging code adds extra
fields to the /proc output):

alg: skcipher: Test 1 failed on encryption for ecb(des)-ixp4xx
00000000: 01 23 45 67 89 ab cd e7
alg: skcipher: Test 1 failed on encryption for ecb(des3_ede)-ixp4xx
00000000: 73 6f 6d 65 64 61 74 61
alg: skcipher: Test 1 failed on encryption for ecb(aes)-ixp4xx
00000000: 00 11 22 33 44 55 66 77 88 99 aa bb cc dd ee ff

# grep 'ecb(des)-ixp4xx\|ecb(des3_ede)-ixp4xx\|ecb(aes)-ixp4xx' /proc/cryp
to -A 6
driver       : ecb(aes)-ixp4xx
module       : ixp4xx_crypto
priority     : 300
refcnt       : 1
flags        : 0x85
ptr          : 0xbf020074
selftest     : unknown
^^^^^^^^^^^^^^^^^^^^^^
--
driver       : ecb(des3_ede)-ixp4xx
module       : ixp4xx_crypto
priority     : 300
refcnt       : 1
flags        : 0x85
ptr          : 0xbf01fe94
selftest     : unknown
--
driver       : ecb(des)-ixp4xx
module       : ixp4xx_crypto
priority     : 300
refcnt       : 1
flags        : 0x85
ptr          : 0xbf01fcb4
selftest     : unknown

I traced the problem to crypto/algapi.c:

int crypto_register_alg(struct crypto_alg *alg)
{
	struct crypto_larval *larval;
	int err;

	err = crypto_check_alg(alg);
	if (err)
		return err;

	down_write(&crypto_alg_sem);
	larval = __crypto_register_alg(alg);
	up_write(&crypto_alg_sem);

	if (IS_ERR(larval))
		return PTR_ERR(larval);

	crypto_wait_for_test(larval);

At this point alg->cra_flags includes CRYPTO_ALG_DEAD (due to failed
test), but larval->alg.cra_flags has only the original flags (0x85).

I'm not sure what's the best fix.
Currently 2.6.31.9, seems to be present in 2.6.32, too.
-- 
Krzysztof Halasa

Re: Crypto test results unused?

[Krzysztof Halasa]

> int crypto_register_alg(struct crypto_alg *alg)
> {
> 	struct crypto_larval *larval;
> 	int err;
>
> 	err = crypto_check_alg(alg);
> 	if (err)
> 		return err;
>
> 	down_write(&crypto_alg_sem);
> 	larval = __crypto_register_alg(alg);
> 	up_write(&crypto_alg_sem);
>
> 	if (IS_ERR(larval))
> 		return PTR_ERR(larval);
>
> 	crypto_wait_for_test(larval);
>
> At this point alg->cra_flags includes CRYPTO_ALG_DEAD (due to failed
> test), but larval->alg.cra_flags has only the original flags (0x85).

Actually it seems all alg->cra_flags are CRYPTO_ALG_DEAD at this point,
not only these which failed tests. Will look at it soon.
-- 
Krzysztof Halasa

Re: Crypto test results unused?

[Krzysztof Halasa]

Krzysztof Halasa <khc@pm.waw.pl> writes:

> is the core crypto code supposed to "kill" algorithms which fail the
> test?
>
> On little-endian IXP4xx 3 hardware-assisted algorithms fail (due to
> apparently unrelated bug which I will take care of). It seems the kernel
> is still using these failing algorithms (my debugging code adds extra
> fields to the /proc output):
>
> alg: skcipher: Test 1 failed on encryption for ecb(des)-ixp4xx
> 00000000: 01 23 45 67 89 ab cd e7
> alg: skcipher: Test 1 failed on encryption for ecb(des3_ede)-ixp4xx
> 00000000: 73 6f 6d 65 64 61 74 61
> alg: skcipher: Test 1 failed on encryption for ecb(aes)-ixp4xx
> 00000000: 00 11 22 33 44 55 66 77 88 99 aa bb cc dd ee ff
>
> # grep 'ecb(des)-ixp4xx\|ecb(des3_ede)-ixp4xx\|ecb(aes)-ixp4xx' /proc/cryp
> to -A 6
> driver       : ecb(aes)-ixp4xx
> module       : ixp4xx_crypto
> priority     : 300
> refcnt       : 1
> flags        : 0x85
> ptr          : 0xbf020074
> selftest     : unknown
> ^^^^^^^^^^^^^^^^^^^^^^

I think probably crypto_alg_tested() should mark the failing algorithm
somehow?
void crypto_alg_tested(const char *name, int err)
{
	...
found:
	q->cra_flags |= CRYPTO_ALG_DEAD;
	alg = test->adult;

  >>>>>>>>>>>>> maybe here? if (err) mark_as_failed(alg); <<<<<<<<<<<<<<<<

	if (err || list_empty(&alg->cra_list))
		goto complete;

	alg->cra_flags |= CRYPTO_ALG_TESTED;
-- 
Krzysztof Halasa

Re: Crypto test results unused?

[Herbert Xu]

Krzysztof Halasa <khc@pm.waw.pl> wrote:
> 
> On little-endian IXP4xx 3 hardware-assisted algorithms fail (due to
> apparently unrelated bug which I will take care of). It seems the kernel
> is still using these failing algorithms (my debugging code adds extra
> fields to the /proc output):

How did you determine that it was still being used? When a kernel
user requests for an algorithm the system is supposed to skip
anything which failed the self-test.
 
> At this point alg->cra_flags includes CRYPTO_ALG_DEAD (due to failed
> test), but larval->alg.cra_flags has only the original flags (0x85).

CRYPTO_ALG_DEAD is used to mark algorithms deleted from the
system.  However, we don't delete algorithms just because they
fail the self-test.  They remain in the system so you can come
back and diagnose the problem.  They just aren't used by anyone.

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: Crypto test results unused?

[Krzysztof Halasa]

Herbert Xu <herbert@gondor.apana.org.au> writes:

>> On little-endian IXP4xx 3 hardware-assisted algorithms fail (due to
>> apparently unrelated bug which I will take care of). It seems the kernel
>> is still using these failing algorithms (my debugging code adds extra
>> fields to the /proc output):
>
> How did you determine that it was still being used? When a kernel
> user requests for an algorithm the system is supposed to skip
> anything which failed the self-test.

cat /proc/crypto shows "selftest: unknown" for those failed tests. I
don't know if that means it's used, but I'd expect "failed" or something
like that. Maybe it's simply a problem in /proc/crypto output.

> CRYPTO_ALG_DEAD is used to mark algorithms deleted from the
> system.  However, we don't delete algorithms just because they
> fail the self-test.  They remain in the system so you can come
> back and diagnose the problem.  They just aren't used by anyone.

Great.

Currently the /proc/crypto contains:

- for passed tests: "selftest: passed" (which is of course right)

- for failed tests: "selftest: unknown" (which is a surprise for me):

alg: skcipher: Test 1 failed on encryption for ecb(des)-ixp4xx
00000000: 01 23 45 67 89 ab cd e7

name         : ecb(des)
driver       : ecb(des)-ixp4xx
module       : ixp4xx_crypto
priority     : 300
refcnt       : 1
selftest     : unknown
type         : ablkcipher
async        : yes
blocksize    : 8
min keysize  : 8
max keysize  : 8
ivsize       : 0
geniv        : <default>

- for routines without a test: "selftest: passed" (which isn't true
  either)

alg: No test for authenc(hmac(md5),cbc(des)) (authenc(hmac(md5),cbc(des))-ixp4xx)

name         : authenc(hmac(md5),cbc(des))
driver       : authenc(hmac(md5),cbc(des))-ixp4xx
module       : ixp4xx_crypto
priority     : 300
refcnt       : 1
selftest     : passed
type         : aead
async        : yes
blocksize    : 8
ivsize       : 8
maxauthsize  : 16
geniv        : <built-in>

I think we need a way to differentiate between "really unknown" and
"failed", do we need another flag for it?
-- 
Krzysztof Halasa

Re: Crypto test results unused?

[Herbert Xu]

On Tue, Jan 12, 2010 at 06:55:07PM +0100, Krzysztof Halasa wrote:
>
> Currently the /proc/crypto contains:
> 
> - for passed tests: "selftest: passed" (which is of course right)
> 
> - for failed tests: "selftest: unknown" (which is a surprise for me):
> 
> alg: skcipher: Test 1 failed on encryption for ecb(des)-ixp4xx
> 00000000: 01 23 45 67 89 ab cd e7
> 
> name         : ecb(des)
> driver       : ecb(des)-ixp4xx
> module       : ixp4xx_crypto
> priority     : 300
> refcnt       : 1
> selftest     : unknown
> type         : ablkcipher
> async        : yes
> blocksize    : 8
> min keysize  : 8
> max keysize  : 8
> ivsize       : 0
> geniv        : <default>

This is fine.

> - for routines without a test: "selftest: passed" (which isn't true
>   either)
> 
> alg: No test for authenc(hmac(md5),cbc(des)) (authenc(hmac(md5),cbc(des))-ixp4xx)

This should be fixed by adding some code to test authenc algorithms
by combining the test vectors of its components.
 
> I think we need a way to differentiate between "really unknown" and
> "failed", do we need another flag for it?

It doesn't matter.  We test all algorithms at registration time.
So unknown means failed except during registration.

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: Crypto test results unused?

[Krzysztof Halasa]

Herbert Xu <herbert@gondor.apana.org.au> writes:

>> I think we need a way to differentiate between "really unknown" and
>> "failed", do we need another flag for it?
>
> It doesn't matter.  We test all algorithms at registration time.
> So unknown means failed except during registration.

So should we change "selftest: unknown" to "failed" then?
-- 
Krzysztof Halasa