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

[Thirupathaiah Annapureddy]

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