Re: [PATCH V4 02/15] asymmetric keys: implement EMSA_PKCS1-v1_5-ENCODE in rsa

[joeyli <jlee@suse.com>]

Hi Phil, 

First! Thanks for your time to review my patch!

於 一，2013-09-23 於 19:49 +0300，Phil Carmody 提到：
> On Sun, Sep 15, 2013 at 08:56:48AM +0800, Lee, Chun-Yi wrote:
> > Implement EMSA_PKCS1-v1_5-ENCODE [RFC3447 sec 9.2] in rsa.c. It's the
> > first step of signature generation operation (RSASSA-PKCS1-v1_5-SIGN).
> > 
> > This patch is temporary set emLen to pks->k, and temporary set EM to
> > pks->S for debugging. We will replace the above values to real signature
> > after implement RSASP1.
> > 
> > The naming of EMSA_PKCS1_v1_5_ENCODE and the variables used in this function
> > accord PKCS#1 spec but not follow kernel naming convention, it useful when look
> > at them with spec.
> > 
> > Reference: ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-1v2/pkcs1ietffinal.txt
> > Reference: http://www.emc.com/collateral/white-papers/h11300-pkcs-1v2-2-rsa-cryptography-standard-wp.pdf
> > 
> > V2:
> 
> You're now at V4.

The V4 is for whole patchset, I didn't do any modify in this patch in
this version.
The version define maybe confuse between separate and whole patchset, I
will avoid it.

> 
> > - Clean up naming of variable: replace _EM by EM, replace EM by EM_tmp.
> > - Add comment to EMSA_PKCS1-v1_5-ENCODE function.
> > 
> > Cc: Pavel Machek <pavel@ucw.cz>
> > Reviewed-by: Jiri Kosina <jkosina@suse.cz>
> > Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
> > ---
> >  crypto/asymmetric_keys/rsa.c |  163 +++++++++++++++++++++++++++++++++++++++++-
> >  include/crypto/public_key.h  |    2 +
> >  2 files changed, 164 insertions(+), 1 deletions(-)
> > 
> > diff --git a/crypto/asymmetric_keys/rsa.c b/crypto/asymmetric_keys/rsa.c
> > index 47f3be4..352ba45 100644
> > --- a/crypto/asymmetric_keys/rsa.c
> > +++ b/crypto/asymmetric_keys/rsa.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/module.h>
> >  #include <linux/kernel.h>
> >  #include <linux/slab.h>
> > +#include <crypto/hash.h>
> >  #include "public_key.h"
> >  #include "private_key.h"
> >  
> > @@ -152,6 +153,132 @@ static int RSA_I2OSP(MPI x, size_t xLen, u8 **_X)
> >  }
> >  
> >  /*
> > + * EMSA_PKCS1-v1_5-ENCODE [RFC3447 sec 9.2]
> > + * @M: message to be signed, an octet string
> > + * @emLen: intended length in octets of the encoded message
> > + * @hash_algo: hash function (option)
> > + * @hash: true means hash M, otherwise M is already a digest
> > + * @EM: encoded message, an octet string of length emLen
> > + *
> > + * This function is a implementation of the EMSA-PKCS1-v1_5 encoding operation
> > + * in RSA PKCS#1 spec. It used by the signautre generation operation of
> > + * RSASSA-PKCS1-v1_5 to encode message M to encoded message EM.
> > + *
> > + * The variables used in this function accord PKCS#1 spec but not follow kernel
> > + * naming convention, it useful when look at them with spec.
> > + */
> > +static int EMSA_PKCS1_v1_5_ENCODE(const u8 *M, size_t emLen,
> > +		enum pkey_hash_algo hash_algo, const bool hash,
> > +		u8 **EM, struct public_key_signature *pks)
> > +{
> > +	u8 *digest;
> > +	struct crypto_shash *tfm;
> > +	struct shash_desc *desc;
> > +	size_t digest_size, desc_size;
> > +	size_t tLen;
> > +	u8 *T, *PS, *EM_tmp;
> > +	int i, ret;
> > +
> > +	pr_info("EMSA_PKCS1_v1_5_ENCODE start\n");
> > +
> > +	if (!RSA_ASN1_templates[hash_algo].data)
> > +		ret = -ENOTSUPP;
> 
> ...
> 
> > +	else
> > +		pks->pkey_hash_algo = hash_algo;
> > +
> > +	/* 1) Apply the hash function to the message M to produce a hash value H */
> > +	tfm = crypto_alloc_shash(pkey_hash_algo[hash_algo], 0, 0);
> > +	if (IS_ERR(tfm))
> > +		return (PTR_ERR(tfm) == -ENOENT) ? -ENOPKG : PTR_ERR(tfm);
> > +
> > +	desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
> > +	digest_size = crypto_shash_digestsize(tfm);
> > +
> > +	ret = -ENOMEM;
> 
> The earlier "ret = -ENOTSUPP;" is either unused because you return at the IS_ERR,
> or unused because you overwrite it here. I'm a little disappointed that
> the compiler didn't recognise that something was assigned to a value that 
> is never used.
> 
> Phil

Yes, Dmitry also pointed out this issue, I should not go on the hash
process if the hash algorithm didn't support.

I will change fix this problem in next version. 


Thanks a lot!
Joey Lee