[PATCH v2 0/4] mkimage usability fixes

[PATCH v2 0/4] mkimage usability fixes

[Joel Stanley]

v2: Move -B and -E out from FIT_SIGNATURE check, and other fixes from
Philippe's review.

Original cover letter:

I was learning about signed FIT today and was unable to convince mkimage
to produce a signature node in my control device tree. It turns out that
Debian's packaged mkimage doesn't enable FIT_SIGNATURE, so the options I
was passing were silently ignored.

There's an existing Debian bug for that[1] but in the mean time this
changes mkimage's behaviour to fail loudly when FIT_SIGNATURE is
disabled.

The first two patches fix some issues I found when testing the third
patch under sandbox_defconfig with FIT_SIGNATURE=n.

[1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=972513

Joel Stanley (4):
  tools/Makefile: FIT_CIPHER requires libssl
  image-fit: Fix FIT_CIPHER linking
  mkimage: Move padding commands outside of FIT_SIGNATURE
  mkimge: Reject signing-related flags without FIT_SIGNATURE

 common/image-fit-sig.c | 14 --------------
 common/image-fit.c     | 15 +++++++++++++++
 tools/Makefile         |  2 +-
 tools/mkimage.c        | 18 +++++++++++-------
 4 files changed, 27 insertions(+), 22 deletions(-)

-- 
2.29.2

[PATCH v2 1/4] tools/Makefile: FIT_CIPHER requires libssl

[Joel Stanley]

If CONFIG_FIT_CIPHER is enabled without CONFIG_FIT_SIGNATURE then
mkimage/dumpimage will fail to link:

 /usr/bin/ld: tools/common/image-cipher.o: in function `fit_image_decrypt_data':
 image-cipher.c:(.text+0x9a): undefined reference to `image_get_host_blob'
 /usr/bin/ld: tools/common/image-cipher.o:(.data.rel+0x10): undefined reference to `EVP_aes_128_cbc'
 /usr/bin/ld: tools/common/image-cipher.o:(.data.rel+0x40): undefined reference to `EVP_aes_192_cbc'
 /usr/bin/ld: tools/common/image-cipher.o:(.data.rel+0x70): undefined reference to `EVP_aes_256_cbc'
 /usr/bin/ld: tools/lib/aes/aes-encrypt.o: in function `image_aes_encrypt':
 aes-encrypt.c:(.text+0x22): undefined reference to `EVP_CIPHER_CTX_new'
 /usr/bin/ld: aes-encrypt.c:(.text+0x6f): undefined reference to `EVP_EncryptInit_ex'
 /usr/bin/ld: aes-encrypt.c:(.text+0x8d): undefined reference to `EVP_EncryptUpdate'
 /usr/bin/ld: aes-encrypt.c:(.text+0xac): undefined reference to `EVP_CIPHER_CTX_free'
 /usr/bin/ld: aes-encrypt.c:(.text+0xf2): undefined reference to `EVP_EncryptFinal_ex'
 collect2: error: ld returned 1 exit status

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 tools/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/Makefile b/tools/Makefile
index 253a6b97065b..99a931312cd8 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -154,7 +154,7 @@ HOSTCFLAGS_kwbimage.o += -DCONFIG_KWB_SECURE
 endif
 
 # MXSImage needs LibSSL
-ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_ARMADA_38X)$(CONFIG_ARMADA_39X)$(CONFIG_FIT_SIGNATURE),)
+ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_ARMADA_38X)$(CONFIG_ARMADA_39X)$(CONFIG_FIT_SIGNATURE)$(CONFIG_FIT_CIPHER),)
 HOSTCFLAGS_kwbimage.o += \
 	$(shell pkg-config --cflags libssl libcrypto 2> /dev/null || echo "")
 HOSTLDLIBS_mkimage += \
-- 
2.29.2

[PATCH v2 1/4] tools/Makefile: FIT_CIPHER requires libssl

[Philippe REYNES]

Hi Joel,


Le 08/12/2020 ? 05:12, Joel Stanley a ?crit?:
> If CONFIG_FIT_CIPHER is enabled without CONFIG_FIT_SIGNATURE then
> mkimage/dumpimage will fail to link:
>
>   /usr/bin/ld: tools/common/image-cipher.o: in function `fit_image_decrypt_data':
>   image-cipher.c:(.text+0x9a): undefined reference to `image_get_host_blob'
>   /usr/bin/ld: tools/common/image-cipher.o:(.data.rel+0x10): undefined reference to `EVP_aes_128_cbc'
>   /usr/bin/ld: tools/common/image-cipher.o:(.data.rel+0x40): undefined reference to `EVP_aes_192_cbc'
>   /usr/bin/ld: tools/common/image-cipher.o:(.data.rel+0x70): undefined reference to `EVP_aes_256_cbc'
>   /usr/bin/ld: tools/lib/aes/aes-encrypt.o: in function `image_aes_encrypt':
>   aes-encrypt.c:(.text+0x22): undefined reference to `EVP_CIPHER_CTX_new'
>   /usr/bin/ld: aes-encrypt.c:(.text+0x6f): undefined reference to `EVP_EncryptInit_ex'
>   /usr/bin/ld: aes-encrypt.c:(.text+0x8d): undefined reference to `EVP_EncryptUpdate'
>   /usr/bin/ld: aes-encrypt.c:(.text+0xac): undefined reference to `EVP_CIPHER_CTX_free'
>   /usr/bin/ld: aes-encrypt.c:(.text+0xf2): undefined reference to `EVP_EncryptFinal_ex'
>   collect2: error: ld returned 1 exit status
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
 ??? Reviewed-by: Philippe Reynes <philippe.reynes@softathome.com>

> ---
>   tools/Makefile | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/Makefile b/tools/Makefile
> index 253a6b97065b..99a931312cd8 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -154,7 +154,7 @@ HOSTCFLAGS_kwbimage.o += -DCONFIG_KWB_SECURE
>   endif
>   
>   # MXSImage needs LibSSL
> -ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_ARMADA_38X)$(CONFIG_ARMADA_39X)$(CONFIG_FIT_SIGNATURE),)
> +ifneq ($(CONFIG_MX23)$(CONFIG_MX28)$(CONFIG_ARMADA_38X)$(CONFIG_ARMADA_39X)$(CONFIG_FIT_SIGNATURE)$(CONFIG_FIT_CIPHER),)
>   HOSTCFLAGS_kwbimage.o += \
>   	$(shell pkg-config --cflags libssl libcrypto 2> /dev/null || echo "")
>   HOSTLDLIBS_mkimage += \

