Re: [U-Boot] [PATCH] autoboot.c: Add feature to stop autobooting via SHA256 encrypted password
Hi Stefan,
On 5 May 2015 at 09:06, Stefan Roese wrote:
> Hi Simon,
>
> On 23.03.2015 21:28, Simon Glass wrote:
>> Hi Stefan,
>>
>> On 13 March 2015 at 01:15, Stefan Roese 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
>
>
> 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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] autoboot.c: Add feature to stop autobooting via SHA256 encrypted password
Hi Simon,
On 23.03.2015 21:28, Simon Glass wrote:
> Hi Stefan,
>
> On 13 March 2015 at 01:15, Stefan Roese 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
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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] autoboot.c: Add feature to stop autobooting via SHA256 encrypted password
Hi Stefan,
On 13 March 2015 at 01:15, Stefan Roese 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
>>>
>>>
>>> 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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] autoboot.c: Add feature to stop autobooting via SHA256 encrypted password
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
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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] autoboot.c: Add feature to stop autobooting via SHA256 encrypted password
Hi, On 11 March 2015 at 08: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 > > 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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] autoboot.c: Add feature to stop autobooting via SHA256 encrypted password
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 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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] autoboot.c: Add feature to stop autobooting via SHA256 encrypted password
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 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! -- Tom signature.asc Description: Digital signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH] autoboot.c: Add feature to stop autobooting via SHA256 encrypted password
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
---
common/autoboot.c | 45 -
1 file changed, 44 insertions(+), 1 deletion(-)
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
#include
#include
+#include
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");
--
2.3.2
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
