[RFC PATCH 1/1] image: add anti rollback protection for FIT Images

Thirupathaiah Annapureddy
2020-09-15T19:46:54Z

Hi Tom,

Please see my comment(s) in-line.

On 9/15/2020 6:40 AM, Tom Rini wrote:
> On Mon, Sep 14, 2020 at 11:18:25PM -0700, Thirupathaiah Annapureddy wrote:
>> Hi Simon,
>>
>> Thanks for the review.
>>
>> On 9/6/2020 6:43 PM, Simon Glass wrote:
>>>>
>>>> diff --git a/Kconfig b/Kconfig
>>>> index 883e3f71d0..3959a6592c 100644
>>>> --- a/Kconfig
>>>> +++ b/Kconfig
>>>> @@ -533,6 +533,15 @@ config FIT_CIPHER
>>>> Enable the feature of data ciphering/unciphering in the tool mkimage
>>>> and in the u-boot support of the FIT image.
>>>>
>>>> +config FIT_ARBP
>>>
>>> How about using ROLLBACK instead of ARBP. It is easier to understand.Looks good to me. I will change it in the next version of the patch.
>>
>>>> +{
>>>> + uint8_t type;
>>>> + uint32_t image_arbvn;
>>>> + uint32_t plat_arbvn = 0;
>>>
>>> Those three can be uint.
>> fit_image_get_type() returns type as uint8_t.
>> I can change it for the other two variables.
>>
>>>> static int fit_config_verify_sig(const void *fit, int conf_noffset,
>>>> const void *sig_blob, int sig_offset)
>>>> {
>>>> @@ -401,6 +472,14 @@ static int fit_config_verify_sig(const void *fit, int conf_noffset,
>>>> goto error;
>>>> }
>>>>
>>>> +#if !defined(USE_HOSTCC)
>>>
>>> Do we need this ?ifdef, or can we rely on IMAGE_ENABLE_ARBP?
>> I believe we can rely on just IMAGE_ENABLE_ARBP.
>>
>>>> #define FIT_LOAD_PROP "load"
>>>> +#define FIT_ARBVN_PROP "arbvn"
>>>
>>> ROLLBACK / "rollback"
>> I will fix it in the next version.
>>
>>>
>>>>
>>>> /* configuration node */
>>>> #define FIT_KERNEL_PROP "kernel"
>>>> @@ -1085,6 +1086,7 @@ int fit_image_get_data_size_unciphered(const void *fit, int noffset,
>>>> size_t *data_size);
>>>> int fit_image_get_data_and_size(const void *fit, int noffset,
>>>> const void **data, size_t *size);
>>>> +int fit_image_get_arbvn(const void *fit, int noffset, uint32_t *arbvn);
>>>
>>> Please add a full function comment
>> comment was added before the function definition to be consistent
>> with other functions.
>>
>>>> +int board_get_arbvn(uint8_t ih_type, uint32_t *arbvn);
>>>
>>> This needs a driver since the rollback counter may be implemented by a
>>> TPM or anything.
>> Board specific hooks can leverage TPM library functions in that case.
>> May I know why a driver is needed?
>
> Sorry for not getting in to this series sooner. One thing that I think
> would be very helpful is to see is a full demonstration on say a
> Raspberry Pi. I know I have a TPM2 module that supports Pi sitting
> around here. I assume you've also tested this on some HW platform.
>

We test patches on our own hardware. But I agree demonstration of this
feature on more widely available hardware is useful. I will include
board (ex: Raspberry Pi) specific changes in the next version of the patch
series.

Best Regards,
Thiru