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