[PATCH v4 6/6] test: add rsa_verify() unit test

[Tom Rini <trini@konsulko.com>]

On Wed, Jan 22, 2020 at 09:22:42AM +0900, AKASHI Takahiro wrote:
> On Tue, Jan 21, 2020 at 10:40:23AM -0500, Tom Rini wrote:
> > On Tue, Jan 21, 2020 at 02:48:52PM +0900, AKASHI Takahiro wrote:
> > > On Fri, Jan 17, 2020 at 06:26:34AM +0100, Heinrich Schuchardt wrote:
> > > > On 1/17/20 2:53 AM, AKASHI Takahiro wrote:
> > > > >On Tue, Jan 14, 2020 at 01:04:58PM +0100, Heinrich Schuchardt wrote:
> > > > >>On 1/14/20 8:33 AM, AKASHI Takahiro wrote:
> > > > >>>On Wed, Jan 08, 2020 at 06:43:51PM +0100, Heinrich Schuchardt wrote:
> > > > >>>>
> > > > >>>>
> > > > >>>>On 11/21/19 1:11 AM, AKASHI Takahiro wrote:
> > > > >>>>>In this patch, a very simple test is added to verify that rsa_verify()
> > > > >>>>>using rsa_verify_with_pkey() work correctly.
> > > > >>>>>
> > > > >>>>>To keep the code simple, all the test data, either public key and
> > > > >>>>>verified binary data, are embedded in the source.
> > > > >>>>>
> > > > >>>>>Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > >>>>>---
> > > > >>>>>  test/Kconfig      |  12 +++
> > > > >>>>>  test/lib/Makefile |   1 +
> > > > >>>>>  test/lib/rsa.c    | 206 ++++++++++++++++++++++++++++++++++++++++++++++
> > > > >>>>>  3 files changed, 219 insertions(+)
> > > > >>>>>  create mode 100644 test/lib/rsa.c
> > > > >>>>>
> > > > >>>>>diff --git a/test/Kconfig b/test/Kconfig
> > > > >>>>>index cb7954041eda..64d76c3b20a5 100644
> > > > >>>>>--- a/test/Kconfig
> > > > >>>>>+++ b/test/Kconfig
> > > > >>>>>@@ -28,6 +28,18 @@ config UT_LIB_ASN1
> > > > >>>>>  	  Enables a test which exercises asn1 compiler and decoder function
> > > > >>>>>  	  via various parsers.
> > > > >>>>>
> > > > >>>>>+config UT_LIB_RSA
> > > > >>>>>+	bool "Unit test for rsa_verify() function"
> > > > >>>>>+	imply RSA
> > > > >>>>>+	imply ASYMMETRIC_KEY_TYPE
> > > > >>>>>+	imply ASYMMETRIC_PUBLIC_KEY_SUBTYPE
> > > > >>>>>+	imply RSA_PUBLIC_KEY_PARSER
> > > > >>>>>+	imply RSA_VERIFY_WITH_PKEY
> > > > >>>>
> > > > >>>>This test should depend on RSA_VERIFY_WITH_PKEY because is cannot be
> > > > >>>>executed otherwise.
> > > > >>>
> > > > >>>See below.
> > > > >>>
> > > > >>>>Best regards
> > > > >>>>
> > > > >>>>Heinrich
> > > > >>>>
> > > > >>>>>+	default y
> > > > >>>>>+	help
> > > > >>>>>+	  Enables rsa_verify() test, currently rsa_verify_with_pkey only()
> > > > >>>>>+	  only, at the 'ut lib' command.
> > > > >>>>>+
> > > > >>>>>  endif
> > > > >>>>>
> > > > >>>>>  config UT_TIME
> > > > >>>>>diff --git a/test/lib/Makefile b/test/lib/Makefile
> > > > >>>>>index 72d2ec74b5f4..2bf6ef3935bb 100644
> > > > >>>>>--- a/test/lib/Makefile
> > > > >>>>>+++ b/test/lib/Makefile
> > > > >>>>>@@ -8,3 +8,4 @@ obj-y += lmb.o
> > > > >>>>>  obj-y += string.o
> > > > >>>>>  obj-$(CONFIG_ERRNO_STR) += test_errno_str.o
> > > > >>>>>  obj-$(CONFIG_UT_LIB_ASN1) += asn1.o
> > > > >>>>>+obj-$(CONFIG_UT_LIB_RSA) += rsa.o
> > > > >>>>>diff --git a/test/lib/rsa.c b/test/lib/rsa.c
> > > > >>>>>new file mode 100644
> > > > >>>>>index 000000000000..44f8ade226f4
> > > > >>>>>--- /dev/null
> > > > >>>>>+++ b/test/lib/rsa.c
> > > > >>>>>@@ -0,0 +1,206 @@
> > > > >>>>>+// SPDX-License-Identifier: GPL-2.0+
> > > > >>>>>+/*
> > > > >>>>>+ * Copyright (c) 2019 Linaro Limited
> > > > >>>>>+ * Author: AKASHI Takahiro
> > > > >>>>>+ *
> > > > >>>>>+ * Unit test for rsa_verify() function
> > > > >>>>>+ */
> > > > >>>>>+
> > > > >>>>>+#include <common.h>
> > > > >>>>>+#include <command.h>
> > > > >>>>>+#include <image.h>
> > > > >>>>>+#include <test/lib.h>
> > > > >>>>>+#include <test/test.h>
> > > > >>>>>+#include <test/ut.h>
> > > > >>>>>+#include <u-boot/rsa.h>
> > > > >>>>>+
> > > > >>>>>+#ifdef CONFIG_RSA_VERIFY_WITH_PKEY
> > > > >>>>
> > > > >>>>Please, remove this ifdef. Simply do not build this module if
> > > > >>>>CONFIG_RSA_VERIFY_WITH_PKEY is not defined.
> > > > >>>
> > > > >>>That is why I added this #ifdef here to avoid a build error.
> > > > >>>I think it is a good idea to make test code as generic as possible
> > > > >>>to easily add more test cases.
> > > > >>
> > > > >>It is preferable to move configuration dependencies to the Makefile.
> > > > >>
> > > > >>You can create a new C file if you have tests needing a different
> > > > >>configuration.
> > > > >>
> > > > >>test/lib/rsa_verify.c might be a better name for the file.
> > > > >>
> > > > >>In test/Kconfig, please, replace
> > > > >>
> > > > >>config UT_LIB_RSA
> > > > >>         bool "Unit test for rsa_verify() function"
> > > > >>         imply RSA
> > > > >>
> > > > >>by
> > > > >>
> > > > >>config UT_LIB_RSA
> > > > >>         bool "Unit test for rsa_verify() function"
> > > > >>	default y
> > > > >>         depends on RSA
> > > > >
> > > > >Initially in my development code, I did the exact same thing,
> > > > >but I didn't adopt this approach.
> > > > >
> > > > >If we follow your proposal, UT_LIB_RSA won't be selected
> > > > >unless RSA is enabled.
> > > > >So UT_LIB_RSA won't be executed under most existing configurations
> > > > >in the current CI.  This was not what I wanted.
> > > > >
> > > > >What I wanted here is that, if UT (and UT_LIB) is selected, all
> > > > >the configurations needed for enabling all the test cases be
> > > > >also selected *automatically*.
> > > > 
> > > > What I want is that if I run 'make menuconfig' and select CONFIG_UT
> > > > manually I end up with a configuration that compiles and that does what
> > > > I have configured.
> > > 
> > > Hmm, that is what I intend to do here, but the code doesn't work
> > > as I expected. It works only if CONFIG_UT_LIB_RSA is on in *defconfig.
> > > 
> > > So I have no option other than changing reverse dependencies as follows,
> > >     CONFIG_UT_LIB_RSA
> > >       select IMAGE_SIGN_INFO
> > >       select RSA
> > >       imply RSA_VERIFY_WITH_PKEY
> > > 
> > > Using 'select' is still safe as CONFIG_RSA has no dependency.
> > > (Please note we can still use 'imply' for VERIFY_WITH_PKEY here.)
> > > 
> > > Tom, do you agree to this change?
> > 
> > OK, my fault for not spotting it before.  Using imply (as we do today on
> > UT_LIB_ASN1) is wrong.  All of the tests should "depends on" what they
> > need in order to function.  Thanks!
> 
> I know that using "depends on" is an obvious solution here, but
> in that case, we won't run those tests automatically in CI unless
> the dependency targets, like CONFIG_RSA, are explicitly or
> implicitly enabled by *defconfig.
> 
> Is that OK for you?

Yes, I expect them to be enabled in sandbox and relevant qemu targets to
get broad testing here as that is part of the reason for those targets.
In fact, sandbox should always be a priority to build-support, that is
also where we get Coverity scans run.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200121/f88f9d19/attachment.sig>