[PATCH v4 1/7] lib: crypto: add public_key_verify_signature()

AKASHI Takahiro
2020-07-20T02:51:29Z

Heinrich,

On Sun, Jul 19, 2020 at 10:20:58AM +0200, Heinrich Schuchardt wrote:
> On 7/17/20 9:16 AM, AKASHI Takahiro wrote:
> > This function will be called from x509_check_for_self_signed() and
> > pkcs7_verify_one(), which will be imported from linux in a later patch.
> >
> > While it does exist in linux code and has a similar functionality of
> > rsa_verify(), it calls further linux-specific interfaces inside.
> > That could lead to more files being imported from linux.
> >
> > So simply re-implement it here instead of re-using the code.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> > include/crypto/public_key.h | 2 +-
> > lib/crypto/public_key.c | 70 ++++++++++++++++++++++++++++++++++++-
> > 2 files changed, 70 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
> > index 436a1ee1ee64..3ba90fcc3483 100644
> > --- a/include/crypto/public_key.h
> > +++ b/include/crypto/public_key.h
> > @@ -82,9 +82,9 @@ extern int decrypt_blob(struct kernel_pkey_params *, const void *, void *);
> > extern int create_signature(struct kernel_pkey_params *, const void *, void *);
> > extern int verify_signature(const struct key *,
> > const struct public_key_signature *);
> > +#endif /* __UBOOT__ */
> >
> > int public_key_verify_signature(const struct public_key *pkey,
> > const struct public_key_signature *sig);
> > -#endif /* !__UBOOT__ */
> >
> > #endif /* _LINUX_PUBLIC_KEY_H */
> > diff --git a/lib/crypto/public_key.c b/lib/crypto/public_key.c
> > index e12ebbb3d0c5..71a0e0356beb 100644
> > --- a/lib/crypto/public_key.c
> > +++ b/lib/crypto/public_key.c
> > @@ -25,7 +25,10 @@
> > #include <keys/asymmetric-subtype.h>
> > #endif
> > #include <crypto/public_key.h>
> > -#ifndef __UBOOT__
> > +#ifdef __UBOOT__
> > +#include <image.h>
> > +#include <u-boot/rsa.h>
> > +#else
> > #include <crypto/akcipher.h>
> > #endif
>
> Shouldn't we describe in the header of the file that the code is taken
> (at least partially) from Linux' crypto/asymmetric_keys/public_key.c?
>
> Please, also state the Linux version number.

Please take a look at the commit c4e961ecec99 ("lib: crypto: add public
key utility") as this file itself was imported in v2020-01.

While I prefer to add such an information in a commit message,
we should have an explicit rule for inclusion from outer sources
for consistency over the tree if you want to see such an information
in a file.

> >
> > @@ -80,6 +83,71 @@ void public_key_signature_free(struct public_key_signature *sig)
> > }
> > EXPORT_SYMBOL_GPL(public_key_signature_free);
> >
> > +/**
> > + * public_key_verify_signature - Verify a signature using a public key.
> > + *
> > + * @pkey: Public key
> > + * @sig: Signature
> > + *
> > + * Verify a signature, @sig, using a RSA public key, @pkey.
> > + *
> > + * Return: 0 - verified, non-zero error code - otherwise
> > + */
> > +int public_key_verify_signature(const struct public_key *pkey,
> > + const struct public_key_signature *sig)
> > +{
> > + struct image_sign_info info;
> > + struct image_region region;
> > + int ret;
> > +
> > + pr_devel("==>%s()\n", __func__);
> > +
> > + if (!pkey || !sig)
> > + return -EINVAL;
> > +
> > + if (pkey->key_is_private)
> > + return -EINVAL;
> > +
> > + memset(&info, '\0', sizeof(info));
> > + info.padding = image_get_padding_algo("pkcs-1.5");
> > + /*
> > + * Note: image_get_[checksum|crypto]_algo takes a string
> > + * argument like "<checksum>,<crypto>"
> > + * TODO: support other hash algorithms
> > + */
> > + if (strcmp(sig->pkey_algo, "rsa") || (sig->s_size * 8) != 2048) {
> > + pr_warn("Encryption is not RSA2048: %s%d\n",
> > + sig->pkey_algo, sig->s_size * 8);
> > + return -ENOPKG;
> > + }
> > + if (!strcmp(sig->hash_algo, "sha1")) {
> > + info.checksum = image_get_checksum_algo("sha1,rsa2048");
> > + info.name = "sha1,rsa2048";
> > + } else if (!strcmp(sig->hash_algo, "sha256")) {
> > + info.checksum = image_get_checksum_algo("sha256,rsa2048");
> > + info.name = "sha256,rsa2048";
> > + } else {
> > + pr_warn("unknown msg digest algo: %s\n", sig->hash_algo);
> > + return -ENOPKG;
> > + }
> > + info.crypto = image_get_crypto_algo(info.name);
> > + if (unlikely(IS_ERR(info.checksum) || IS_ERR(info.crypto)))
> > + return -ENOPKG;
>
> scripts/checkpatch.pl complains:
>
> WARNING: nested (un)?likely() calls, IS_ERR already uses unlikely()
> internally
> #104: FILE: lib/crypto/public_key.c:134:
> + if (unlikely(IS_ERR(info.checksum) || IS_ERR(info.crypto)))

Probably I missed it out when checking.

> I cannot find this unlikely in the Linux v5.8-rc4
> crypto/asymmetric_keys/public_key.c file. From where did you copy this code?

As I stated in the commit message, the whole code of this function
was written from the scratch rather than by re-using linux code.

-Takahiro Akashi

> Best regards
>
> Heinrich
>
> > +
> > + info.key = pkey->key;
> > + info.keylen = pkey->keylen;
> > +
> > + region.data = sig->digest;
> > + region.size = sig->digest_size;
> > +
> > + if (rsa_verify_with_pkey(&info, sig->digest, sig->s, sig->s_size))
> > + ret = -EKEYREJECTED;
> > + else
> > + ret = 0;
> > +
> > + pr_devel("<==%s() = %d\n", __func__, ret);
> > + return ret;
> > +}
> > #else
> > /*
> > * Destroy a public key algorithm key.
> >
>