| Message ID | 1426063900-7267-1-git-send-email-sr@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
On Wed, Mar 11, 2015 at 09:51:37AM +0100, Stefan Roese wrote: > This patch adds the feature to only stop the autobooting, and therefor > boot into the U-Boot prompt, when the input string / password matches > a values that is encypted via a SHA256 hash and saved in the environment. > > This feature is enabled by defined these config options: > CONFIG_AUTOBOOT_KEYED > CONFIG_AUTOBOOT_STOP_STR_SHA256 > > Signed-off-by: Stefan Roese <sr@denx.de> This is certainly interesting but I think brings us back to a point Simon made a long while back about needing to factor out this code better. Especially since this adds big long #if-#else-#endif blocks. Can we re-do this so at least have some functions be called out instead? Thanks!
Hi Tom, On 11.03.2015 15:36, Tom Rini wrote: > On Wed, Mar 11, 2015 at 09:51:37AM +0100, Stefan Roese wrote: > >> This patch adds the feature to only stop the autobooting, and therefor >> boot into the U-Boot prompt, when the input string / password matches >> a values that is encypted via a SHA256 hash and saved in the environment. >> >> This feature is enabled by defined these config options: >> CONFIG_AUTOBOOT_KEYED >> CONFIG_AUTOBOOT_STOP_STR_SHA256 >> >> Signed-off-by: Stefan Roese <sr@denx.de> > > This is certainly interesting but I think brings us back to a point > Simon made a long while back about needing to factor out this code > better. Especially since this adds big long #if-#else-#endif blocks. > Can we re-do this so at least have some functions be called out instead? Yes, I'll try to rework this patch a bit to make this feature integration less ugly. Thanks, Stefan
Hi, On 11 March 2015 at 08:36, Tom Rini <trini@konsulko.com> wrote: > > On Wed, Mar 11, 2015 at 09:51:37AM +0100, Stefan Roese wrote: > > > This patch adds the feature to only stop the autobooting, and therefor > > boot into the U-Boot prompt, when the input string / password matches > > a values that is encypted via a SHA256 hash and saved in the environment. > > > > This feature is enabled by defined these config options: > > CONFIG_AUTOBOOT_KEYED > > CONFIG_AUTOBOOT_STOP_STR_SHA256 > > > > Signed-off-by: Stefan Roese <sr@denx.de> > > This is certainly interesting but I think brings us back to a point > Simon made a long while back about needing to factor out this code > better. Especially since this adds big long #if-#else-#endif blocks. > Can we re-do this so at least have some functions be called out instead? > Thanks! > Also if these CONFIG options are in Kconfig (as they should be) then we can use if (IS_ENABLED(CONFIG_AUTOBOOT_STOP_STR_SHA256)) instead of #ifdef which may improve the code. Regards, Simon
Hi Simon, On 13.03.2015 03:48, Simon Glass wrote: >>> This patch adds the feature to only stop the autobooting, and therefor >>> boot into the U-Boot prompt, when the input string / password matches >>> a values that is encypted via a SHA256 hash and saved in the environment. >>> >>> This feature is enabled by defined these config options: >>> CONFIG_AUTOBOOT_KEYED >>> CONFIG_AUTOBOOT_STOP_STR_SHA256 >>> >>> Signed-off-by: Stefan Roese <sr@denx.de> >> >> This is certainly interesting but I think brings us back to a point >> Simon made a long while back about needing to factor out this code >> better. Especially since this adds big long #if-#else-#endif blocks. >> Can we re-do this so at least have some functions be called out instead? >> Thanks! >> > > Also if these CONFIG options are in Kconfig (as they should be) then we can use > > if (IS_ENABLED(CONFIG_AUTOBOOT_STOP_STR_SHA256)) > > instead of #ifdef which may improve the code. Right. I also thought about this. But the resulting code has all the functionality extracted into 2 functions: #if defined(CONFIG_AUTOBOOT_STOP_STR_SHA256) static int passwd_abort(uint64_t etime) { const char *sha_env_str = getenv("bootstopkeysha256"); ... } #else static int passwd_abort(uint64_t etime) { int abort = 0; ... } #endif And this function is now called unconditionally: ... abort = passwd_abort(etime); So there is nothing here that could be simplified by using IS_ENABLED(). I could of course just add this new config option to Kconfig. But with all the other related options not in Kconfig (CONFIG_AUTOBOOT_KEYED, CONFIG_AUTOBOOT_DELAY_STR...), this doesn't make much sense. So at some point all those config options should be moved to Kconfig. Unfortunately I don't have the time for this right now. But I'll add it to my list to do this at a later time. Thanks, Stefan
Hi Stefan, On 13 March 2015 at 01:15, Stefan Roese <sr@denx.de> wrote: > Hi Simon, > > On 13.03.2015 03:48, Simon Glass wrote: >>>> >>>> This patch adds the feature to only stop the autobooting, and therefor >>>> boot into the U-Boot prompt, when the input string / password matches >>>> a values that is encypted via a SHA256 hash and saved in the >>>> environment. >>>> >>>> This feature is enabled by defined these config options: >>>> CONFIG_AUTOBOOT_KEYED >>>> CONFIG_AUTOBOOT_STOP_STR_SHA256 >>>> >>>> Signed-off-by: Stefan Roese <sr@denx.de> >>> >>> >>> This is certainly interesting but I think brings us back to a point >>> Simon made a long while back about needing to factor out this code >>> better. Especially since this adds big long #if-#else-#endif blocks. >>> Can we re-do this so at least have some functions be called out instead? >>> Thanks! >>> >> >> Also if these CONFIG options are in Kconfig (as they should be) then we >> can use >> >> if (IS_ENABLED(CONFIG_AUTOBOOT_STOP_STR_SHA256)) >> >> instead of #ifdef which may improve the code. > > > Right. I also thought about this. But the resulting code has all the > functionality extracted into 2 functions: > > #if defined(CONFIG_AUTOBOOT_STOP_STR_SHA256) > static int passwd_abort(uint64_t etime) > { > const char *sha_env_str = getenv("bootstopkeysha256"); > ... > } > #else > static int passwd_abort(uint64_t etime) > { > int abort = 0; > ... > } > #endif > > And this function is now called unconditionally: > > ... > abort = passwd_abort(etime); > > So there is nothing here that could be simplified by using IS_ENABLED(). > > I could of course just add this new config option to Kconfig. But with all > the other related options not in Kconfig (CONFIG_AUTOBOOT_KEYED, > CONFIG_AUTOBOOT_DELAY_STR...), this doesn't make much sense. So at some > point all those config options should be moved to Kconfig. Unfortunately I > don't have the time for this right now. But I'll add it to my list to do > this at a later time. Well rather than adding more options, perhaps we should wait until we get this moved to Kconfig? It's not going to get any easier :-) Regards, Simon
Hi Simon, On 23.03.2015 21:28, Simon Glass wrote: > Hi Stefan, > > On 13 March 2015 at 01:15, Stefan Roese <sr@denx.de> wrote: >> Hi Simon, >> >> On 13.03.2015 03:48, Simon Glass wrote: >>>>> >>>>> This patch adds the feature to only stop the autobooting, and therefor >>>>> boot into the U-Boot prompt, when the input string / password matches >>>>> a values that is encypted via a SHA256 hash and saved in the >>>>> environment. >>>>> >>>>> This feature is enabled by defined these config options: >>>>> CONFIG_AUTOBOOT_KEYED >>>>> CONFIG_AUTOBOOT_STOP_STR_SHA256 >>>>> >>>>> Signed-off-by: Stefan Roese <sr@denx.de> >>>> >>>> >>>> This is certainly interesting but I think brings us back to a point >>>> Simon made a long while back about needing to factor out this code >>>> better. Especially since this adds big long #if-#else-#endif blocks. >>>> Can we re-do this so at least have some functions be called out instead? >>>> Thanks! >>>> >>> >>> Also if these CONFIG options are in Kconfig (as they should be) then we >>> can use >>> >>> if (IS_ENABLED(CONFIG_AUTOBOOT_STOP_STR_SHA256)) >>> >>> instead of #ifdef which may improve the code. >> >> >> Right. I also thought about this. But the resulting code has all the >> functionality extracted into 2 functions: >> >> #if defined(CONFIG_AUTOBOOT_STOP_STR_SHA256) >> static int passwd_abort(uint64_t etime) >> { >> const char *sha_env_str = getenv("bootstopkeysha256"); >> ... >> } >> #else >> static int passwd_abort(uint64_t etime) >> { >> int abort = 0; >> ... >> } >> #endif >> >> And this function is now called unconditionally: >> >> ... >> abort = passwd_abort(etime); >> >> So there is nothing here that could be simplified by using IS_ENABLED(). >> >> I could of course just add this new config option to Kconfig. But with all >> the other related options not in Kconfig (CONFIG_AUTOBOOT_KEYED, >> CONFIG_AUTOBOOT_DELAY_STR...), this doesn't make much sense. So at some >> point all those config options should be moved to Kconfig. Unfortunately I >> don't have the time for this right now. But I'll add it to my list to do >> this at a later time. > > Well rather than adding more options, perhaps we should wait until we > get this moved to Kconfig? It's not going to get any easier :-) Right. And now I'm finally back at this task. To get this encrypted password support into mainline. With Kconfig support of course this time. ;) Unfortunately I'm hitting a problem while moving some of the "old" macros to Kconfig. Especially some strings like CONFIG_AUTOBOOT_PROMPT. Here how this looks in some config headers: #define CONFIG_AUTOBOOT_PROMPT "autoboot in %d seconds\n", bootdelay This does not work, as Kconfig truncates the string after the 2nd '"'. Escaping this '"' using '\' also doesn't seem to work. Do you or Masahiro have some experience with this kind of Kconfig macro transition? Thanks, Stefan
Hi Stefan, On 5 May 2015 at 09:06, Stefan Roese <sr@denx.de> wrote: > Hi Simon, > > On 23.03.2015 21:28, Simon Glass wrote: >> Hi Stefan, >> >> On 13 March 2015 at 01:15, Stefan Roese <sr@denx.de> wrote: >>> Hi Simon, >>> >>> On 13.03.2015 03:48, Simon Glass wrote: >>>>>> >>>>>> This patch adds the feature to only stop the autobooting, and therefor >>>>>> boot into the U-Boot prompt, when the input string / password matches >>>>>> a values that is encypted via a SHA256 hash and saved in the >>>>>> environment. >>>>>> >>>>>> This feature is enabled by defined these config options: >>>>>> CONFIG_AUTOBOOT_KEYED >>>>>> CONFIG_AUTOBOOT_STOP_STR_SHA256 >>>>>> >>>>>> Signed-off-by: Stefan Roese <sr@denx.de> >>>>> >>>>> >>>>> This is certainly interesting but I think brings us back to a point >>>>> Simon made a long while back about needing to factor out this code >>>>> better. Especially since this adds big long #if-#else-#endif blocks. >>>>> Can we re-do this so at least have some functions be called out instead? >>>>> Thanks! >>>>> >>>> >>>> Also if these CONFIG options are in Kconfig (as they should be) then we >>>> can use >>>> >>>> if (IS_ENABLED(CONFIG_AUTOBOOT_STOP_STR_SHA256)) >>>> >>>> instead of #ifdef which may improve the code. >>> >>> >>> Right. I also thought about this. But the resulting code has all the >>> functionality extracted into 2 functions: >>> >>> #if defined(CONFIG_AUTOBOOT_STOP_STR_SHA256) >>> static int passwd_abort(uint64_t etime) >>> { >>> const char *sha_env_str = getenv("bootstopkeysha256"); >>> ... >>> } >>> #else >>> static int passwd_abort(uint64_t etime) >>> { >>> int abort = 0; >>> ... >>> } >>> #endif >>> >>> And this function is now called unconditionally: >>> >>> ... >>> abort = passwd_abort(etime); >>> >>> So there is nothing here that could be simplified by using IS_ENABLED(). >>> >>> I could of course just add this new config option to Kconfig. But with all >>> the other related options not in Kconfig (CONFIG_AUTOBOOT_KEYED, >>> CONFIG_AUTOBOOT_DELAY_STR...), this doesn't make much sense. So at some >>> point all those config options should be moved to Kconfig. Unfortunately I >>> don't have the time for this right now. But I'll add it to my list to do >>> this at a later time. >> >> Well rather than adding more options, perhaps we should wait until we >> get this moved to Kconfig? It's not going to get any easier :-) > > Right. And now I'm finally back at this task. To get this encrypted > password support into mainline. With Kconfig support of course this > time. ;) > > Unfortunately I'm hitting a problem while moving some of the "old" > macros to Kconfig. Especially some strings like CONFIG_AUTOBOOT_PROMPT. > Here how this looks in some config headers: > > #define CONFIG_AUTOBOOT_PROMPT "autoboot in %d seconds\n", bootdelay > > This does not work, as Kconfig truncates the string after the 2nd > '"'. Escaping this '"' using '\' also doesn't seem to work. Do you > or Masahiro have some experience with this kind of Kconfig macro > transition? Not me. I noticed this when refactoring the code. IMO it is broken - we should not be doing things like that. From what I can see we only ever pass bootdelay as a parameter. So perhaps you can drop the ", bootdelay" part and adjust the code in from common/autoboot.c from: printf(CONFIG_AUTOBOOT_PROMPT); to: printf(CONFIG_AUTOBOOT_PROMPT, bootdelay); Of course that will create printf() warnings for a few boards but it should be possible to turn them off at that call site. Regards, Simon
diff --git a/common/autoboot.c b/common/autoboot.c index c27cc2c..4635551 100644 --- a/common/autoboot.c +++ b/common/autoboot.c @@ -12,6 +12,7 @@ #include <fdtdec.h> #include <menu.h> #include <post.h> +#include <u-boot/sha256.h> DECLARE_GLOBAL_DATA_PTR; @@ -35,6 +36,11 @@ static int abortboot_keyed(int bootdelay) { int abort = 0; uint64_t etime = endtick(bootdelay); +#if defined(CONFIG_AUTOBOOT_STOP_STR_SHA256) + const char *sha_env_str = getenv("bootstopkeysha256"); + u8 sha_env[SHA256_SUM_LEN]; + u8 sha[SHA256_SUM_LEN]; +#else struct { char *str; u_int len; @@ -46,10 +52,11 @@ static int abortboot_keyed(int bootdelay) { .str = getenv("bootstopkey"), .retry = 0 }, { .str = getenv("bootstopkey2"), .retry = 0 }, }; + u_int presskey_max = 0; +#endif char presskey[MAX_DELAY_STOP_STR]; u_int presskey_len = 0; - u_int presskey_max = 0; u_int i; #ifndef CONFIG_ZERO_BOOTDELAY_CHECK @@ -61,6 +68,41 @@ static int abortboot_keyed(int bootdelay) printf(CONFIG_AUTOBOOT_PROMPT); # endif +#if defined(CONFIG_AUTOBOOT_STOP_STR_SHA256) + if (sha_env_str == NULL) + sha_env_str = CONFIG_AUTOBOOT_STOP_STR_SHA256; + + /* + * Generate the binary value from the environment hash value + * so that we can compare this value with the computed hash + * from the user input + */ + for (i = 0; i < SHA256_SUM_LEN; i++) { + char chr[3]; + + strncpy(chr, &sha_env_str[i * 2], 2); + sha_env[i] = simple_strtoul(chr, NULL, 16); + } + + /* + * We don't know how long the stop-string is, so we need to + * generate the sha256 hash upon each input character and + * compare the value with the one saved in the environment + */ + do { + if (tstc()) { + presskey[presskey_len++] = getc(); + + /* Calculate sha256 upon each new char */ + sha256_csum_wd((unsigned char *)presskey, presskey_len, + sha, CHUNKSZ_SHA256); + + /* And check if sha matches saved value in env */ + if (memcmp(sha, sha_env, SHA256_SUM_LEN) == 0) + abort = 1; + } + } while (!abort && get_ticks() <= etime); +#else # ifdef CONFIG_AUTOBOOT_DELAY_STR if (delaykey[0].str == NULL) delaykey[0].str = CONFIG_AUTOBOOT_DELAY_STR; @@ -124,6 +166,7 @@ static int abortboot_keyed(int bootdelay) } } } while (!abort && get_ticks() <= etime); +#endif if (!abort) debug_bootkeys("key timeout\n");
This patch adds the feature to only stop the autobooting, and therefor boot into the U-Boot prompt, when the input string / password matches a values that is encypted via a SHA256 hash and saved in the environment. This feature is enabled by defined these config options: CONFIG_AUTOBOOT_KEYED CONFIG_AUTOBOOT_STOP_STR_SHA256 Signed-off-by: Stefan Roese <sr@denx.de> --- common/autoboot.c | 45 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-)