| Message ID | 1523459585-7594-1-git-send-email-jun.nie@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | fit: skip signature verification if board request | expand |
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
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
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
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
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
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@lists.denx.de > https://lists.denx.de/listinfo/u-boot
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@lists.denx.de >> https://lists.denx.de/listinfo/u-boot
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
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) {
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(+)