diff mbox series

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

Message ID 20201208041216.888902-5-joel@jms.id.au
State Changes Requested
Delegated to: Tom Rini
Headers show
Series mkimage usability fixes | expand

Commit Message

Joel Stanley Dec. 8, 2020, 4:12 a.m. UTC
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(-)

Comments

Philippe REYNES Dec. 8, 2020, 3:38 p.m. UTC | #1
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
Simon Glass Dec. 12, 2020, 3:39 p.m. UTC | #2
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
Tom Rini Jan. 22, 2021, 9:58 p.m. UTC | #3
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!
diff mbox series

Patch

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);