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

[Thirupathaiah Annapureddy <thiruan@linux.microsoft.com>]

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?

> If you want to use the board, add a new
> get_rollback() to UCLASS_BOARD (board.h). Or you could create a new
> UCLASS_SECURITY which includes these two API calls.
I explored the option of using UCLASS_BOARD. But it does not have "set"
interfaces and the "id" parameter used in "get" functions seem to be
board specific. We can look into the option of UCLASS_SECURITY for these
two API calls.

> 
> Also please update the vboot test to add a check for rollback.

Yes, will do in the next version of the patch series.

Best Regards,
Thiru