From mboxrd@z Thu Jan  1 00:00:00 1970
From: Thirupathaiah Annapureddy <thiruan@linux.microsoft.com>
Date: Mon, 14 Sep 2020 23:18:25 -0700
Subject: [RFC PATCH 1/1] image: add anti rollback protection for FIT Images
In-Reply-To: <CAPnjgZ1jFMTv-5zYxZPs0N=5Y=qrNW9rCD5Z7D7XAg2m_DFXYg@mail.gmail.com>
References: <cover.1598373235.git.thiruan@linux.microsoft.com>
 <7bb443952704d347bea760554410b432a3b339b6.1598373235.git.thiruan@linux.microsoft.com>
 <CAPnjgZ1jFMTv-5zYxZPs0N=5Y=qrNW9rCD5Z7D7XAg2m_DFXYg@mail.gmail.com>
Message-ID: <e7c7e595-d07b-433d-00b9-820596f8bbf0@linux.microsoft.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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