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

[Andrew F. Davis]

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.

>>
>>>> +       bool "Disable SPL loading of non-FIT images"
>>>> +       default y if SPL_FIT_SIGNATURE
>>>> +       help
>>>> +         SPL will not load and image if it is not a FIT image. This is
>>>> +         useful for devices that only support authentication/encryption
>>>> +         through SPL FIT loading paths and do not want SPL falling back
>>>> +         to legacy image loading when a non-FIT image is present.
>>>> +
>>>>  config SPL_DFU_SUPPORT
>>>>         bool "Enable SPL with DFU to load binaries to memory device"
>>>>         depends on USB
>>>> diff --git a/common/spl/spl.c b/common/spl/spl.c
>>>> index bdb165a..3d8bee9 100644
>>>> --- a/common/spl/spl.c
>>>> +++ b/common/spl/spl.c
>>>> @@ -93,6 +93,10 @@ void spl_set_header_raw_uboot(struct spl_image_info *spl_image)
>>>>  int spl_parse_image_header(struct spl_image_info *spl_image,
>>>>                            const struct image_header *header)
>>>>  {
>>>> +#ifdef CONFIG_SPL_ABORT_ON_NON_FIT_IMAGE
>>>> +       /* non-FIT image found, proceed to other boot methods. */
>>>> +       return -EINVAL;
>>>
>>> How about -EPROTONOSUPPORT since the request is not really invalid.
>>>
>>>> +#else
>>>>         u32 header_size = sizeof(struct image_header);
>>>>
>>>>         if (image_get_magic(header) == IH_MAGIC) {
>>>> @@ -156,6 +160,7 @@ int spl_parse_image_header(struct spl_image_info *spl_image,
>>>>                 spl_set_header_raw_uboot(spl_image);
>>>>  #endif
>>>>         }
>>>> +#endif
>>>>         return 0;
>>>>  }
>>>>
>>>> --
>>>> 2.10.1
>>>>
> 
> Regards,
> Simon
>