Regards,

Philippe

[PATCH v2 1/4] tools/Makefile: FIT_CIPHER requires libssl

[Tom Rini]

On Tue, Dec 08, 2020 at 02:42:13PM +1030, Joel Stanley wrote:

> If CONFIG_FIT_CIPHER is enabled without CONFIG_FIT_SIGNATURE then
> mkimage/dumpimage will fail to link:
> 
>  /usr/bin/ld: tools/common/image-cipher.o: in function `fit_image_decrypt_data':
>  image-cipher.c:(.text+0x9a): undefined reference to `image_get_host_blob'
>  /usr/bin/ld: tools/common/image-cipher.o:(.data.rel+0x10): undefined reference to `EVP_aes_128_cbc'
>  /usr/bin/ld: tools/common/image-cipher.o:(.data.rel+0x40): undefined reference to `EVP_aes_192_cbc'
>  /usr/bin/ld: tools/common/image-cipher.o:(.data.rel+0x70): undefined reference to `EVP_aes_256_cbc'
>  /usr/bin/ld: tools/lib/aes/aes-encrypt.o: in function `image_aes_encrypt':
>  aes-encrypt.c:(.text+0x22): undefined reference to `EVP_CIPHER_CTX_new'
>  /usr/bin/ld: aes-encrypt.c:(.text+0x6f): undefined reference to `EVP_EncryptInit_ex'
>  /usr/bin/ld: aes-encrypt.c:(.text+0x8d): undefined reference to `EVP_EncryptUpdate'
>  /usr/bin/ld: aes-encrypt.c:(.text+0xac): undefined reference to `EVP_CIPHER_CTX_free'
>  /usr/bin/ld: aes-encrypt.c:(.text+0xf2): undefined reference to `EVP_EncryptFinal_ex'
>  collect2: error: ld returned 1 exit status
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>

Applied to u-boot/master, thanks!

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

[PATCH v2 2/4] image-fit: Fix FIT_CIPHER linking

[Joel Stanley]

When CONFIG_FIT_CIPHER=y and CONFIG_FIT_SIGNATURE=n is there is no
implementation of image_get_host_blob for mkimage/dumpimage:

 /usr/bin/ld: tools/common/image-cipher.o: in function `fit_image_decrypt_data':
 image-cipher.c:(.text+0x9a): undefined reference to `image_get_host_blob'

Move the implementation to a common file so it can be shaed between
image-cipher.c and image-fit-sig.c.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
v2: Fix compilation when signature and ciphering are both enabled
---
 common/image-fit-sig.c | 14 --------------
 common/image-fit.c     | 15 +++++++++++++++
 2 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/common/image-fit-sig.c b/common/image-fit-sig.c
index 5401d9411b98..d39741e9058f 100644
--- a/common/image-fit-sig.c
+++ b/common/image-fit-sig.c
@@ -19,20 +19,6 @@ DECLARE_GLOBAL_DATA_PTR;
 
 #define IMAGE_MAX_HASHED_NODES		100
 
-#ifdef USE_HOSTCC
-void *host_blob;
-
-void image_set_host_blob(void *blob)
-{
-	host_blob = blob;
-}
-
-void *image_get_host_blob(void)
-{
-	return host_blob;
-}
-#endif
-
 /**
  * fit_region_make_list() - Make a list of image regions
  *
diff --git a/common/image-fit.c b/common/image-fit.c
index c82d4d8015f0..664a0d00006c 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -112,6 +112,21 @@ int fit_parse_subimage(const char *spec, ulong addr_curr,
 }
 #endif /* !USE_HOSTCC */
 
+#ifdef USE_HOSTCC
+/* Host tools use these implementations for Cipher and Signature support */
+static void *host_blob;
+
+void image_set_host_blob(void *blob)
+{
+	host_blob = blob;
+}
+
+void *image_get_host_blob(void)
+{
+	return host_blob;
+}
+#endif /* USE_HOSTCC */
+
 static void fit_get_debug(const void *fit, int noffset,
 		char *prop_name, int err)
 {
-- 
2.29.2

[PATCH v2 2/4] image-fit: Fix FIT_CIPHER linking

[Philippe REYNES]

Hi Joel


Le 08/12/2020 ? 05:12, Joel Stanley a ?crit?:
> When CONFIG_FIT_CIPHER=y and CONFIG_FIT_SIGNATURE=n is there is no
> implementation of image_get_host_blob for mkimage/dumpimage:
>
>   /usr/bin/ld: tools/common/image-cipher.o: in function `fit_image_decrypt_data':
>   image-cipher.c:(.text+0x9a): undefined reference to `image_get_host_blob'
>
> Move the implementation to a common file so it can be shaed between
> image-cipher.c and image-fit-sig.c.
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
 ??? Reviewed-by: Philippe Reynes <philippe.reynes@softathome.com>

> ---
> v2: Fix compilation when signature and ciphering are both enabled
> ---
>   common/image-fit-sig.c | 14 --------------
>   common/image-fit.c     | 15 +++++++++++++++
>   2 files changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/common/image-fit-sig.c b/common/image-fit-sig.c
> index 5401d9411b98..d39741e9058f 100644
> --- a/common/image-fit-sig.c
> +++ b/common/image-fit-sig.c
> @@ -19,20 +19,6 @@ DECLARE_GLOBAL_DATA_PTR;
>   
>   #define IMAGE_MAX_HASHED_NODES		100
>   
> -#ifdef USE_HOSTCC
> -void *host_blob;
> -
> -void image_set_host_blob(void *blob)
> -{
> -	host_blob = blob;
> -}
> -
> -void *image_get_host_blob(void)
> -{
> -	return host_blob;
> -}
> -#endif
> -
>   /**
>    * fit_region_make_list() - Make a list of image regions
>    *
> diff --git a/common/image-fit.c b/common/image-fit.c
> index c82d4d8015f0..664a0d00006c 100644
> --- a/common/image-fit.c
> +++ b/common/image-fit.c
> @@ -112,6 +112,21 @@ int fit_parse_subimage(const char *spec, ulong addr_curr,
>   }
>   #endif /* !USE_HOSTCC */
>   
> +#ifdef USE_HOSTCC
> +/* Host tools use these implementations for Cipher and Signature support */
> +static void *host_blob;
> +
> +void image_set_host_blob(void *blob)
> +{
> +	host_blob = blob;
> +}
> +
> +void *image_get_host_blob(void)
> +{
> +	return host_blob;
> +}
> +#endif /* USE_HOSTCC */
> +
>   static void fit_get_debug(const void *fit, int noffset,
>   		char *prop_name, int err)
>   {
Regards,
Philippe

[PATCH v2 2/4] image-fit: Fix FIT_CIPHER linking

[Tom Rini]

On Tue, Dec 08, 2020 at 02:42:14PM +1030, Joel Stanley wrote:

> When CONFIG_FIT_CIPHER=y and CONFIG_FIT_SIGNATURE=n is there is no
> implementation of image_get_host_blob for mkimage/dumpimage:
> 
>  /usr/bin/ld: tools/common/image-cipher.o: in function `fit_image_decrypt_data':
>  image-cipher.c:(.text+0x9a): undefined reference to `image_get_host_blob'
> 
> Move the implementation to a common file so it can be shaed between
> image-cipher.c and image-fit-sig.c.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>

Applied to u-boot/master, thanks!

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

[PATCH v2 3/4] mkimage: Move padding commands outside of FIT_SIGNATURE

[Joel Stanley]

These commands were disabled when CONFIG_FIT_SIGNATURE is disabled, but
they do not depend on crypto support so they can be unconditionally
enabled.

Signed-off-by: Joel Stanley <joel@jms.id.au>
--
v2: New patch
---
 tools/mkimage.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/tools/mkimage.c b/tools/mkimage.c
index e78608293e72..68d5206cb4fd 100644
--- a/tools/mkimage.c
+++ b/tools/mkimage.c
@@ -94,18 +94,18 @@ static void usage(const char *msg)
 		"          -x ==> set XIP (execute in place)\n",
 		params.cmdname);
 	fprintf(stderr,
-		"       %s [-D dtc_options] [-f fit-image.its|-f auto|-F] [-b <dtb> [-b <dtb>]] [-i <ramdisk.cpio.gz>] fit-image\n"
+		"       %s [-D dtc_options] [-f fit-image.its|-f auto|-F] [-b <dtb> [-b <dtb>]] [-E] [-B size] [-i <ramdisk.cpio.gz>] fit-image\n"
 		"           <dtb> file is used with -f auto, it may occur multiple times.\n",
 		params.cmdname);
 	fprintf(stderr,
 		"          -D => set all options for device tree compiler\n"
 		"          -f => input filename for FIT source\n"
-		"          -i => input filename for ramdisk file\n");
+		"          -i => input filename for ramdisk file\n"
+		"          -E => place data outside of the FIT structure\n"
+		"          -B => align size in hex for FIT structure and header\n");
 #ifdef CONFIG_FIT_SIGNATURE
 	fprintf(stderr,
-		"Signing / verified boot options: [-E] [-B size] [-k keydir] [-K dtb] [ -c <comment>] [-p addr] [-r] [-N engine]\n"
-		"          -E => place data outside of the FIT structure\n"
-		"          -B => align size in hex for FIT structure and header\n"
+		"Signing / verified boot options: [-k keydir] [-K dtb] [ -c <comment>] [-p addr] [-r] [-N engine]\n"
 		"          -k => set directory containing private keys\n"
 		"          -K => write public keys to this .dtb file\n"
 		"          -c => add comment in signature node\n"
@@ -142,6 +142,7 @@ static int add_content(int type, const char *fname)
 	return 0;
 }
 
+#define OPT_STRING "a:A:b:B:c:C:d:D:e:Ef:Fk:i:K:ln:N:p:O:rR:qstT:vVx"
 static void process_args(int argc, char **argv)
 {
 	char *ptr;
-- 
2.29.2

[PATCH v2 3/4] mkimage: Move padding commands outside of FIT_SIGNATURE

[Philippe REYNES]

Hi Joel


Le 08/12/2020 ? 05:12, Joel Stanley a ?crit?:
> These commands were disabled when CONFIG_FIT_SIGNATURE is disabled, but
> they do not depend on crypto support so they can be unconditionally
> enabled.
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> --
> v2: New patch
> ---
>   tools/mkimage.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/tools/mkimage.c b/tools/mkimage.c
> index e78608293e72..68d5206cb4fd 100644
> --- a/tools/mkimage.c
> +++ b/tools/mkimage.c
> @@ -94,18 +94,18 @@ static void usage(const char *msg)
>   		"          -x ==> set XIP (execute in place)\n",
>   		params.cmdname);
>   	fprintf(stderr,
> -		"       %s [-D dtc_options] [-f fit-image.its|-f auto|-F] [-b <dtb> [-b <dtb>]] [-i <ramdisk.cpio.gz>] fit-image\n"
> +		"       %s [-D dtc_options] [-f fit-image.its|-f auto|-F] [-b <dtb> [-b <dtb>]] [-E] [-B size] [-i <ramdisk.cpio.gz>] fit-image\n"
>   		"           <dtb> file is used with -f auto, it may occur multiple times.\n",
>   		params.cmdname);
>   	fprintf(stderr,
>   		"          -D => set all options for device tree compiler\n"
>   		"          -f => input filename for FIT source\n"
> -		"          -i => input filename for ramdisk file\n");
> +		"          -i => input filename for ramdisk file\n"
> +		"          -E => place data outside of the FIT structure\n"
> +		"          -B => align size in hex for FIT structure and header\n");
>   #ifdef CONFIG_FIT_SIGNATURE
>   	fprintf(stderr,
> -		"Signing / verified boot options: [-E] [-B size] [-k keydir] [-K dtb] [ -c <comment>] [-p addr] [-r] [-N engine]\n"
> -		"          -E => place data outside of the FIT structure\n"
> -		"          -B => align size in hex for FIT structure and header\n"
> +		"Signing / verified boot options: [-k keydir] [-K dtb] [ -c <comment>] [-p addr] [-r] [-N engine]\n"
>   		"          -k => set directory containing private keys\n"
>   		"          -K => write public keys to this .dtb file\n"
>   		"          -c => add comment in signature node\n"
Option -p is a generic option, not a signing option. it defines the 
offset of the padding.
So it should be moved above.

Option -k, -K are also used for ciphering. So it signature is disabled 
and ciphering is enabled, the help won't display those options.
I think it could be acceptable to? use #if 
CONFIG_IS_ENABLED(FIT_SIGNATURE) || CONFIG_IS_ENABLED(FIT_CIPHER)
and also change a bit the text "Signing / verified option ..."


> @@ -142,6 +142,7 @@ static int add_content(int type, const char 
> *fname) return 0; } +#define OPT_STRING 
> "a:A:b:B:c:C:d:D:e:Ef:Fk:i:K:ln:N:p:O:rR:qstT:vVx"

I think that OPT_STRING should be defined in the next patch.

>   static void process_args(int argc, char **argv)
>   {
>   	char *ptr;
Regards,
Philippe

[PATCH v2 3/4] mkimage: Move padding commands outside of FIT_SIGNATURE

[Tom Rini]

On Tue, Dec 08, 2020 at 02:42:15PM +1030, Joel Stanley wrote:

> These commands were disabled when CONFIG_FIT_SIGNATURE is disabled, but
> they do not depend on crypto support so they can be unconditionally
> enabled.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>

Applied to u-boot/master, thanks!

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

[PATCH v2 4/4] mkimge: Reject signing-related flags without FIT_SIGNATURE

[Joel Stanley]

When CONFIG_FIT_SIGNATURE=n the signing options are not available. If a
user is careful they will notice this when looking at the help output.

If they are not careful they will waste several hours wondering why
their FIT doesn't contain a /signature node, as mkimage will silently
ingore the signing related options.

Make it obvious that the commands don't work by removing them from the
getopt opt_string.

 $ mkimage -f machine.its -k keys -K u-boot-pubkey.dtb -r image.fit
 mkimage: invalid option -- 'k'
 Error: Invalid option

Signed-off-by: Joel Stanley <joel@jms.id.au>
--
v2: Leave padding related options in the CONFIG_FIT_SIGNATURE=y optargs
---
 tools/mkimage.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/mkimage.c b/tools/mkimage.c
index 68d5206cb4fd..f7d3ac40e681 100644
--- a/tools/mkimage.c
+++ b/tools/mkimage.c
@@ -142,7 +142,11 @@ static int add_content(int type, const char *fname)
 	return 0;
 }
 
+#ifdef CONFIG_FIT_SIGNATURE
 #define OPT_STRING "a:A:b:B:c:C:d:D:e:Ef:Fk:i:K:ln:N:p:O:rR:qstT:vVx"
+#else
+#define OPT_STRING "a:A:b:B:C:d:D:e:Ef:i:ln:O:R:qstT:vVx"
+#endif
 static void process_args(int argc, char **argv)
 {
 	char *ptr;
@@ -150,8 +154,7 @@ static void process_args(int argc, char **argv)
 	char *datafile = NULL;
 	int opt;
 
-	while ((opt = getopt(argc, argv,
-		   "a:A:b:B:c:C:d:D:e:Ef:Fk:i:K:ln:N:p:O:rR:qstT:vVx")) != -1) {
+	while ((opt = getopt(argc, argv, OPT_STRING)) != -1) {
 		switch (opt) {
 		case 'a':
 			params.addr = strtoull(optarg, &ptr, 16);
-- 
2.29.2

[PATCH v2 4/4] mkimge: Reject signing-related flags without FIT_SIGNATURE

[Philippe REYNES]

Hi Joel


Le 08/12/2020 ? 05:12, Joel Stanley a ?crit?:
> When CONFIG_FIT_SIGNATURE=n the signing options are not available. If a
> user is careful they will notice this when looking at the help output.
>
> If they are not careful they will waste several hours wondering why
> their FIT doesn't contain a /signature node, as mkimage will silently
> ingore the signing related options.
>
> Make it obvious that the commands don't work by removing them from the
> getopt opt_string.
>
>   $ mkimage -f machine.its -k keys -K u-boot-pubkey.dtb -r image.fit
>   mkimage: invalid option -- 'k'
>   Error: Invalid option
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> --
> v2: Leave padding related options in the CONFIG_FIT_SIGNATURE=y optargs
> ---
>   tools/mkimage.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/tools/mkimage.c b/tools/mkimage.c
> index 68d5206cb4fd..f7d3ac40e681 100644
> --- a/tools/mkimage.c
> +++ b/tools/mkimage.c
> @@ -142,7 +142,11 @@ static int add_content(int type, const char *fname)
>   	return 0;
>   }
>   
> +#ifdef CONFIG_FIT_SIGNATURE
>   #define OPT_STRING "a:A:b:B:c:C:d:D:e:Ef:Fk:i:K:ln:N:p:O:rR:qstT:vVx"
Option -k and -K is also needed for ciphering. So we should also check 
FIT_CIPHER.
Option -p is "generic", it is used to define the size of the padding.
> +#else
> +#define OPT_STRING "a:A:b:B:C:d:D:e:Ef:i:ln:O:R:qstT:vVx"
> +#endif
>   static void process_args(int argc, char **argv)
>   {
>   	char *ptr;
> @@ -150,8 +154,7 @@ static void process_args(int argc, char **argv)
>   	char *datafile = NULL;
>   	int opt;
>   
> -	while ((opt = getopt(argc, argv,
> -		   "a:A:b:B:c:C:d:D:e:Ef:Fk:i:K:ln:N:p:O:rR:qstT:vVx")) != -1) {
> +	while ((opt = getopt(argc, argv, OPT_STRING)) != -1) {
>   		switch (opt) {
>   		case 'a':
>   			params.addr = strtoull(optarg, &ptr, 16);
Regards,
Philippe

[PATCH v2 4/4] mkimge: Reject signing-related flags without FIT_SIGNATURE

[Simon Glass]

Hi,

On Tue, 8 Dec 2020 at 08:38, Philippe REYNES
<philippe.reynes@softathome.com> wrote:
>
> Hi Joel
>
>
> Le 08/12/2020 ? 05:12, Joel Stanley a ?crit :
> > When CONFIG_FIT_SIGNATURE=n the signing options are not available. If a
> > user is careful they will notice this when looking at the help output.
> >
> > If they are not careful they will waste several hours wondering why
> > their FIT doesn't contain a /signature node, as mkimage will silently
> > ingore the signing related options.
> >
> > Make it obvious that the commands don't work by removing them from the
> > getopt opt_string.
> >
> >   $ mkimage -f machine.its -k keys -K u-boot-pubkey.dtb -r image.fit
> >   mkimage: invalid option -- 'k'
> >   Error: Invalid option
> >
> > Signed-off-by: Joel Stanley <joel@jms.id.au>
> > --
> > v2: Leave padding related options in the CONFIG_FIT_SIGNATURE=y optargs
> > ---
> >   tools/mkimage.c | 7 +++++--
> >   1 file changed, 5 insertions(+), 2 deletions(-)
> >

I have somehow missed these patches on the mailing list. I'm not sure
why, but I'm not going to review them as is.

Regards,
Simon

[PATCH v2 4/4] mkimge: Reject signing-related flags without FIT_SIGNATURE

[Tom Rini]

On Tue, Dec 08, 2020 at 02:42:16PM +1030, Joel Stanley wrote:

> When CONFIG_FIT_SIGNATURE=n the signing options are not available. If a
> user is careful they will notice this when looking at the help output.
> 
> If they are not careful they will waste several hours wondering why
> their FIT doesn't contain a /signature node, as mkimage will silently
> ingore the signing related options.
> 
> Make it obvious that the commands don't work by removing them from the
> getopt opt_string.
> 
>  $ mkimage -f machine.its -k keys -K u-boot-pubkey.dtb -r image.fit
>  mkimage: invalid option -- 'k'
>  Error: Invalid option
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> v2: Leave padding related options in the CONFIG_FIT_SIGNATURE=y optargs
> ---
>  tools/mkimage.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/mkimage.c b/tools/mkimage.c
> index 68d5206cb4fd..f7d3ac40e681 100644
> --- a/tools/mkimage.c
> +++ b/tools/mkimage.c
> @@ -142,7 +142,11 @@ static int add_content(int type, const char *fname)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_FIT_SIGNATURE
>  #define OPT_STRING "a:A:b:B:c:C:d:D:e:Ef:Fk:i:K:ln:N:p:O:rR:qstT:vVx"
> +#else
> +#define OPT_STRING "a:A:b:B:C:d:D:e:Ef:i:ln:O:R:qstT:vVx"
> +#endif
>  static void process_args(int argc, char **argv)
>  {
>  	char *ptr;
> @@ -150,8 +154,7 @@ static void process_args(int argc, char **argv)
>  	char *datafile = NULL;
>  	int opt;
>  
> -	while ((opt = getopt(argc, argv,
> -		   "a:A:b:B:c:C:d:D:e:Ef:Fk:i:K:ln:N:p:O:rR:qstT:vVx")) != -1) {
> +	while ((opt = getopt(argc, argv, OPT_STRING)) != -1) {
>  		switch (opt) {
>  		case 'a':
>  			params.addr = strtoull(optarg, &ptr, 16);

This part of the series breaks a huge number of platforms default build
because we don't have 'p' which is:
          -p => place external data at a static position
and not strictly used in signature only options and is used for
FIT_EXTERNAL_OFFSET.  While I could adjust the patch to keep 'p' in both
cases I wanted to bring this up so it's not a surprise and ask you to
update just this patch, or suggest things need to be further adjusted.
The rest of the series seems fine and is under review.  Thanks!

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