[U-Boot] [PATCH] vboot: Add FIT_SIGNATURE_MAX_SIZE protection

[Simon Glass]

Hi Teddy,

On 7 June 2018 at 13:57, Teddy Reed <teddy.reed at gmail.com> wrote:
> On Thu, Jun 7, 2018 at 4:25 PM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Teddy,
>>
>> On 3 June 2018 at 15:28, Teddy Reed <teddy.reed at gmail.com> wrote:
>>> This adds a new config value FIT_SIGNATURE_MAX_SIZE, which controls the
>>> max size of a FIT header's totalsize field. The max size is checked before
>>> signature checks are applied to prevent reading past defined FIT regions.
>>>
>>> This field is not part of the vboot signature so it should be sanity
>>> checked. If the field is corrupted then the structure or string region
>>> reads may have unintended behavior, such as reading from device memory.
>>> A default value of 256MB is set and intended to support most max storage
>>> sizes.
>>>
>>> Suggested-by: Simon Glass <sjg at chromium.org>
>>> Signed-off-by: Teddy Reed <teddy.reed at gmail.com>
>>> ---
>>>  Kconfig                     | 10 ++++++++++
>>>  common/image-sig.c          |  7 +++++++
>>>  test/py/tests/test_vboot.py | 31 +++++++++++++++++++++++++++++++
>>>  3 files changed, 48 insertions(+)
>>
>> Reviewed-by: Simon Glass <sjg at chromium.org>
>>
>> Please see below.
>>
>>>
>>> diff --git a/Kconfig b/Kconfig
>>> index 5a82c95..c8b86cd 100644
>>> --- a/Kconfig
>>> +++ b/Kconfig
>>> @@ -267,6 +267,16 @@ config FIT_SIGNATURE
>>>           format support in this case, enable it using
>>>           CONFIG_IMAGE_FORMAT_LEGACY.
>>>
>>> +config FIT_SIGNATURE_MAX_SIZE
>>> +       hex "Max size of signed FIT structures"
>>> +       depends on FIT_SIGNATURE
>>> +       default 0x10000000
>>> +       help
>>> +         This option sets a max size in bytes for verified FIT uImages.
>>> +         A sane value of 256MB protects corrupted DTB structures from overlapping
>>> +         device memory. Assure this size does not extend past expected storage
>>> +         space.
>>> +
>>>  config FIT_VERBOSE
>>>         bool "Show verbose messages when FIT images fail"
>>>         help
>>> diff --git a/common/image-sig.c b/common/image-sig.c
>>> index f65d883..e67d2a2 100644
>>> --- a/common/image-sig.c
>>> +++ b/common/image-sig.c
>>> @@ -156,6 +156,13 @@ static int fit_image_setup_verify(struct image_sign_info *info,
>>>  {
>>>         char *algo_name;
>>>
>>> +#ifdef CONFIG_FIT_SIGNATURE_MAX_SIZE
>>
>> Is there a case where this CONFIG item is not present? If not, can we
>> stop the #ifdef?
>
> I will investigate, perhaps when building host tools?

I was hoping that this file would not be built unless that is defined,
since it only depends on FIT_SIGNATURE.

[..]

Regards,
Simon