Akashi-san,
On Tue, Jan 18, 2022 at 09:38:22PM +0900, AKASHI Takahiro wrote:
> Hi Ilias,
>
> On Tue, Jan 18, 2022 at 01:12:37PM +0200, Ilias Apalodimas wrote:
> > Right now the code explicitly limits us to sha1,256 hashes with RSA2048
> > encryption. But the limitation is artificial since U-Boot supports
> > a wider range of algorithms.
> >
> > The internal image_get_[checksum|crypto]_algo() functions expect an
> > argument in the format of <checksum>,<crypto>. So let's remove the size
> > checking and create the needed string on the fly in order to support
> > more hash/signing combinations.
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodi...@linaro.org>
> > ---
> > lib/crypto/public_key.c | 27 +++++++++++++--------------
> > 1 file changed, 13 insertions(+), 14 deletions(-)
> >
> > diff --git a/lib/crypto/public_key.c b/lib/crypto/public_key.c
> > index df6033cdb499..b783c63f5a51 100644
> > --- a/lib/crypto/public_key.c
> > +++ b/lib/crypto/public_key.c
> > @@ -97,6 +97,7 @@ int public_key_verify_signature(const struct public_key
> > *pkey,
> > const struct public_key_signature *sig)
> > {
> > struct image_sign_info info;
> > + char algo[256];
> > int ret;
> >
> > pr_devel("==>%s()\n", __func__);
> > @@ -108,29 +109,27 @@ int public_key_verify_signature(const struct
> > public_key *pkey,
> > return -EINVAL;
> >
> > memset(&info, '\0', sizeof(info));
> > + memset(algo, 0, sizeof(algo));
> > 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 this patch is applied, the TODO comment above will make no sense :)
We are still only handle SHA, but there's a printable error now, so i'll
get rid of the comment.
>
> > - 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);
> > + if (strcmp(sig->pkey_algo, "rsa")) {
> > + pr_err("Encryption is not RSA: %s\n", sig->pkey_algo);
> > return -ENOPKG;
> > }
> > + ret = snprintf(algo, sizeof(algo), "%s,%s%d", sig->hash_algo,
> > + sig->pkey_algo, sig->s_size * 8);
>
> I'm not sure that this naming rule, in particular the latter part, will
> always hold in the future while all the existing algo's observe it.
> (Maybe we need some note somewhere?)
The if a few lines below will shield us and return -EINVAL. How about
adding an error message there?
Cheers
/Ilias
>
> -Takahiro Akashi
>
> > +
> > + if (ret >= sizeof(algo))
> > + return -EINVAL;
> > +
> > + info.checksum = image_get_checksum_algo((const char *)algo);
> > + info.name = (const char *)algo;
> > info.crypto = image_get_crypto_algo(info.name);
> > - if (IS_ERR(info.checksum) || IS_ERR(info.crypto))
> > + if (!info.checksum || !info.crypto)
> > return -ENOPKG;
> >
> > info.key = pkey->key;
> > --
> > 2.30.2
> >
