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

[Simon Glass]

Hi Thirupathaiah,

On Tue, 15 Sep 2020 at 00:18, Thirupathaiah Annapureddy
<thiruan at linux.microsoft.com> 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.

My point here is just that you are declaring variables which end up
being in registers. It doesn't make sense to try to choose a small
type just because it will fit. The machine registers are 32/64-bits.
Using an 8-bit variable can only increase code size as the compiler
may have to mask things off before storing and passing values.

It is fine to use these sorts of types in a data structure to save
memory, or in a protocol to ensure the bits are correct, but it really
doesn't make sense for local variables and parameters.

[..]

> >> +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?

U-Boot uses driver model to deal with the complexity of the system. It
allows the system to be described by devicetree, it allows different
drivers to be created to implement the same functionality. It is more
discoverable than weak functions and less error-prone.

>
> > 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.

OK that sounds good.

[..]

Regards,
Simon