crypto: add function to free rsa keypair

[elias-vd]

There was no function to proper free a rsa kepair from inside a PTA
or the core itself. Now there is crypto_acipher_free_rsa_keypair().

Signed-off-by: E. von Däniken elias.vondaeniken@bluewin.ch

[jforissier]

Looks reasonable to me but I would prefer to merge this only when we have at least one user of the new function.
Also, I think the CAAM driver (core/drivers/crypto/caam/acipher/caam_rsa.c) needs to implement the new function otherwise crypto_acipher_free_rsa_keypair() would crash.
@cneveux @sdininno @clementfaure

[jenswi-linaro]

We could make the implementation of crypto_acipher_free_rsa_public_key() to core/crypto/crypto.c unconditionally as:

void crypto_acipher_free_rsa_public_key(struct rsa_public_key *s)
{
        if (!s)
                return;
        crypto_bignum_free(s->n);
        crypto_bignum_free(s->e);
}         

This should always be valid, same goes for the corresponding crypto_acipher_free_rsa_keypair(). Comments?

[cneveux]

We could make the implementation of crypto_acipher_free_rsa_public_key() to core/crypto/crypto.c unconditionally as:

void crypto_acipher_free_rsa_public_key(struct rsa_public_key *s)
{
        if (!s)
                return;
        crypto_bignum_free(s->n);
        crypto_bignum_free(s->e);
}         

This should always be valid, same goes for the corresponding crypto_acipher_free_rsa_keypair(). Comments?

Function do_keypair_free already exists in the caam_rsa.c and can be used has done for the free_publickey.

[cneveux]

We could make the implementation of crypto_acipher_free_rsa_public_key() to core/crypto/crypto.c unconditionally as:

void crypto_acipher_free_rsa_public_key(struct rsa_public_key *s)
{
        if (!s)
                return;
        crypto_bignum_free(s->n);
        crypto_bignum_free(s->e);
}         

This should always be valid, same goes for the corresponding crypto_acipher_free_rsa_keypair(). Comments?

@jenswi-linaro
I will work if driver is using bignumber buffers for the key like tomcrypt.
In the case of the CAAM, at the end we will remove unneeded bignumber conversion where not necessary because of the overhead added by conversion from bignumber to buffer used by the CAAM.

[jenswi-linaro]

We could make the implementation of crypto_acipher_free_rsa_public_key() to core/crypto/crypto.c unconditionally as:

void crypto_acipher_free_rsa_public_key(struct rsa_public_key *s)
{
        if (!s)
                return;
        crypto_bignum_free(s->n);
        crypto_bignum_free(s->e);
}         

This should always be valid, same goes for the corresponding crypto_acipher_free_rsa_keypair(). Comments?

@jenswi-linaro
I will work if driver is using bignumber buffers for the key like tomcrypt.
In the case of the CAAM, at the end we will remove unneeded bignumber conversion where not necessary because of the overhead added by conversion from bignumber to buffer used by the CAAM.

The allocation in tee_obj_set_type() file core/tee/tee_svc_cryp.c is freed using tee_obj_attr_free(). This depends on being able to free each element as above. Are you planning to change that?

[cneveux]

We could make the implementation of crypto_acipher_free_rsa_public_key() to core/crypto/crypto.c unconditionally as:

void crypto_acipher_free_rsa_public_key(struct rsa_public_key *s)
{
        if (!s)
                return;
        crypto_bignum_free(s->n);
        crypto_bignum_free(s->e);
}         

This should always be valid, same goes for the corresponding crypto_acipher_free_rsa_keypair(). Comments?

@jenswi-linaro
I will work if driver is using bignumber buffers for the key like tomcrypt.
In the case of the CAAM, at the end we will remove unneeded bignumber conversion where not necessary because of the overhead added by conversion from bignumber to buffer used by the CAAM.

The allocation in tee_obj_set_type() file core/tee/tee_svc_cryp.c is freed using tee_obj_attr_free(). This depends on being able to free each element as above. Are you planning to change that?

What I can see if I'm not wrong, there is a conversion from user binary buffer to internal TEE big number buffer. Then when exporting TEE internal big number buffer to User, there is a conversion big number to binary. Hence why adding this overhead that may not be useful?

In case of CAAM Driver, the TEE internal big number must be converted back to binary to be used by the HW. I will not generalize but I never see a HW module using a big number format for the operation.

I would be more logic to not convert User binary to bignumber when not needed and let the library or driver doing the conversion it needs.
So if we would like to remove the bignumber conversions not needed, the only way is to change the core/tee/tee_svc_crypt.c and add a conversion bin to bn and bn to bin where needed.

[jenswi-linaro]

@cneveux, now I fear we have hijacked this PR. I agree with you, there's no point in using an intermediate format that will just be converted again. I'm happy to help sorting that out, but perhaps in another PR/Issue/private conversation.

So back to this PR. For the time being let's have three implementations of crypto_acipher_free_rsa_keypair() in the same way that we have three implementations of crypto_acipher_free_rsa_public_key(). We'll try to refactor that later on when we know more.

@elias-vd, Jerome pointed out that:

optee_os/core/drivers/crypto/caam/acipher/caam_rsa.c

Lines 1650 to 1659 in 30c53a7

	static const struct drvcrypt_rsa driver_rsa = {
	.alloc_keypair = &do_allocate_keypair,
	.alloc_publickey = &do_allocate_publickey,
	.free_publickey = &do_free_publickey,
	.gen_keypair = &do_gen_keypair,
	.encrypt = &do_encrypt,
	.decrypt = &do_decrypt,
	.optional.ssa_sign = NULL,
	.optional.ssa_verify = NULL,
	};

needs to have a .free_keypair initialization too.

[cneveux]

I agree on the needs to have the .free_keypair same way done for the .free_publickey

[elias-vd]

Fixed

[jforissier]

Acked-by: Jerome Forissier <jerome@forissier.org>

Preferably as a preparatory commit in #4011.

[jenswi-linaro]

Please fix the compile error

[elias-vd]

The new commit contains a new function to free the rsa keypair. The problem was that the do_keypair_free functio, in the CAAM driver, does free a struct caam_rsa_keypair, but we want to free a struct rsa_public_key which contains bignum not caambuf. Please take a look at it, because I am do not feel comfortable with this solution.

[jenswi-linaro]

This test can be removed since this function is only called if key isn't NULL.

[jenswi-linaro]

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

@cneveux @sdininno @clementfaure are you OK with this too?

[cneveux]

Minor but if we want to be consistent with the free_publickey operation function do_free_publickey the function do_keypair_free_bn should be named do_free_keypair_free
And no need to rename the do_keypair_free to do_keypair_free_caam. There is no risk of executing error as the parameter is not the same.

Otherwise I'm ok

[cneveux]

This function name is duplicated. I suggested to rename it do_free_keypair

[cneveux]

And so you have to change .free_keypair = &do_keypair_free, to .free_keypair = &do_free_keypair,

[etienne-lms]

minor: looks not very nice these 2 functions almost have the same names: do_keypair_free() and do_fre_keypair.

[cneveux]

There are not doing the same and this is internal to the caam driver. This doesn't cause a problem for me

[elias-vd]

I think all comments have been addressed. If you agree I will sqash the commits?

[jforissier]

This seems about ready for merging, @cneveux is this OK with you?

[cneveux]

This seems about ready for merging, @cneveux is this OK with you?

@jforissier,

Yes, I'm ok with the latest update