[U-Boot] [PATCH 3/3] x86: baytrail: secureboot: Add functions for verification of U-Boot

[Anatolij Gustschin]

Hi Bin,

Sorry for long delay, I finally got to prepare v3 series.

On Tue, 16 May 2017 22:40:29 +0800
Bin Meng bmeng.cn at gmail.com wrote:
...
> > +#define SB_MANIFEST_BASE               0xFFFE0000  
> 
> nits: please use lower case hex and fix this globally in this file

OK, done in v3.

...
> > +#define PUB_KEY_MODULUS_SIZE           0x100
> > +#define U_BOOT_STAGE_SIZE              0xDD360  
> 
> What is this?
> 
> > +#define U_BOOT_OFFSET                  0x2CA0  
> 
> and this?

I don't know where these values are comming from, but I think they
are both wrong. A big U-Boot RAM stage part from reset vector remains
unprotected with these. I'll fix U_BOOT_STAGE_SIZE, drop U_BOOT_OFFSET
and will add a comment in v3. Will also adapt the image signing script
as needed.

> > +#define U_BOOT_STAGE_START             (CONFIG_SYS_TEXT_BASE + U_BOOT_OFFSET)
> > +#define U_BOOT_STAGE_END               (U_BOOT_STAGE_START + U_BOOT_STAGE_SIZE)
> > +
> > +#define SHA256_U_BOOT_STAGE_ID         0
> > +#define SHA256_FSP_STAGE2_ID           1
> > +#define SHA256_FIT_PUB_KEY_ID          2
> > +  
> 
> I believe a comment block is needed for explaining things like number
> 0/1/2, at least where are they coming?

These are indexes of 32-byte blocks in the manifest containing SHA256 hashes,
the stage order is fixed. Will add some comments here in v3.

...
> > +       /* compare the two hash  values */  
> 
> nits: two spaces after 'hash'

OK, fixed.

...
> > +/**
> > + * fsp_verify_u_boot_bin() - U-Boot binary verification
> > + *
> > + * This function verifies the integrity for U-Boot, its devicetree and the ucode
> > + * appended or inserted to the devicetree.
> > + *
> > + * @retval:    true on success, false on error
> > + */
> > +bool fsp_verify_u_boot_bin(void);
> > +
> > +/**
> > + * fsp_verify_public_key() - integrity of public key for fit image verification
> > + *
> > + * This function verifies the integrity for the modulus of the public key which
> > + * is stored in the U-Boot devicetree for fit image verification. It tries to  
> 
> nits: device tree

OK, fixed.

> > + * find the "rsa,modulus" property in the dtb and then verifies it with the
> > + * checksum stored in the oem-data block
> > + *
> > + * @retval:    true on success, false on error
> > + */
> > +bool fsp_verify_public_key(void);  
> 
> Again, are these two APIs common for all FSP based platforms?

No, these are used only for Bay Trail secure boot, so the prototypes
are in baytrail specific fsp_configs.h.

...
> > +               /*
> > +                * Verification of the public key happens with verification of
> > +                * the devicetree binary (that's where it's stored), this check  
> 
> nits: device tree

OK, fixed.

> > +                * is not necessary, but nice to see it's integer
> > +                */
> > +               if (!fsp_verify_public_key())
> > +                       puts("Secure Boot: Failed to verify public key for fit-image\n");  
> 
> As the comments say it's not necessary, which means it will never
> fail, right? But in case it fails, that means either hardware is doing
> wrong, or software is doing wrong? Can we adjust the output message to
> indicate some hints?

It can fail if the key is missing in the device tree or if the key
hash in the verified boot manifest doesn't match the hash of the key
in the device tree. I'll slightly rework it here to give more hints.

... 
> BTW: I don't see device tree update for the secure boot enabled board,
> like a device tree that has the "rsa,modulus" stuff.

The device tree will be updated by users, they have to supply their
own keys to the mkimage tool and it will insert the signature node
with custom "rsa,modulus" and other properties.

Thanks,

Anatolij