[U-Boot] [PATCH v2 1/4] Introduce CONFIG_SPL_ABORT_ON_NON_FIT_IMAGE

[Andrew F. Davis]

On 02/10/2017 10:23 AM, Simon Glass wrote:
> Hi Andrew,
> 
> On 8 February 2017 at 08:18, Andrew F. Davis <afd at ti.com> wrote:
>> On 12/06/2016 09:47 PM, Simon Glass wrote:
>>> Hi Andrew,
>>>
>>> On 5 December 2016 at 17:37, Andrew F. Davis <afd at ti.com> wrote:
>>>> On 11/14/2016 06:33 PM, Simon Glass wrote:
>>>>> Hi Andrew,
>>>>>
>>>>> On 14 November 2016 at 15:05, Andrew F. Davis <afd at ti.com> wrote:
>>>>>> On 11/14/2016 02:44 PM, Simon Glass wrote:
>>>>>>> Hi Andrew,
>>>>>>>
>>>>>>> On 14 November 2016 at 12:14, Andrew F. Davis <afd at ti.com> wrote:
>>>>>>>> Introduce CONFIG_SPL_ABORT_ON_NON_FIT_IMAGE. An SPL which define
>>>>>>>> this will abort image loading if the image is not a FIT image.
>>>>>>>>
>>>>>>>> Signed-off-by: Andrew F. Davis <afd at ti.com>
>>>>>>>> ---
>>>>>>>>  Kconfig          | 9 +++++++++
>>>>>>>>  common/spl/spl.c | 5 +++++
>>>>>>>>  2 files changed, 14 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/Kconfig b/Kconfig
>>>>>>>> index 1263d0b..eefebef 100644
>>>>>>>> --- a/Kconfig
>>>>>>>> +++ b/Kconfig
>>>>>>>> @@ -291,6 +291,15 @@ config FIT_IMAGE_POST_PROCESS
>>>>>>>>           injected into the FIT creation (i.e. the blobs would have been pre-
>>>>>>>>           processed before being added to the FIT image).
>>>>>>>>
>>>>>>>> +config SPL_ABORT_ON_NON_FIT_IMAGE
>>>>>>>
>>>>>>> We already have CONFIG_IMAGE_FORMAT_LEGACY so how about
>>>>>>> CONFIG_SPL_IMAGE_FORMAT_LEGACY instead? It can default to y if secure
>>>>>>> boot is disabled.
>>>>>>>
>>>>>>
>>>>>> We also already have CONFIG_SPL_ABORT_ON_RAW_IMAGE on which this is
>>>>>> based. If we only disable legacy image support then RAW images should
>>>>>> still be allowed, but we will fail early anyway, we will start to need
>>>>>> an unmaintainable amount of pre-processor logic to to handle the
>>>>>> different image types and what is allowed/not allowed.
>>>>>>
>>>>>> Even worse some boot modes don't seem to support FIT images (net,
>>>>>> onenand) so these will alway expect legacy to work. Right now we simply
>>>>>> have to disable these modes.
>>>>>
>>>>> IMO CONFIG_SPL_ABORT_ON_RAW_IMAGE should become a positive option, to
>>>>> fit in with the legacy format. Otherwise we'll get very confused I
>>>>> think.
>>>>>
>>>>
>>>> I'm not sure what you are suggesting here, would you like
>>>>
>>>> CONFIG_SPL_SUPPORT_RAW_IMAGE
>>>> CONFIG_SPL_SUPPORT_LEGACY_IMAGE
>>>> CONFIG_SPL_SUPPORT_FIT_IMAGE
>>>>
>>>> And then we disable as needed? I'm not sure this will work in our case,
>>>> as a new image type may be introduced and enabled by default, this will
>>>> break our board security until we discover this and disabled it. The
>>>> benefit of a negative option for us is that we can specify we *only*
>>>> allow FIT, then it will be obvious to someone adding a new image type
>>>> they will not meet this check and should not put code outside this block.
>>>
>>> I don't think we add new image types often, and they should default to
>>> n just as we do for U-Boot proper. Although something odd has happened
>>> with DISABLE_IMAGE_LEGACY.
>>>
>>> his should of thing should be caught in a security review.
>>>
>>> Also you could add something to print a warning if secure boot is
>>> enabled but insecure boot options are available? Perhaps that should
>>> be another option, and default to y?
>>>
>>> It's just that it is really hard to deal with multiple negative
>>> options, so best avoided if we can.
>>>
>>
>> I agree in general, but this time I think this is hard to properly
>> avoid. All that would be different with a positivoption only case
>> would be a bunch of checks that all other image modes are off, then
>> block undefining the same code I am here.
> 
> But why is SPL different from U-Boot proper on this point? It seems
> like we could use the same scheme in SPL as we do in U-Boot proper?
> 
> Positive options are easier to understand and combine. If we end up
> adding another image format it wouldn't be hard to default it to n if
> we are using secure boot.
> 

Right after I send this response I caved and decided to do it your way
in v3: https://www.mail-archive.com/u-boot@lists.denx.de/msg238520.html

Sorry I forgot to express that here.

Thanks,
Andrew

> Regards,
> Simon
>