From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Tue, 5 May 2015 09:12:26 -0600 Subject: [U-Boot] [PATCH] autoboot.c: Add feature to stop autobooting via SHA256 encrypted password In-Reply-To: <5548DC63.4070606@denx.de> References: <1426063900-7267-1-git-send-email-sr@denx.de> <20150311143646.GL32541@bill-the-cat> <55028E80.1050304@denx.de> <5548DC63.4070606@denx.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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