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

[Simon Glass]

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.

Regards,
Simon