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

Thirupathaiah Annapureddy
2020-09-15T06:18:25Z

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