From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thirupathaiah Annapureddy Date: Tue, 15 Sep 2020 12:46:54 -0700 Subject: [RFC PATCH 1/1] image: add anti rollback protection for FIT Images In-Reply-To: <20200915134019.GG5110@bill-the-cat> References: <7bb443952704d347bea760554410b432a3b339b6.1598373235.git.thiruan@linux.microsoft.com> <20200915134019.GG5110@bill-the-cat> Message-ID: <2011da18-49ea-78dd-7979-f58bf40e47ff@linux.microsoft.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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