diff mbox

[v6,0/7] rsa: extend rsa_verify() for UEFI secure boot

Message ID 20200217014240.GC22953@linaro.org
State New
Headers show

Commit Message

AKASHI Takahiro Feb. 17, 2020, 1:42 a.m. UTC 
Hi Tom,

On Fri, Feb 14, 2020 at 07:29:37AM -0500, Tom Rini wrote:
> On Mon, Jan 27, 2020 at 07:27:33PM +0900, AKASHI Takahiro wrote:
> 
> > # This patch set is a prerequisite for UEFI secure boot.
> > 
> > The current rsa_verify() requires five parameters for a RSA public key
> > for efficiency while RSA, in theory, requires only two. In addition,
> > those parameters are expected to come from FIT image.
> > 
> > So this function won't fit very well when we want to use it for the purpose
> > of implementing UEFI secure boot, in particular, image authentication
> > as well as variable authentication, where the essential two parameters
> > are set to be retrieved from one of X509 certificates in signature
> > database.
> > 
> > So, in this patch, additional three parameters will be calculated
> > on the fly when rsa_verify() is called without fdt which should contain
> > parameters above.
> > 
> > This calculation heavily relies on "big-number (or multi-precision)
> > library." Therefore some routines from BearSSL[1] under MIT license are
> > imported in this implementation. See Patch#4.
> > # Please let me know if this is not appropriate.
> > 
> > Prerequisite:
> > * public key parser in my "import x509/pkcs7 parser" patch[2]
> > 
> > # Checkpatch will complain with lots of warnings/errors, but
> > # I intentionally don't fix them for maximum maintainability.
> > 
> >   [1] https://bearssl.org/
> >   [2] https://lists.denx.de/pipermail/u-boot/2019-November/390127.html
> 
> At this point it needs to be rebased again.  There's a ton of failures
> in https://gitlab.denx.de/u-boot/u-boot/pipelines/2198 which is after I

I think that you have wrongly merged my rsa extension patch here.
Looking at your modified commit,
https://gitlab.denx.de/u-boot/u-boot/commit/13fb61ce20dcd65cd4ccba1554eca6343c92ed6b
there is one missing hunk from my original.
Please revert the change in include/image.h and then apply a diff attached below.

> did
> https://gitlab.denx.de/u-boot/u-boot/commit/7db0379f85995d8c7673db7b04eb36d96546c9c8
> and I'll put a proper commit message on that later today and post it and
> CC relevant parties.

I believe that your commit above has nothing to do with my patch
(and test failures).

> It's otherwise looking good.  I do want to confirm
> that on boards like minnowmax the slight growth in fit_image_check_sig
> is expected.  It's only 6 bytes so it probably is and we get a larger
> reduction in rsa_verify all-around.

Growth due to my patch??

Thanks,
-Takahiro Akashi

> Thanks!
> 
> -- 
> Tom

Comments

Tom Rini Feb. 18, 2020, 7:57 p.m. UTC | #1 
On Mon, Feb 17, 2020 at 10:42:41AM +0900, AKASHI Takahiro wrote:
> Hi Tom,
> 
> On Fri, Feb 14, 2020 at 07:29:37AM -0500, Tom Rini wrote:
> > On Mon, Jan 27, 2020 at 07:27:33PM +0900, AKASHI Takahiro wrote:
> > 
> > > # This patch set is a prerequisite for UEFI secure boot.
> > > 
> > > The current rsa_verify() requires five parameters for a RSA public key
> > > for efficiency while RSA, in theory, requires only two. In addition,
> > > those parameters are expected to come from FIT image.
> > > 
> > > So this function won't fit very well when we want to use it for the purpose
> > > of implementing UEFI secure boot, in particular, image authentication
> > > as well as variable authentication, where the essential two parameters
> > > are set to be retrieved from one of X509 certificates in signature
> > > database.
> > > 
> > > So, in this patch, additional three parameters will be calculated
> > > on the fly when rsa_verify() is called without fdt which should contain
> > > parameters above.
> > > 
> > > This calculation heavily relies on "big-number (or multi-precision)
> > > library." Therefore some routines from BearSSL[1] under MIT license are
> > > imported in this implementation. See Patch#4.
> > > # Please let me know if this is not appropriate.
> > > 
> > > Prerequisite:
> > > * public key parser in my "import x509/pkcs7 parser" patch[2]
> > > 
> > > # Checkpatch will complain with lots of warnings/errors, but
> > > # I intentionally don't fix them for maximum maintainability.
> > > 
> > >   [1] https://bearssl.org/
> > >   [2] https://lists.denx.de/pipermail/u-boot/2019-November/390127.html
> > 
> > At this point it needs to be rebased again.  There's a ton of failures
> > in https://gitlab.denx.de/u-boot/u-boot/pipelines/2198 which is after I
> 
> I think that you have wrongly merged my rsa extension patch here.
> Looking at your modified commit,
> https://gitlab.denx.de/u-boot/u-boot/commit/13fb61ce20dcd65cd4ccba1554eca6343c92ed6b
> there is one missing hunk from my original.
> Please revert the change in include/image.h and then apply a diff attached below.

