[U-Boot] [PATCH] autoboot.c: Add feature to stop autobooting via SHA256 encrypted password

[Simon Glass]

Hi Stefan,

On 5 May 2015 at 09:06, Stefan Roese <sr at 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 at 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 at 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