[U-Boot] [PATCH] fit: skip signature verification if board request

[U-Boot] [PATCH] fit: skip signature verification if board request

[Jun Nie]

It may be unnecessary to check signature on unlocked board.
Get the hint from platform specific code to support secure boot
and non-secure boot with the same binary, so that boot is not
blocked if board is not locked and has no key for signature
verification.

Signed-off-by: Jun Nie <jun.nie@linaro.org>
---
 common/image-sig.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/common/image-sig.c b/common/image-sig.c
index d9f712f..f3d1252 100644
--- a/common/image-sig.c
+++ b/common/image-sig.c
@@ -151,6 +151,11 @@ struct image_region *fit_region_make_list(const void *fit,
 	return region;
 }
 
+int __attribute__((weak)) fit_board_skip_sig_verification(void)
+{
+	return 0;
+}
+
 static int fit_image_setup_verify(struct image_sign_info *info,
 		const void *fit, int noffset, int required_keynode,
 		char **err_msgp)
@@ -188,6 +193,12 @@ int fit_image_check_sig(const void *fit, int noffset, const void *data,
 	uint8_t *fit_value;
 	int fit_value_len;
 
+	/* Skip verification if board says that */
+	if (fit_board_skip_sig_verification()) {
+		printf("signature check skipped\n");
+		return 0;
+	}
+
 	*err_msgp = NULL;
 	if (fit_image_setup_verify(&info, fit, noffset, required_keynode,
 				   err_msgp))
@@ -438,6 +449,12 @@ int fit_config_verify_required_sigs(const void *fit, int conf_noffset,
 	int noffset;
 	int sig_node;
 
+	/* Skip verification if board says that */
+	if (fit_board_skip_sig_verification()) {
+		printf("signature check skipped\n");
+		return 0;
+	}
+
 	/* Work out what we need to verify */
 	sig_node = fdt_subnode_offset(sig_blob, 0, FIT_SIG_NODENAME);
 	if (sig_node < 0) {
-- 
2.7.4

[U-Boot] [PATCH] fit: skip signature verification if board request

[Tom Rini]

On Wed, Apr 11, 2018 at 11:13:05PM +0800, Jun Nie wrote:

> It may be unnecessary to check signature on unlocked board.
> Get the hint from platform specific code to support secure boot
> and non-secure boot with the same binary, so that boot is not
> blocked if board is not locked and has no key for signature
> verification.
> 
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  common/image-sig.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/common/image-sig.c b/common/image-sig.c
> index d9f712f..f3d1252 100644
> --- a/common/image-sig.c
> +++ b/common/image-sig.c
> @@ -151,6 +151,11 @@ struct image_region *fit_region_make_list(const void *fit,
>  	return region;
>  }
>  
> +int __attribute__((weak)) fit_board_skip_sig_verification(void)
> +{
> +	return 0;
> +}
> +
>  static int fit_image_setup_verify(struct image_sign_info *info,
>  		const void *fit, int noffset, int required_keynode,
>  		char **err_msgp)
> @@ -188,6 +193,12 @@ int fit_image_check_sig(const void *fit, int noffset, const void *data,
>  	uint8_t *fit_value;
>  	int fit_value_len;
>  
> +	/* Skip verification if board says that */
> +	if (fit_board_skip_sig_verification()) {
> +		printf("signature check skipped\n");
> +		return 0;
> +	}
> +
>  	*err_msgp = NULL;
>  	if (fit_image_setup_verify(&info, fit, noffset, required_keynode,
>  				   err_msgp))
> @@ -438,6 +449,12 @@ int fit_config_verify_required_sigs(const void *fit, int conf_noffset,
>  	int noffset;
>  	int sig_node;
>  
> +	/* Skip verification if board says that */
> +	if (fit_board_skip_sig_verification()) {
> +		printf("signature check skipped\n");
> +		return 0;
> +	}
> +
>  	/* Work out what we need to verify */
>  	sig_node = fdt_subnode_offset(sig_blob, 0, FIT_SIG_NODENAME);
>  	if (sig_node < 0) {

I'm not sure I like the concept here.  Wouldn't this make it easier to
break in to a secure setup with some binary editing?  Or is that really
no worse than today?  Also, can you please follow up with an
implementation of fit_board_skip_sig_verification?  Thanks!

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

[U-Boot] [PATCH] fit: skip signature verification if board request

[Jun Nie]

2018-04-12 21:53 GMT+08:00 Tom Rini <trini@konsulko.com>:
> On Wed, Apr 11, 2018 at 11:13:05PM +0800, Jun Nie wrote:
>
>> It may be unnecessary to check signature on unlocked board.
>> Get the hint from platform specific code to support secure boot
>> and non-secure boot with the same binary, so that boot is not
>> blocked if board is not locked and has no key for signature
>> verification.
>>
>> Signed-off-by: Jun Nie <jun.nie@linaro.org>
>> ---
> I'm not sure I like the concept here.  Wouldn't this make it easier to
> break in to a secure setup with some binary editing?  Or is that really
> no worse than today?  Also, can you please follow up with an
> implementation of fit_board_skip_sig_verification?  Thanks!
>
> --
> Tom

SoC boot ROM shall verify signature of SPL before running. Binary
editing shall make signature invalid. If it is possible to run an
edited SPL, the secure boot is already hacked and this patch does not
make it worse. I surely will post implementation, which is just
reading some register to get fuse value for lock status. I post this
patch to get opinions from community first.

Jun

[U-Boot] [PATCH] fit: skip signature verification if board request

[Simon Glass]

Hi,

On 11 April 2018 at 09:13, Jun Nie <jun.nie@linaro.org> wrote:
> It may be unnecessary to check signature on unlocked board.
> Get the hint from platform specific code to support secure boot
> and non-secure boot with the same binary, so that boot is not
> blocked if board is not locked and has no key for signature
> verification.
>
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  common/image-sig.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/common/image-sig.c b/common/image-sig.c
> index d9f712f..f3d1252 100644
> --- a/common/image-sig.c
> +++ b/common/image-sig.c
> @@ -151,6 +151,11 @@ struct image_region *fit_region_make_list(const void *fit,
>         return region;
>  }
>
> +int __attribute__((weak)) fit_board_skip_sig_verification(void)
> +{
> +       return 0;
> +}
> +
>  static int fit_image_setup_verify(struct image_sign_info *info,
>                 const void *fit, int noffset, int required_keynode,
>                 char **err_msgp)
> @@ -188,6 +193,12 @@ int fit_image_check_sig(const void *fit, int noffset, const void *data,
>         uint8_t *fit_value;
>         int fit_value_len;
>
> +       /* Skip verification if board says that */
> +       if (fit_board_skip_sig_verification()) {
> +               printf("signature check skipped\n");
> +               return 0;
> +       }

Instead of a weak function can you please add a parameter to this
function (perhaps a flags word?) and a add test for this case to the
test?

Regards,
Simon

[U-Boot] [PATCH] fit: skip signature verification if board request

[Jun Nie]

2018-04-13 1:24 GMT+08:00 Simon Glass <sjg@chromium.org>:
> Hi,
>
> On 11 April 2018 at 09:13, Jun Nie <jun.nie@linaro.org> wrote:
>> It may be unnecessary to check signature on unlocked board.
>> Get the hint from platform specific code to support secure boot
>> and non-secure boot with the same binary, so that boot is not
>> blocked if board is not locked and has no key for signature
>> verification.
>>
>> Signed-off-by: Jun Nie <jun.nie@linaro.org>
>> ---
>
> Instead of a weak function can you please add a parameter to this
> function (perhaps a flags word?) and a add test for this case to the
> test?
>
> Regards,
> Simon

Sure, I can add a parameter to the function. But not sure what's the
usage of it in your mind. Maybe "int flag" is enough for you?

Do you mean add implementation of this function for a specific platform
for "add test"?

Best Regards,
Jun

[U-Boot] [PATCH] fit: skip signature verification if board request

[Simon Glass]

Hi Jun,

On 13 April 2018 at 04:05, Jun Nie <jun.nie@linaro.org> wrote:
> 2018-04-13 1:24 GMT+08:00 Simon Glass <sjg@chromium.org>:
>> Hi,
>>
>> On 11 April 2018 at 09:13, Jun Nie <jun.nie@linaro.org> wrote:
>>> It may be unnecessary to check signature on unlocked board.
>>> Get the hint from platform specific code to support secure boot
>>> and non-secure boot with the same binary, so that boot is not
>>> blocked if board is not locked and has no key for signature
>>> verification.
>>>
>>> Signed-off-by: Jun Nie <jun.nie@linaro.org>
>>> ---
>>
>> Instead of a weak function can you please add a parameter to this
>> function (perhaps a flags word?) and a add test for this case to the
>> test?
>>
>> Regards,
>> Simon
>
> Sure, I can add a parameter to the function. But not sure what's the
> usage of it in your mind. Maybe "int flag" is enough for you?

Yes a flag is fine if you define an enum in the header file for that
function. You will likely need to pass the flag around a few other
functions.
>
> Do you mean add implementation of this function for a specific platform
> for "add test"?

See test/py/tests/test_vboot.py which you should be able to modify for
your case.

Regards,
Simon

[U-Boot] [PATCH] fit: skip signature verification if board request

[Jun Nie]

2018-04-17 3:06 GMT+08:00 Simon Glass <sjg@chromium.org>:
> Hi Jun,
>
> On 13 April 2018 at 04:05, Jun Nie <jun.nie@linaro.org> wrote:
>> 2018-04-13 1:24 GMT+08:00 Simon Glass <sjg@chromium.org>:
>>> Hi,
>>>
>>> On 11 April 2018 at 09:13, Jun Nie <jun.nie@linaro.org> wrote:
>>>> It may be unnecessary to check signature on unlocked board.
>>>> Get the hint from platform specific code to support secure boot
>>>> and non-secure boot with the same binary, so that boot is not
>>>> blocked if board is not locked and has no key for signature
>>>> verification.
>>>>
>>>> Signed-off-by: Jun Nie <jun.nie@linaro.org>
>>>> ---
>>>
>>> Instead of a weak function can you please add a parameter to this
>>> function (perhaps a flags word?) and a add test for this case to the
>>> test?
>>>
>>> Regards,
>>> Simon
>>
>> Sure, I can add a parameter to the function. But not sure what's the
>> usage of it in your mind. Maybe "int flag" is enough for you?
>
> Yes a flag is fine if you define an enum in the header file for that
> function. You will likely need to pass the flag around a few other
> functions.

I am still unclear on what the usage of this flag. I mean what variable
from fit_image_setup_verify() and fit_config_verify_required_sigs()
to be feed to this function as a flag. Maybe a void pointer is better
for different platform to pass context data and cost to specific data
structure. For example, pass data of image that to be verified.
fit_board_skip_sig_verification(const void *data)

>>
>> Do you mean add implementation of this function for a specific platform
>> for "add test"?
>
> See test/py/tests/test_vboot.py which you should be able to modify for
> your case.

Will check this file for more modification and test.

Thank you!

>
> Regards,
> Simon

[U-Boot] [PATCH] fit: skip signature verification if board request

[Quentin Schulz]

Hi,

On Wed, Apr 11, 2018 at 11:13:05PM +0800, Jun Nie wrote:
> It may be unnecessary to check signature on unlocked board.
> Get the hint from platform specific code to support secure boot
> and non-secure boot with the same binary, so that boot is not
> blocked if board is not locked and has no key for signature
> verification.
> 

Isn't it what the environment variable `verify` is made for?

i.e. setting verify=no will skip checks and boot an image even though it
isn't signed or hash/signature does not match.

I may be missing some context here, so please ignore if it's not what
you're after.

BTW, I saw that you were speaking of reading the lock fuse to decide
whether to check the signature or not. I'd like to have at least a
bypass option for this as it would be horribly tedious for
debugging/development purposes. E.g. I want to be able to boot from an
unverified U-Boot binary a signed (and checked) fitImage so that I can
validate everything works as it should before locking down the
bootloader.

Regards,
Quentin

> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  common/image-sig.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/common/image-sig.c b/common/image-sig.c
> index d9f712f..f3d1252 100644
> --- a/common/image-sig.c
> +++ b/common/image-sig.c
> @@ -151,6 +151,11 @@ struct image_region *fit_region_make_list(const void *fit,
>  	return region;
>  }
>  
> +int __attribute__((weak)) fit_board_skip_sig_verification(void)
> +{
> +	return 0;
> +}
> +
>  static int fit_image_setup_verify(struct image_sign_info *info,
>  		const void *fit, int noffset, int required_keynode,
>  		char **err_msgp)
> @@ -188,6 +193,12 @@ int fit_image_check_sig(const void *fit, int noffset, const void *data,
>  	uint8_t *fit_value;
>  	int fit_value_len;
>  
> +	/* Skip verification if board says that */
> +	if (fit_board_skip_sig_verification()) {
> +		printf("signature check skipped\n");
> +		return 0;
> +	}
> +
>  	*err_msgp = NULL;
>  	if (fit_image_setup_verify(&info, fit, noffset, required_keynode,
>  				   err_msgp))
> @@ -438,6 +449,12 @@ int fit_config_verify_required_sigs(const void *fit, int conf_noffset,
>  	int noffset;
>  	int sig_node;
>  
> +	/* Skip verification if board says that */
> +	if (fit_board_skip_sig_verification()) {
> +		printf("signature check skipped\n");
> +		return 0;
> +	}
> +
>  	/* Work out what we need to verify */
>  	sig_node = fdt_subnode_offset(sig_blob, 0, FIT_SIG_NODENAME);
>  	if (sig_node < 0) {
> -- 
> 2.7.4
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180417/c1992908/attachment.sig>

[U-Boot] [PATCH] fit: skip signature verification if board request

[Jun Nie]

2018-04-17 20:06 GMT+08:00 Quentin Schulz <quentin.schulz@bootlin.com>:
> Hi,
>
> On Wed, Apr 11, 2018 at 11:13:05PM +0800, Jun Nie wrote:
>> It may be unnecessary to check signature on unlocked board.
>> Get the hint from platform specific code to support secure boot
>> and non-secure boot with the same binary, so that boot is not
>> blocked if board is not locked and has no key for signature
>> verification.
>>
>
> Isn't it what the environment variable `verify` is made for?
>
> i.e. setting verify=no will skip checks and boot an image even though it
> isn't signed or hash/signature does not match.
>
> I may be missing some context here, so please ignore if it's not what
> you're after.

Thanks for pointing me for this. I check code and find that this variable
does not cover all signature verification cases, such as fit_image_verify().
There is no variable in SPL neither, I suppose.
>
> BTW, I saw that you were speaking of reading the lock fuse to decide
> whether to check the signature or not. I'd like to have at least a
> bypass option for this as it would be horribly tedious for
> debugging/development purposes. E.g. I want to be able to boot from an
> unverified U-Boot binary a signed (and checked) fitImage so that I can
> validate everything works as it should before locking down the
> bootloader.

For this case, it is OK if you do not detect lock fuse value and use default
weak function to indicate no skipping.

>
> Regards,
> Quentin
>
>> Signed-off-by: Jun Nie <jun.nie@linaro.org>
>> ---
>>  common/image-sig.c | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/common/image-sig.c b/common/image-sig.c
>> index d9f712f..f3d1252 100644
>> --- a/common/image-sig.c
>> +++ b/common/image-sig.c
>> @@ -151,6 +151,11 @@ struct image_region *fit_region_make_list(const void *fit,
>>       return region;
>>  }
>>
>> +int __attribute__((weak)) fit_board_skip_sig_verification(void)
>> +{
>> +     return 0;
>> +}
>> +
>>  static int fit_image_setup_verify(struct image_sign_info *info,
>>               const void *fit, int noffset, int required_keynode,
>>               char **err_msgp)
>> @@ -188,6 +193,12 @@ int fit_image_check_sig(const void *fit, int noffset, const void *data,
>>       uint8_t *fit_value;
>>       int fit_value_len;
>>
>> +     /* Skip verification if board says that */
>> +     if (fit_board_skip_sig_verification()) {
>> +             printf("signature check skipped\n");
>> +             return 0;
>> +     }
>> +
>>       *err_msgp = NULL;
>>       if (fit_image_setup_verify(&info, fit, noffset, required_keynode,
>>                                  err_msgp))
>> @@ -438,6 +449,12 @@ int fit_config_verify_required_sigs(const void *fit, int conf_noffset,
>>       int noffset;
>>       int sig_node;
>>
>> +     /* Skip verification if board says that */
>> +     if (fit_board_skip_sig_verification()) {
>> +             printf("signature check skipped\n");
>> +             return 0;
>> +     }
>> +
>>       /* Work out what we need to verify */
>>       sig_node = fdt_subnode_offset(sig_blob, 0, FIT_SIG_NODENAME);
>>       if (sig_node < 0) {
>> --
>> 2.7.4
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> https://lists.denx.de/listinfo/u-boot