Please rebase and repost the series.

> > did
> > https://gitlab.denx.de/u-boot/u-boot/commit/7db0379f85995d8c7673db7b04eb36d96546c9c8
> > and I'll put a proper commit message on that later today and post it and
> > CC relevant parties.
> 
> I believe that your commit above has nothing to do with my patch
> (and test failures).

I'll re-confirm things then with the next post.

> > It's otherwise looking good.  I do want to confirm
> > that on boards like minnowmax the slight growth in fit_image_check_sig
> > is expected.  It's only 6 bytes so it probably is and we get a larger
> > reduction in rsa_verify all-around.
> 
> Growth due to my patch??

Unless it's something else I mis-merged, yes.  But given the area this
series works on, it's not unexpected growth.

Thanks!
diff mbox

Patch

===8<===
diff --git a/include/image.h b/include/image.h
index b316d167d8d7..eb7aa5622aa3 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1114,6 +1114,7 @@  int fit_conf_get_prop_node(const void *fit, int noffset,
 
 int fit_check_ramdisk(const void *fit, int os_noffset,
 		uint8_t arch, int verify);
+#endif /* IMAGE_ENABLE_FIT */
 
 int calculate_hash(const void *data, int data_len, const char *algo,
 			uint8_t *value, int *value_len);
@@ -1126,16 +1127,20 @@  int calculate_hash(const void *data, int data_len, const char *algo,
 # if defined(CONFIG_FIT_SIGNATURE)
 #  define IMAGE_ENABLE_SIGN	1
 #  define IMAGE_ENABLE_VERIFY	1
+#  define FIT_IMAGE_ENABLE_VERIFY	1
 #  include <openssl/evp.h>
 # else
 #  define IMAGE_ENABLE_SIGN	0
 #  define IMAGE_ENABLE_VERIFY	0
+#  define FIT_IMAGE_ENABLE_VERIFY	0
 # endif
 #else
 # define IMAGE_ENABLE_SIGN	0
-# define IMAGE_ENABLE_VERIFY	CONFIG_IS_ENABLED(FIT_SIGNATURE)
+# define IMAGE_ENABLE_VERIFY		CONFIG_IS_ENABLED(RSA_VERIFY)
+# define FIT_IMAGE_ENABLE_VERIFY	CONFIG_IS_ENABLED(FIT_SIGNATURE)
 #endif
 
+#if IMAGE_ENABLE_FIT
 #ifdef USE_HOSTCC
 void *image_get_host_blob(void);
 void image_set_host_blob(void *host_blob);
@@ -1149,6 +1154,7 @@  void image_set_host_blob(void *host_blob);
 #else
 #define IMAGE_ENABLE_BEST_MATCH	0
 #endif
+#endif /* IMAGE_ENABLE_FIT */
 
 /* Information passed to the signing routines */
 struct image_sign_info {
@@ -1166,16 +1172,12 @@  struct image_sign_info {
 	const char *engine_id;		/* Engine to use for signing */
 };
 
-#endif /* Allow struct image_region to always be defined for rsa.h */
-
 /* A part of an image, used for hashing */
 struct image_region {
 	const void *data;
 	int size;
 };
 
-#if IMAGE_ENABLE_FIT
-
 #if IMAGE_ENABLE_VERIFY
 # include <u-boot/rsa-checksum.h>
 #endif
@@ -1276,6 +1278,8 @@  struct crypto_algo *image_get_crypto_algo(const char *full_name);
  */
 struct padding_algo *image_get_padding_algo(const char *name);
 
+#if IMAGE_ENABLE_FIT
+
 /**
  * fit_image_verify_required_sigs() - Verify signatures marked as 'required'
  *