Re: [PATCH 2/2] efi/capsule: Add support for Quark security header

["Bryan O'Donoghue" <pure.logic@nexus-software.ie>]

On 15/02/17 18:14, Jan Kiszka wrote:
> The firmware for Quark X102x prepends a security header to the capsule
> which is needed to support the mandatory secure boot on this processor.
> The header can be detected by checking for the "_CSH" signature and -
> to avoid any GUID conflict - validating its size field to contain the
> expected value. Then we need to look for the EFI header right after the
> security header and pass the image offset to efi_capsule_update while
> keeping the whole image in RAM - the firmware will look for the header
> on its own.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  drivers/firmware/efi/capsule-loader.c | 73 ++++++++++++++++++++++++++++++-----
>  1 file changed, 63 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c
> index 63ceca9..571d931 100644
> --- a/drivers/firmware/efi/capsule-loader.c
> +++ b/drivers/firmware/efi/capsule-loader.c
> @@ -26,10 +26,32 @@ struct capsule_info {
>  	long		index;
>  	size_t		count;
>  	size_t		total_size;
> +	unsigned int	efi_hdr_offset;
>  	struct page	**pages;
>  	size_t		page_bytes_remain;
>  };
>
> +#define QUARK_CSH_SIGNATURE		0x5f435348	/* _CSH */
> +#define QUARK_SECURITY_HEADER_SIZE	0x400
> +
> +struct efi_quark_security_header {
> +	u32 csh_signature;
> +	u32 version;
> +	u32 modulesize;
> +	u32 security_version_number_index;
> +	u32 security_version_number;
> +	u32 rsvd_module_id;
> +	u32 rsvd_module_vendor;
> +	u32 rsvd_date;
> +	u32 headersize;
> +	u32 hash_algo;
> +	u32 cryp_algo;
> +	u32 keysize;
> +	u32 signaturesize;
> +	u32 rsvd_next_header;
> +	u32 rsvd[2];
> +};

This is a real nitpick (sorry) - but it'd be nice to have a document 
reference or a link to describe this header i.e. it is officially 
documented - outside of the UEFI specification. Make life easy for 
someone reading this header and make an document reference.

Also it'd be appreciated if you could describe the format of the 
structure with

@member	member-attribute description

> +
>  /**
>   * efi_free_all_buff_pages - free all previous allocated buffer pages
>   * @cap_info: pointer to current instance of capsule_info structure
> @@ -56,18 +78,46 @@ static void efi_free_all_buff_pages(struct capsule_info *cap_info)
>  static ssize_t efi_capsule_setup_info(struct capsule_info *cap_info,
>  				      void *kbuff, size_t hdr_bytes)
>  {
> +	struct efi_quark_security_header *quark_hdr;
>  	efi_capsule_header_t *cap_hdr;
>  	size_t pages_needed;
>  	int ret;
>  	void *temp_page;
>
> -	/* Only process data block that is larger than efi header size */
> -	if (hdr_bytes < sizeof(efi_capsule_header_t))
> +	/* Only process data block that is larger than the security header
> +	 * (which is larger than the EFI header) */
> +	if (hdr_bytes < sizeof(struct efi_quark_security_header))
>  		return 0;
>
>  	/* Reset back to the correct offset of header */
>  	cap_hdr = kbuff - cap_info->count;
> -	pages_needed = ALIGN(cap_hdr->imagesize, PAGE_SIZE) >> PAGE_SHIFT;
> +
> +	quark_hdr = (struct efi_quark_security_header *)cap_hdr;
> +
> +	if (quark_hdr->csh_signature == QUARK_CSH_SIGNATURE &&
> +	    quark_hdr->headersize == QUARK_SECURITY_HEADER_SIZE) {
> +		/* Only process data block if EFI header is included */
> +		if (hdr_bytes < QUARK_SECURITY_HEADER_SIZE +
> +				sizeof(efi_capsule_header_t))
> +			return 0;

At this point if cap_info->header_obtained == false then this is an 
error - you should be barfing on this - not literally barfing - at least 
not on your keyboard :)

return -ETHISHEADERSUCKS or some other sensible value -EINVAL like you 
have below.

Point being you've validated the signature, the header size and 
cap_info->header_obtained is false then you definitely have a bogus 
capsule..


> +
> +		pr_debug("%s: Quark security header detected\n", __func__);

... and %s __func__ is verboten don't do it. actually there's a bunch of 
those pairs all over this code - if you have the time in a supplementary 
patch please kill them - there must be a a dev pointer we can get at 
somewhere that makes sense to use dev_dbg- if not those __func__ 
parameters still need to go away - please kill them.

> +
> +		if (quark_hdr->rsvd_next_header != 0) {
> +			pr_err("%s: multiple security headers not supported\n",
> +			       __func__);
> +			return -EINVAL;
> +		}


> +
> +		cap_hdr = (void *)cap_hdr + quark_hdr->headersize;

You could have a separate void * variable and not have the cast.


---
bod