[v4,16/18] secvar/backend: add edk2 derived key updates processing

From: Nayna Jain <nayna@linux.ibm.com>

As part of secureboot key management, the scheme for handling key updates
is derived from tianocore reference implementation[1]. The wrappers for
holding the signed update is the Authentication Header and for holding
the public key certificate is ESL (EFI Signature List), both derived from
tianocore reference implementation[1].

This patch adds the support to process update queue. This involves:
1. Verification of the update signature using the key authorized as per the
key hierarchy
2. Handling addition/deletion of the keys
3. Support for dbx (blacklisting of hashes)
4. Validation checks for the updates
5. Supporting multiple ESLs for single variable both for update/verification
6. Timestamp check
7. Allowing only single PK
8. Failure Handling
9. Resetting keystore if the hardware key hash changes

[1] https://github.com/tianocore/edk2-staging.git

Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
Signed-off-by: Eric Richter <erichte@linux.ibm.com>
---
V4:
 - fixed a typo in the hw-key-hash mismatch prlog
 - replace PRIORITY with PROTECTED
 - adjust most lines to 80 columns, some exceptions and prlog statements
     may still run over

 doc/secvar/edk2.rst                         |  49 ++
 include/secvar.h                            |   1 +
 libstb/secvar/backend/Makefile.inc          |   4 +-
 libstb/secvar/backend/edk2-compat-process.c | 717 ++++++++++++++++++++
 libstb/secvar/backend/edk2-compat-process.h |  61 ++
 libstb/secvar/backend/edk2-compat-reset.c   | 115 ++++
 libstb/secvar/backend/edk2-compat-reset.h   |  24 +
 libstb/secvar/backend/edk2-compat.c         | 262 +++++++
 libstb/secvar/backend/edk2.h                | 243 +++++++
 9 files changed, 1474 insertions(+), 2 deletions(-)
 create mode 100644 doc/secvar/edk2.rst
 create mode 100644 libstb/secvar/backend/edk2-compat-process.c
 create mode 100644 libstb/secvar/backend/edk2-compat-process.h
 create mode 100644 libstb/secvar/backend/edk2-compat-reset.c
 create mode 100644 libstb/secvar/backend/edk2-compat-reset.h
 create mode 100644 libstb/secvar/backend/edk2-compat.c
 create mode 100644 libstb/secvar/backend/edk2.h

On Mon, 2020-05-11 at 16:31 -0500, Eric Richter wrote:
> From: Nayna Jain <nayna@linux.ibm.com>
> 
> As part of secureboot key management, the scheme for handling key updates
> is derived from tianocore reference implementation[1]. The wrappers for
> holding the signed update is the Authentication Header and for holding
> the public key certificate is ESL (EFI Signature List), both derived from
> tianocore reference implementation[1].
> 
> This patch adds the support to process update queue. This involves:
> 1. Verification of the update signature using the key authorized as per the
> key hierarchy
> 2. Handling addition/deletion of the keys
> 3. Support for dbx (blacklisting of hashes)
> 4. Validation checks for the updates
> 5. Supporting multiple ESLs for single variable both for update/verification
> 6. Timestamp check
> 7. Allowing only single PK
> 8. Failure Handling
> 9. Resetting keystore if the hardware key hash changes
> 
> [1] https://github.com/tianocore/edk2-staging.git
> 
> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
> Signed-off-by: Eric Richter <erichte@linux.ibm.com>
> ---
> V4:
>  - fixed a typo in the hw-key-hash mismatch prlog
>  - replace PRIORITY with PROTECTED
>  - adjust most lines to 80 columns, some exceptions and prlog statements
>      may still run over
> 
>  doc/secvar/edk2.rst                         |  49 ++
>  include/secvar.h                            |   1 +
>  libstb/secvar/backend/Makefile.inc          |   4 +-
>  libstb/secvar/backend/edk2-compat-process.c | 717 ++++++++++++++++++++
>  libstb/secvar/backend/edk2-compat-process.h |  61 ++
>  libstb/secvar/backend/edk2-compat-reset.c   | 115 ++++
>  libstb/secvar/backend/edk2-compat-reset.h   |  24 +
>  libstb/secvar/backend/edk2-compat.c         | 262 +++++++
>  libstb/secvar/backend/edk2.h                | 243 +++++++
>  9 files changed, 1474 insertions(+), 2 deletions(-)
>  create mode 100644 doc/secvar/edk2.rst
>  create mode 100644 libstb/secvar/backend/edk2-compat-process.c
>  create mode 100644 libstb/secvar/backend/edk2-compat-process.h
>  create mode 100644 libstb/secvar/backend/edk2-compat-reset.c
>  create mode 100644 libstb/secvar/backend/edk2-compat-reset.h
>  create mode 100644 libstb/secvar/backend/edk2-compat.c
>  create mode 100644 libstb/secvar/backend/edk2.h
> 
> diff --git a/doc/secvar/edk2.rst b/doc/secvar/edk2.rst
> new file mode 100644
> index 00000000..1e4cc9e3
> --- /dev/null
> +++ b/doc/secvar/edk2.rst
> @@ -0,0 +1,49 @@
> +.. _secvar/edk2:
> +
> +Skiboot edk2-compatible Secure Variable Backend
> +===============================================
> +
> +Overview
> +--------
> +
> +The edk2 secure variable backend for skiboot borrows from edk2 concepts
> +such as the three key hierarchy (PK, KEK, and db), and a similar
> +structure. In general, variable updates must be signed with a key
> +of a higher level. So, updates to the db must be signed with a key stored
> +in the KEK; updates to the KEK must be signed with the PK. Updates to the
> +PK must be signed with the previous PK (if any).
> +
> +Variables are stored in the efi signature list format, and updates are a
> +signed variant that includes an authentication header.
> +
> +If no PK is currently enrolled, the system is considered to be in "Setup
> +Mode". Any key can be enrolled without signature checks. However, once a
> +PK is enrolled, the system switches to "User Mode", and each update must
> +now be signed according to the hierarchy. Furthermore, when in "User
> +Mode", the backend initialized the ``os-secure-mode`` device tree flag,
> +signaling to the kernel that we are in secure mode.
> +
> +Updates are processed sequentially, in the order that they were provided
> +in the update queue. If any update fails to validate, appears to be
> +malformed, or any other error occurs, NO updates will not be applied.
> +This includes updates that may have successfully applied prior to the
> +error. The system will continue in an error state, reporting the error
> +reason via the ``update-status`` device tree property.
> +
> +P9 Special Case for the Platform Key
> +------------------------------------
> +
> +Due to the powerful nature of the platform key and the lack of lockable
> +flash, the edk2 backend will store the PK in TPM NV rather than PNOR on
> +P9 systems. (TODO expand on this)
> +
> +Update Status Return Codes
> +--------------------------
> +
> +TODO, edk2 driver needs to actually return these properly first
> +
> +
> +Device Tree Bindings
> +--------------------
> +
> +TODO
> diff --git a/include/secvar.h b/include/secvar.h
> index 7a45db2b..4f40c115 100644
> --- a/include/secvar.h
> +++ b/include/secvar.h
> @@ -28,6 +28,7 @@ struct secvar_backend_driver {
>  };
>  
>  extern struct secvar_storage_driver secboot_tpm_driver;
> +extern struct secvar_backend_driver edk2_compatible_v1;
>  
>  int secvar_main(struct secvar_storage_driver, struct secvar_backend_driver);
>  
> diff --git a/libstb/secvar/backend/Makefile.inc b/libstb/secvar/backend/Makefile.inc
> index 6f491a63..bc987f69 100644
> --- a/libstb/secvar/backend/Makefile.inc
> +++ b/libstb/secvar/backend/Makefile.inc
> @@ -1,11 +1,11 @@
>  # SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later
>  # -*-Makefile-*-
>  
> -SECVAR_BACKEND_DIR = libstb/secvar/backend
> +SECVAR_BACKEND_DIR = $(SRC)/libstb/secvar/backend
>  
>  SUBDIRS += $(SECVAR_BACKEND_DIR)
>  
> -SECVAR_BACKEND_SRCS =
> +SECVAR_BACKEND_SRCS = edk2-compat.c edk2-compat-process.c edk2-compat-reset.c
>  SECVAR_BACKEND_OBJS = $(SECVAR_BACKEND_SRCS:%.c=%.o)
>  SECVAR_BACKEND = $(SECVAR_BACKEND_DIR)/built-in.a
>  
> diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c
> new file mode 100644
> index 00000000..60ebb0b2
> --- /dev/null
> +++ b/libstb/secvar/backend/edk2-compat-process.c
> @@ -0,0 +1,717 @@
> +// SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later
> +/* Copyright 2020 IBM Corp. */
> +#ifndef pr_fmt
> +#define pr_fmt(fmt) "EDK2_COMPAT: " fmt
> +#endif
> +
> +#include <opal.h>
> +#include <string.h>
> +#include <time.h>
> +#include <unistd.h>
> +#include <stdint.h>
> +#include <ccan/endian/endian.h>
> +#include <mbedtls/error.h>
> +#include <device.h>
> +#include "libstb/crypto/pkcs7/pkcs7.h"
> +#include "edk2.h"
> +#include "../secvar.h"
> +#include "edk2-compat-process.h"
> +
> +bool setup_mode;
> +
> +int update_variable_in_bank(struct secvar *secvar, const char *data,
> +			    uint64_t dsize, struct list_head *bank)
> +{
> +	struct secvar_node *node;
> +
> +	node = find_secvar(secvar->key, secvar->key_len, bank);
> +	if (!node)
> +		return OPAL_EMPTY;
> +
> +        /* Reallocate the data memory, if there is change in data size */
> +	if (node->size < dsize)
> +		if (realloc_secvar(node, dsize))
> +			return OPAL_NO_MEM;
> +
> +	if (dsize && data)
> +		memcpy(node->var->data, data, dsize);
> +	node->var->data_size = dsize;
> +
> +        /* Clear the volatile bit only if updated with positive data size */
> +	if (dsize)
> +		node->flags &= ~SECVAR_FLAG_VOLATILE;
> +	else
> +		node->flags |= SECVAR_FLAG_VOLATILE;
> +
> +        /* Is it required to be set everytime ? */
you tell me

> +	if ((!strncmp(secvar->key, "PK", 3))
> +	     || (!strncmp(secvar->key, "HWKH", 5)))
> +		node->flags |= SECVAR_FLAG_PROTECTED;
> +
> +	return 0;
> +}
> +

> +/* Converts utf8 string to ucs2 */
> +static char *utf8_to_ucs2(const char *key, size_t keylen)
> +{
> +	int i;
> +	char *str;
> +
> +	str = zalloc(keylen * 2);
> +	if (!str)
> +		return NULL;
> +
> +	for (i = 0; i < keylen*2; key++) {
> +		str[i++] = *key;
> +		str[i++] = '\0';
> +	}
> +
> +	return str;
> +}

This is completely broken for anything that doesn't fall into the ASCII
subset of UTF-8.

> +/* Returns the authority that can sign the given key update */
> +static void get_key_authority(const char *ret[3], const char *key)
> +{
> +	int i = 0;
> +
> +	if (key_equals(key, "PK")) {
> +		ret[i++] = "PK";
> +	} else if (key_equals(key, "KEK")) {
> +		ret[i++] = "PK";
> +	} else if (key_equals(key, "db") || key_equals(key, "dbx")) {
> +		ret[i++] = "KEK";
> +		ret[i++] = "PK";
> +	}
> +
> +	ret[i] = NULL;
> +}
> +
> +/* Returns the size of the complete ESL. */
> +static int get_esl_signature_list_size(char *buf, size_t buflen)
buf should be const
> +{
> +	EFI_SIGNATURE_LIST list;
> +
> +	if (buflen < sizeof(EFI_SIGNATURE_LIST))
> +		return OPAL_PARAMETER;
> +
> +	memcpy(&list, buf, sizeof(EFI_SIGNATURE_LIST));
> +
> +	prlog(PR_DEBUG, "size of signature list size is %u\n",
> +			le32_to_cpu(list.SignatureListSize));
> +
> +	return le32_to_cpu(list.SignatureListSize);
> +}
> +
> +/* Copies the certificate from the ESL into cert buffer and returns the size
> + * of the certificate
> + */
> +static int get_esl_cert(char *buf, size_t buflen, char **cert)
buf should be const
> +{
> +	size_t sig_data_offset;
> +	size_t size;
> +	EFI_SIGNATURE_LIST list;
> +
> +	if (buflen < sizeof(EFI_SIGNATURE_LIST))
> +		return OPAL_PARAMETER;
> +
> +	memcpy(&list, buf, sizeof(EFI_SIGNATURE_LIST));

This looks familiar

> +
> +	size = le32_to_cpu(list.SignatureSize) - sizeof(uuid_t);
> +	/* No certificate in the ESL */
> +	if (size <= 0)
> +		return OPAL_PERMISSION;

size_t is unsigned so checking for less than zero doesn't do anything.
If list.SignatureSize is too small the calculation will underflow and
probably lead to a memory allocation failure later on.

> +	if (!cert)
> +		return OPAL_PARAMETER;
Passing a null pointer here is a programming error, use an assert.

> +	*cert = zalloc(size);
> +	if (!(*cert))
> +		return OPAL_NO_MEM;
> +
> +	prlog(PR_DEBUG,"size of signature list size is %u\n",
> +			le32_to_cpu(list.SignatureListSize));
> +	prlog(PR_DEBUG, "size of signature header size is %u\n",
> +			le32_to_cpu(list.SignatureHeaderSize));
> +	prlog(PR_DEBUG, "size of signature size is %u\n",
> +			le32_to_cpu(list.SignatureSize));
> +
> +	sig_data_offset = sizeof(list) + le32_to_cpu(list.SignatureHeaderSize)
> +		+ 16 * sizeof(uint8_t);
> +	if (sig_data_offset > buflen) {
> +		free(*cert);
> +		return OPAL_PARAMETER;
> +	}
The free() here is only required because you're performing the
allocation before verifying that the input buffer is long enough.
Reverse the order, it's less code and less error prone since you don't
have to unwind the allocation.

> +	memcpy(*cert, buf + sig_data_offset, size);
> +
> +	return size;
> +}
> +
> +/* Extracts size of the PKCS7 signed data embedded in the
> + * struct Authentication 2 Descriptor Header.
> + */
> +static int get_pkcs7_len(struct efi_variable_authentication_2 *auth)
auth should be const

> +{
> +	uint32_t dw_length;
> +	size_t size;
> +
> +	if (!auth)
> +		return OPAL_PARAMETER;
assert

> +
> +	dw_length = le32_to_cpu(auth->auth_info.hdr.dw_length);
> +	size = dw_length - (sizeof(auth->auth_info.hdr.dw_length)
> +			+ sizeof(auth->auth_info.hdr.w_revision)
> +			+ sizeof(auth->auth_info.hdr.w_certificate_type)
> +			+ sizeof(auth->auth_info.cert_type));
> +
> +	return size;
> +}
> +
> +int get_auth_descriptor2(void *buf, size_t buflen, char **auth_buffer)
buf should be const

> +{
> +	struct efi_variable_authentication_2 *auth = NULL;
> +	size_t auth_buffer_size;
> +	int len;
> +
> +	if (buflen < sizeof(struct efi_variable_authentication_2))
> +			return OPAL_PARAMETER;
> +
> +	auth = buf;
You can initalise auth to buf in the declaration above.

> +
> +	len = get_pkcs7_len(auth);
> +
> +	/* We need PKCS7 data else there is no signature */
> +	if (len <= 0)
> +		return OPAL_PARAMETER;
> +
> +	if (!auth_buffer)
> +		return OPAL_PARAMETER;

should be an assert. Also, do your parameter validation before you
start doing any actual work.

> +
> +	auth_buffer_size = sizeof(auth->timestamp) + sizeof(auth->auth_info.hdr)
> +			   + sizeof(auth->auth_info.cert_type) + len;
> +
> +	if (auth_buffer_size <= 0)
> +		return OPAL_PARAMETER;
> +
> +	*auth_buffer = zalloc(auth_buffer_size);
> +	if (!(*auth_buffer))
> +		return OPAL_NO_MEM;
> +
> +	memcpy(*auth_buffer, buf, auth_buffer_size);
why are we making a copy of this?

> +
> +	return auth_buffer_size;
> +}
> +
> +int validate_esl_list(char *key, char *esl, size_t size)
> +{
> +	int count = 0;
> +	int signing_cert_size;
> +	char *signing_cert = NULL;
> +	mbedtls_x509_crt x509;
> +	char *x509_buf = NULL;
> +	int eslvarsize = size;
> +	int rc = OPAL_SUCCESS;
> +	int eslsize;
> +	int offset = 0;
> +
> +	while (eslvarsize > 0) {
> +		prlog(PR_DEBUG, "esl var size size is %d offset is %d\n", eslvarsize, offset);
> +		if (eslvarsize < sizeof(EFI_SIGNATURE_LIST))
> +			break;
> +
> +		/* Calculate the size of the ESL */
> +		eslsize = get_esl_signature_list_size(esl, eslvarsize);
> +		/* If could not extract the size */
> +		if (eslsize <= 0) {
> +			prlog(PR_ERR, "Invalid size of the ESL\n");
> +			rc = OPAL_PARAMETER;
> +			break;
> +		}
> +
> +		/* Extract the certificate from the ESL */
> +		signing_cert_size = get_esl_cert(esl,
> +						 eslvarsize,
> +						 &signing_cert);
> +		if (signing_cert_size < 0) {
> +			rc = signing_cert_size;
> +			break;
> +		}
> +
> +		mbedtls_x509_crt_init(&x509);
> +		rc = mbedtls_x509_crt_parse(&x509,
> +					    signing_cert,
> +					    signing_cert_size);
> +
> +		/* If failure in parsing the certificate, exit */
> +		if(rc) {
> +			prlog(PR_INFO, "X509 certificate parsing failed %04x\n", rc);
> +			rc = OPAL_PARAMETER;
> +			break;
> +		}
> +
> +		x509_buf = zalloc(CERT_BUFFER_SIZE);
> +		rc = mbedtls_x509_crt_info(x509_buf,
> +					   CERT_BUFFER_SIZE,
> +					   "CRT:",
> +					   &x509);
> +		prlog(PR_INFO, "%s ", x509_buf);
> +
> +		/* If failure in reading the certificate, exit */
> +		if (rc < 0) {
> +			prlog(PR_INFO, "Failed to show X509 certificate info %04x\n", rc);
> +			rc = OPAL_PARAMETER;
> +			free(x509_buf);
> +			break;
> +		}
> +		rc = 0;
> +
> +		free(x509_buf);
> +		x509_buf = NULL;
> +		count++;
> +
> +		/* Look for the next ESL */
> +		offset = offset + eslsize;
> +		eslvarsize = eslvarsize - eslsize;
> +		mbedtls_x509_crt_free(&x509);
> +		free(signing_cert);
> +		/* Since we are going to allocate again in the next iteration */
> +		signing_cert = NULL;
> +	}
> +
> +	if (rc == OPAL_SUCCESS) {
> +		if (key_equals(key, "PK") && (count > 1)) {
> +			prlog(PR_ERR, "PK can only be one\n");
> +			rc = OPAL_PARAMETER;
> +		} else {
> +			rc = count;
> +		}
> +	}
> +
> +	prlog(PR_INFO, "Total ESLs are %d\n", rc);
> +	return rc;
> +}
> +

> +/* Get the timestamp for the last update of the give key */
> +static struct efi_time *get_last_timestamp(const char *key, char *last_timestamp)
> +{
> +	u8 off;
> +
> +	if (!last_timestamp)
> +		return NULL;
> +
> +	if (!strncmp(key, "PK", 3))
> +		off = 0;
> +	else if (!strncmp(key, "KEK", 4))
> +		off = 1;
> +	else if (!strncmp(key, "db", 3))
> +		off = 2;
> +	else if (!strncmp(key, "dbx", 4))
> +		off = 3;
> +	else
> +		return NULL;
> +
> +	return &((struct efi_time *)last_timestamp)[off];
> +}

I don't know what you're doing here, but I don't like it.


> +int update_timestamp(char *key, struct efi_time *timestamp, char *last_timestamp)
> +{
> +	struct efi_time *prev;
> +
> +	prev = get_last_timestamp(key, last_timestamp);
> +	if (prev == NULL)
> +		return OPAL_INTERNAL_ERROR;
> +
> +	memcpy(prev, timestamp, sizeof(struct efi_time));
> +
> +	prlog(PR_DEBUG, "updated prev year is %d month %d day %d\n",
> +			le16_to_cpu(prev->year), prev->month, prev->day);
> +
> +	return OPAL_SUCCESS;
> +}
> +
> +int check_timestamp(char *key, struct efi_time *timestamp,
> +		    char *last_timestamp)
> +{
> +	struct efi_time *prev;
> +
> +	prev = get_last_timestamp(key, last_timestamp);
> +	if (prev == NULL)
> +		return OPAL_INTERNAL_ERROR;
> +
> +	prlog(PR_DEBUG, "timestamp year is %d month %d day %d\n",
> +			le16_to_cpu(timestamp->year), timestamp->month,
> +			timestamp->day);
> +	prlog(PR_DEBUG, "prev year is %d month %d day %d\n",
> +			le16_to_cpu(prev->year), prev->month, prev->day);
> +	if (le16_to_cpu(timestamp->year) > le16_to_cpu(prev->year))
> +		return OPAL_SUCCESS;
> +	if (le16_to_cpu(timestamp->year) < le16_to_cpu(prev->year))
> +		return OPAL_PERMISSION;
> +
> +	if (timestamp->month > prev->month)
> +		return OPAL_SUCCESS;
> +	if (timestamp->month < prev->month)
> +		return OPAL_PERMISSION;
> +
> +	if (timestamp->day > prev->day)
> +		return OPAL_SUCCESS;
> +	if (timestamp->day < prev->day)
> +		return OPAL_PERMISSION;
> +
> +	if (timestamp->hour > prev->hour)
> +		return OPAL_SUCCESS;
> +	if (timestamp->hour < prev->hour)
> +		return OPAL_PERMISSION;
> +
> +	if (timestamp->minute > prev->minute)
> +		return OPAL_SUCCESS;
> +	if (timestamp->minute < prev->minute)
> +		return OPAL_PERMISSION;
> +
> +	if (timestamp->second > prev->second)
> +		return OPAL_SUCCESS;
> +
> +	/* Time less than or equal to is considered as replay*/
> +	if (timestamp->second <= prev->second)
> +		return OPAL_PERMISSION;

You could simplify this a lot by unpacking the timestamp bytes into a
uint64 with the following format: 0xYYYYMMDDHHMMSS, then the two can be
compared with one numeric test.

> +	/* nanosecond, timezone, daylight and pad2 are meant to be zero */

I'd verify that, but that's just me.

> +
> +	return OPAL_SUCCESS;
> +}
> +

> +/* Extract PKCS7 from the authentication header */
> +static int get_pkcs7(struct efi_variable_authentication_2 *auth,
> +		     mbedtls_pkcs7 **pkcs7)
auth should be const

> +{
> +	char *checkpkcs7cert = NULL;
> +	int len;
> +	int rc;
> +
> +	len = get_pkcs7_len(auth);
> +	if (len <= 0)
> +		return OPAL_PARAMETER;
> +
> +	if (!pkcs7)
> +		return OPAL_PARAMETER;
> +
> +	*pkcs7 = malloc(sizeof(struct mbedtls_pkcs7));
> +	if (!(*pkcs7))
> +		return OPAL_NO_MEM;
> +
> +	mbedtls_pkcs7_init(*pkcs7);
> +	rc = mbedtls_pkcs7_parse_der(
> +			(const unsigned char *)auth->auth_info.cert_data,
> +			(const unsigned int)len, *pkcs7);
Why are those casts necessary?

> +	if (rc) {
> +		prlog(PR_ERR, "Parsing pkcs7 failed %04x\n", rc);
> +		mbedtls_pkcs7_free(*pkcs7);
> +		return rc;
> +	}
> +
> +	checkpkcs7cert = zalloc(CERT_BUFFER_SIZE);
> +	if (!checkpkcs7cert) {
> +		mbedtls_pkcs7_free(*pkcs7);
> +		return OPAL_NO_MEM;
> +	}
> +
> +	rc = mbedtls_x509_crt_info(checkpkcs7cert, CERT_BUFFER_SIZE, "CRT:",
> +			&((*pkcs7)->signed_data.certs));
> +	if (rc < 0) {
> +		prlog(PR_ERR, "Failed to parse the certificate in PKCS7 structure\n");
> +		rc = OPAL_PARAMETER;
> +	} else {
> +		rc = OPAL_SUCCESS;
> +		prlog(PR_DEBUG, "%s \n", checkpkcs7cert);
> +	}
> +
> +	free(checkpkcs7cert);
> +	mbedtls_pkcs7_free(*pkcs7);
> +
> +	return rc;
> +}
> +
> +/* Verify the PKCS7 signature on the signed data. */
> +static int verify_signature(struct efi_variable_authentication_2 *auth,
> +			    char *newcert, size_t new_data_size,
> +			    struct secvar *avar)
const where necessary

> +{
> +	mbedtls_pkcs7 *pkcs7 = NULL;
> +	mbedtls_x509_crt x509;
> +	char *signing_cert = NULL;
> +	char *x509_buf = NULL;
> +	int signing_cert_size;
> +	int rc;
> +	char *errbuf;
> +	int eslvarsize;
> +	int eslsize;
> +	int offset = 0;
> +
> +	if (!auth)
> +		return OPAL_PARAMETER;
> +
> +	/* Extract the pkcs7 from the auth structure */
> +	rc  = get_pkcs7(auth, &pkcs7);
> +	/* Failure to parse pkcs7 implies bad input. */
> +	if (rc != OPAL_SUCCESS)
> +		return OPAL_PARAMETER;
> +
> +	prlog(PR_INFO, "Load the signing certificate from the keystore");
> +
> +	eslvarsize = avar->data_size;
> +
> +	/* Variable is not empty */
> +	while (eslvarsize > 0) {
> +		prlog(PR_DEBUG, "esl var size size is %d offset is %d\n", eslvarsize, offset);
> +		if (eslvarsize < sizeof(EFI_SIGNATURE_LIST))
> +			break;
> +
> +		/* Calculate the size of the ESL */
> +		eslsize = get_esl_signature_list_size(avar->data + offset,
> +						      eslvarsize);
> +		/* If could not extract the size */
> +		if (eslsize <= 0) {
> +			rc = OPAL_PARAMETER;
> +			break;
> +		}
> +
> +		/* Extract the certificate from the ESL */
> +		signing_cert_size = get_esl_cert(avar->data + offset,
> +						 eslvarsize, &signing_cert);
> +		if (signing_cert_size < 0) {
> +			rc = signing_cert_size;
> +			break;
> +		}
> +
> +		mbedtls_x509_crt_init(&x509);
> +		rc = mbedtls_x509_crt_parse(&x509,
> +					    signing_cert,
> +					    signing_cert_size);
> +
> +		/* This should not happen, unless something corrupted in PNOR */
> +		if(rc) {
> +			prlog(PR_INFO, "X509 certificate parsing failed %04x\n", rc);
> +			rc = OPAL_INTERNAL_ERROR;
> +			break;
> +		}
> +
> +		x509_buf = zalloc(CERT_BUFFER_SIZE);
> +		rc = mbedtls_x509_crt_info(x509_buf,
> +					   CERT_BUFFER_SIZE,
> +					   "CRT:",
> +					   &x509);
> +
> +		/* This should not happen, unless something corrupted in PNOR */
> +		if (rc < 0) {
> +			free(x509_buf);
> +			rc = OPAL_INTERNAL_ERROR;
> +			break;
> +		}
> +
> +		prlog(PR_INFO, "%s \n", x509_buf);
> +		free(x509_buf);
> +		x509_buf = NULL;
> +
> +		/* Verify the signature */
> +		rc = mbedtls_pkcs7_signed_data_verify(pkcs7, &x509, newcert,
> +						      new_data_size);
> +
> +		/* If you find a signing certificate, you are done */
> +		if (rc == 0) {
> +			prlog(PR_INFO, "Signature Verification passed\n");
> +			mbedtls_x509_crt_free(&x509);
> +			break;
> +		}
> +
> +		errbuf = zalloc(MBEDTLS_ERR_BUFFER_SIZE);
> +		mbedtls_strerror(rc, errbuf, MBEDTLS_ERR_BUFFER_SIZE);
> +		prlog(PR_INFO, "Signature Verification failed %02x %s\n",
> +				rc, errbuf);
> +		free(errbuf);
> +
> +		/* Look for the next ESL */
> +		offset = offset + eslsize;
> +		eslvarsize = eslvarsize - eslsize;
> +		mbedtls_x509_crt_free(&x509);
> +		free(signing_cert);
> +		/* Since we are going to allocate again in the next iteration */
> +		signing_cert = NULL;
> +
> +	}
> +
> +	free(signing_cert);
> +	mbedtls_pkcs7_free(pkcs7);
> +	free(pkcs7);
> +
> +	return rc;
> +}
> +
> +/* Create the single buffer
> + * name || vendor guid || attributes || timestamp || newcontent
> + * which is submitted as signed by the user.
> + * Returns number of bytes in the new buffer, else negative error
> + * code.
> + */

I'd be happier if you incrementally computed the hash on each field
rather than copying everything constantly.

> +static int get_data_to_verify(char *key, char *new_data, size_t new_data_size,
> +			      char **buffer, size_t *buffer_size,
> +			      struct efi_time *timestamp)
consts
> +{
> +	le32 attr = cpu_to_le32(SECVAR_ATTRIBUTES);
> +	size_t offset = 0;
> +	size_t varlen;
> +	char *wkey;
> +	uuid_t guid;
> +
> +	if (key_equals(key, "PK")
> +	    || key_equals(key, "KEK"))
> +		guid = EFI_GLOBAL_VARIABLE_GUID;
> +	else if (key_equals(key, "db")
> +	    || key_equals(key, "dbx"))
> +		guid = EFI_IMAGE_SECURITY_DATABASE_GUID;
> +	else
> +		return OPAL_INTERNAL_ERROR;
> +
> +	/* Convert utf8 name to ucs2 width */
> +	varlen = strlen(key) * 2;
> +	wkey = utf8_to_ucs2(key, strlen(key));
> +
> +	/* Prepare the single buffer */
> +	*buffer_size = varlen + UUID_SIZE + sizeof(attr)
> +		       + sizeof(struct efi_time) + new_data_size;
> +	*buffer = zalloc(*buffer_size);
> +	if (!*buffer)
> +		return OPAL_NO_MEM;
> +
> +	memcpy(*buffer + offset, wkey, varlen);
> +	offset = offset + varlen;
> +	memcpy(*buffer + offset, &guid, sizeof(guid));
> +	offset = offset + sizeof(guid);
> +	memcpy(*buffer + offset, &attr, sizeof(attr));
> +	offset = offset + sizeof(attr);
> +	memcpy(*buffer + offset, timestamp , sizeof(struct efi_time));
> +	offset = offset + sizeof(struct efi_time);
> +
> +	memcpy(*buffer + offset, new_data, new_data_size);
> +	offset = offset + new_data_size;
> +
> +	free(wkey);
> +
> +	return offset;
> +}
> +

> +bool is_pkcs7_sig_format(void *data)
const
> +{
> +	struct efi_variable_authentication_2 *auth = data;
> +	uuid_t pkcs7_guid = EFI_CERT_TYPE_PKCS7_GUID;
> +
> +	if(!(memcmp(&auth->auth_info.cert_type, &pkcs7_guid, 16) == 0))
> +		return false;
> +	return true;

That's one way to do it. Alternatively:

	if (memcmp(&auth->auth_info.cert_type, &pkcs7_guid, 16) != 0)
		return false;
	return true;

Or:
	if (memcmp(&auth->auth_info.cert_type, &pkcs7_guid, 16))
		return false;
	return true;

Or even:

	return !memcmp(&auth->auth_info.cert_type, &pkcs7_guid, 16);


> +int process_update(struct secvar_node *update, char **newesl,
> +		   int *new_data_size, struct efi_time *timestamp,
> +		   struct list_head *bank, char *last_timestamp)
> +{
> +	struct efi_variable_authentication_2 *auth = NULL;
> +	char *auth_buffer = NULL;

If you want a pointer to an arbitrary buffer use a void pointer rather
than a char. Voids will automaticly coerce to another pointer type so
you don't have to manually cast it. Also, this should be const.

> +	int auth_buffer_size = 0;
> +	const char *key_authority[3];
> +	char *tbhbuffer = NULL;
> +	size_t tbhbuffersize = 0;
> +	struct secvar_node *anode = NULL;
> +	int rc = 0;
> +	int i;
> +
> +	auth_buffer_size = get_auth_descriptor2(update->var->data,
> +						update->var->data_size,
> +						&auth_buffer);
> +	if ((auth_buffer_size < 0)
> +	     || (update->var->data_size < auth_buffer_size)) {
> +		prlog(PR_ERR, "Invalid auth buffer size\n");
> +		rc = auth_buffer_size;
> +		goto out;
> +	}
> +
> +	auth = (struct efi_variable_authentication_2 *)auth_buffer;
> +
> +	if (!timestamp) {
> +		rc = OPAL_INTERNAL_ERROR;
> +		goto out;
> +	}
> +
> +	memcpy(timestamp, auth_buffer, sizeof(struct efi_time));
*timestamp = auth->timestamp?

> +
> +	rc = check_timestamp(update->var->key, timestamp, last_timestamp);
> +	/* Failure implies probably an older command being resubmitted */
> +	if (rc != OPAL_SUCCESS) {
> +		prlog(PR_INFO, "Timestamp verification failed for key %s\n", update->var->key);
> +		goto out;
> +	}
> +
> +	/* Calculate the size of new ESL data */
> +	*new_data_size = update->var->data_size - auth_buffer_size;
> +	if (*new_data_size < 0) {
> +		prlog(PR_ERR, "Invalid new ESL (new data content) size\n");
> +		rc = OPAL_PARAMETER;
> +		goto out;
> +	}
> +	*newesl = zalloc(*new_data_size);
> +	if (!(*newesl)) {
> +		rc = OPAL_NO_MEM;
> +		goto out;
> +	}
> +	memcpy(*newesl, update->var->data + auth_buffer_size, *new_data_size);
> +
> +	/* Validate the new ESL is in right format */
> +	rc = validate_esl_list(update->var->key, *newesl, *new_data_size);
> +	if (rc < 0) {
> +		prlog(PR_ERR, "ESL validation failed for key %s with error %04x\n",
> +		      update->var->key, rc);
> +		goto out;
> +	}
> +
> +	if (setup_mode) {
> +		rc = OPAL_SUCCESS;
> +		goto out;
> +	}
> +
> +	/* Prepare the data to be verified */
> +	rc = get_data_to_verify(update->var->key, *newesl, *new_data_size,
> +				&tbhbuffer, &tbhbuffersize, timestamp);
> +
> +	/* Get the authority to verify the signature */
> +	get_key_authority(key_authority, update->var->key);

> +	i = 0;
> +
> +	/* Try for all the authorities that are allowed to sign.
> +	 * For eg. db/dbx can be signed by both PK or KEK
> +	 */
> +	while (key_authority[i] != NULL) {
use a for loop

> +		prlog(PR_DEBUG, "key is %s\n", update->var->key);
> +		prlog(PR_DEBUG, "key authority is %s\n", key_authority[i]);
> +		anode = find_secvar(key_authority[i],
> +				    strlen(key_authority[i]) + 1,
> +				    bank);
considering most of the calls to find_secvar() use a string literal and
hardcode the key length you could clean things up by moving the
strlen() into find_secvar() so the prototype is just find_secvar(key,
bank).

For the few cases that need to provide the explicit length they can use
a helper that allows an explicit length to be passed in.

> +		if (!anode || !anode->var->data_size) {
> +			i++;
> +			continue;
> +		}
> +
> +		/* Verify the signature */
> +		rc = verify_signature(auth, tbhbuffer, tbhbuffersize,
> +				      anode->var);
> +
> +		/* Break if signature verification is successful */
> +		if (rc == OPAL_SUCCESS)
> +			break;
> +		i++;
> +	}

> +
> +out:
> +	free(auth_buffer);
> +	free(tbhbuffer);

So... are either of these copies actually modified? Just return const
pointer to the input buffer.

> +
> +	return rc;
> +}



diff --git a/libstb/secvar/backend/edk2-compat.c
b/libstb/secvar/backend/edk2-compat.c
> new file mode 100644
> index 00000000..334aca3e
> --- /dev/null
> +++ b/libstb/secvar/backend/edk2-compat.c
> @@ -0,0 +1,262 @@
> +// SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later
> +/* Copyright 2020 IBM Corp. */
> +#ifndef pr_fmt
> +#define pr_fmt(fmt) "EDK2_COMPAT: " fmt
> +#endif
> +
> +#include <opal.h>
> +#include <string.h>
> +#include <time.h>
> +#include <unistd.h>
> +#include <stdint.h>
> +#include <skiboot.h>
> +#include <ccan/endian/endian.h>
> +#include <mbedtls/error.h>
> +#include "libstb/crypto/pkcs7/pkcs7.h"
> +#include "edk2.h"
> +#include "../secvar.h"
> +#include "edk2-compat-process.h"
> +#include "edk2-compat-reset.h"
> +
> +struct list_head staging_bank;
> +
> +/*
> + * Initializes supported variables as empty if not loaded from
> + * storage. Variables are initialized as volatile if not found.
> + * Updates should clear this flag.
> + * Returns OPAL Error if anything fails in initialization
> + */
> +static int edk2_compat_pre_process(struct list_head *variable_bank,
> +				   struct list_head *update_bank __unused)
> +{
> +	struct secvar_node *pkvar;
> +	struct secvar_node *kekvar;
> +	struct secvar_node *dbvar;
> +	struct secvar_node *dbxvar;
> +	struct secvar_node *tsvar;
> +
> +	pkvar = find_secvar("PK", 3, variable_bank);
> +	if (!pkvar) {
> +		pkvar = new_secvar("PK", 3, NULL, 0, SECVAR_FLAG_VOLATILE
> +				| SECVAR_FLAG_PROTECTED);
> +		if (!pkvar)
> +			return OPAL_NO_MEM;
> +
> +		list_add_tail(variable_bank, &pkvar->link);
> +	}
> +	if (pkvar->var->data_size == 0)
> +		setup_mode = true;
> +	else
> +		setup_mode = false;
> +
> +	kekvar = find_secvar("KEK", 4, variable_bank);
> +	if (!kekvar) {
> +		kekvar = new_secvar("KEK", 4, NULL, 0, SECVAR_FLAG_VOLATILE);
> +		if (!kekvar)
> +			return OPAL_NO_MEM;
> +
> +		list_add_tail(variable_bank, &kekvar->link);
> +	}
> +
> +	dbvar = find_secvar("db", 3, variable_bank);
> +	if (!dbvar) {
> +		dbvar = new_secvar("db", 3, NULL, 0, SECVAR_FLAG_VOLATILE);
> +		if (!dbvar)
> +			return OPAL_NO_MEM;
> +
> +		list_add_tail(variable_bank, &dbvar->link);
> +	}
> +
> +	dbxvar = find_secvar("dbx", 4, variable_bank);
> +	if (!dbxvar) {
> +		dbxvar = new_secvar("dbx", 4, NULL, 0, SECVAR_FLAG_VOLATILE);
> +		if (!dbxvar)
> +			return OPAL_NO_MEM;
> +
> +		list_add_tail(variable_bank, &dbxvar->link);
> +	}
> +
> +	/* Should only ever happen on first boot. Timestamp is
> +	 * initialized with all zeroes. */
> +	tsvar = find_secvar("TS", 3, variable_bank);
> +	if (!tsvar) {
> +		tsvar = alloc_secvar(sizeof(struct efi_time) * 4);
> +		if (!tsvar)
> +			return OPAL_NO_MEM;
> +
> +		memcpy(tsvar->var->key, "TS", 3);
> +		tsvar->var->key_len = 3;
> +		tsvar->var->data_size = sizeof(struct efi_time) * 4;
> +		memset(tsvar->var->data, 0, tsvar->var->data_size);
> +		list_add_tail(variable_bank, &tsvar->link);
> +	}
> +
> +	return OPAL_SUCCESS;
> +};

What's the point of these null variables? If you're doing it to ensure
that find_secvar() or whatever doesn't return null pointers it's not
really buying you anything since you still have to check for zero
length data.

> +
> +static int edk2_compat_process(struct list_head *variable_bank,
> +			       struct list_head *update_bank)
> +{
> +	struct secvar_node *node = NULL;
> +	struct secvar_node *tsvar = NULL;
> +	struct efi_time timestamp;
> +	char *newesl = NULL;
> +	int neweslsize;
> +	int rc = 0;
> +
> +	prlog(PR_INFO, "Setup mode = %d\n", setup_mode);
> +
> +	/* Check HW-KEY-HASH */
> +	if (!setup_mode) {
> +		rc = verify_hw_key_hash();
> +		if (rc != OPAL_SUCCESS) {
> +			prlog(PR_ERR, "Hardware key hash verification mismatch\n");
> +			rc = reset_keystore(variable_bank);
> +			if (rc)
> +				goto cleanup;
> +			setup_mode = true;
> +			goto cleanup;
> +		}
> +	}
> +
> +	/* Return early if we have no updates to process */
> +	if (list_empty(update_bank)) {
> +		return OPAL_EMPTY;
> +	}

> +
> +	/* Make a working copy of variable bank that is updated
> +	 * during process */
> +	list_head_init(&staging_bank);
> +	copy_bank_list(&staging_bank, variable_bank);
> +
> +	/* Loop through each command in the update bank.
> +	 * If any command fails, it just loops out of the update bank.
> +	 * It should also clear the update bank.
> +	 */
> +
> +	list_for_each(update_bank, node, link) {
> +
> +		/* Submitted data is auth_2 descriptor + new ESL data
> +		 * Extract the auth_2 2 descriptor
> +		 */
> +		prlog(PR_INFO, "Update for %s\n", node->var->key);
> +
> +		tsvar = find_secvar("TS", 3, &staging_bank);
Is looking this up each loop iteration necessary?

> +
> +		/* We cannot find timestamp variable, did someone tamper it? */
> +	        if (!tsvar) {
> +			rc = OPAL_PERMISSION;
> +			break;
> +		}
> +
> +		rc = process_update(node, &newesl,
> +				    &neweslsize, &timestamp,
> +				    &staging_bank,
> +				    tsvar->var->data);
> +		if (rc) {
> +			prlog(PR_ERR, "Update processing failed with rc %04x\n", rc);
> +			break;
> +		}
> +
> +		/*
> +		 * If reached here means, signature is verified so update the
> +		 * value in the variable bank
> +		 */
> +		rc = update_variable_in_bank(node->var,
> +					     newesl,
> +					     neweslsize,
> +					     &staging_bank);
> +		if (rc) {
> +			prlog(PR_ERR, "Updating the variable data failed %04x\n", rc);
> +			break;
> +		}
> +
> +		free(newesl);
> +		/* Update the TS variable with the new timestamp */
> +		rc = update_timestamp(node->var->key,
> +				      &timestamp,
> +				      tsvar->var->data);

Is having all the timestamps in a single secvar an actual requirement
or is that just how you've decided to implement the timestamp tracking?

Seem like it'd be easier all around if you tracked it on a per-variable 
basis and stored it as a part of the serialised data format rather than
wedging them together into a variable. Wedging them also creates the
awkward data flow we're seeing here where process_update() needs to
return the timestamp via out pointers.

> +		if (rc) {
> +			prlog (PR_ERR, "Variable updated, but timestamp updated failed %04x\n", rc);
> +			break;
> +		}
> +
> +		/* If the PK is updated, update the secure boot state of the
> +		 * system at the end of processing */
> +		if (key_equals(node->var->key, "PK")) {
> +			/* PK is tied to a particular firmware image by mapping
> +			 * it with hw-key-hash of that firmware. When PK is
> +			 * updated, hw-key-hash is updated. And when PK is
> +			 * deleted, delete hw-key-hash as well */
> +			if(neweslsize == 0) {
> +				setup_mode = true;
> +				delete_hw_key_hash(&staging_bank);
> +			} else  {
> +				setup_mode = false;
> +				add_hw_key_hash(&staging_bank);
> +			}
> +			prlog(PR_DEBUG, "setup mode is %d\n", setup_mode);
> +		}
> +	}
> +
> +	if (rc == 0) {
> +		/* Update the variable bank with updated working copy */
> +		clear_bank_list(variable_bank);
> +		copy_bank_list(variable_bank, &staging_bank);

Isn't this leaking all the variables in staging bank?

> +	}
> +
> +cleanup:
> +	/* For any failure in processing update queue, we clear the update bank
> +	 * and return failure */
> +	clear_bank_list(update_bank);
> +
> +	return rc;
> +}
> +
> +static int edk2_compat_post_process(struct list_head *variable_bank,
> +				    struct list_head *update_bank __unused)
> +{
> +	struct secvar_node *hwvar;
> +	if (!setup_mode) {
> +		secvar_set_secure_mode();
> +		prlog(PR_INFO, "Enforcing OS secure mode\n");
> +		/* HW KEY HASH is no more needed after this point. It is already
> +		 * visible to userspace via device-tree, so exposing via sysfs is
> +		 * just a duplication. Remove it from in-memory copy. */
> +		hwvar = find_secvar("HWKH", 5, variable_bank);
> +		if (!hwvar) {
> +			prlog(PR_ERR, "cannot find hw-key-hash, should not happen\n");
> +			return OPAL_INTERNAL_ERROR;
> +		}
> +		list_del(&hwvar->link);
> +		dealloc_secvar(hwvar);
> +	}
> +
> +	return OPAL_SUCCESS;
> +}
> +
> +static int edk2_compat_validate(struct secvar *var)
> +{
> +
> +	/* Checks if the update is for supported
> +	 * Non-volatile secure variables */
> +	if (!key_equals(var->key, "PK")
> +			&& !key_equals(var->key, "KEK")
> +			&& !key_equals(var->key, "db")
> +			&& !key_equals(var->key, "dbx"))
> +		return OPAL_PARAMETER;

Being unable to support user variables is a bit crap. oh well.

> +	/* Check that signature type is PKCS7 */
> +	if (!is_pkcs7_sig_format(var->data))
> +		return OPAL_PARAMETER;
> +
> +	return OPAL_SUCCESS;
> +};
> +
> +struct secvar_backend_driver edk2_compatible_v1 = {
> +	.pre_process = edk2_compat_pre_process,
> +	.process = edk2_compat_process,
> +	.post_process = edk2_compat_post_process,
> +	.validate = edk2_compat_validate,
> +	.compatible = "ibm,edk2-compat-v1",
> +};

> diff --git a/libstb/secvar/backend/edk2.h b/libstb/secvar/backend/edk2.h
> new file mode 100644
> index 00000000..fb0121af
> --- /dev/null
> +++ b/libstb/secvar/backend/edk2.h
> *snip*
> +/*
> + * Certificate which encapsulates a GUID-specific digital signature
> + */
> +struct win_certificate_uefi_guid {
> +	/*
> +	 * This is the standard win_certificate header, where w_certificate_type
> +	 * is set to WIN_CERT_TYPE_EFI_GUID.
> +	 */
> +	struct win_certificate hdr;
> +	/*
> +	 * This is the unique id which determines the format of the cert_data.
> +	 */
> +	uuid_t cert_type;
> +	/*
> +	 * The following is the certificate data. The format of the data is
> +	 * determined by the @cert_type. If @cert_type is
> +	 * EFI_CERT_TYPE_RSA2048_SHA256_GUID, the @cert_data will be
> +	 * EFI_CERT_BLOCK_RSA_2048_SHA256 structure.
> +	 */
> +	u8 cert_data[1];

I'm guessing this is TianoCore-ism, but using a length-one array at the
end of a struct to indicate trailing data is something that went out of
fashion in the 90s. GCC has an extension to allow zero-length arrays at
the end of a struct for this purpose that was added decades ago. That
extension was deprecated by C99 which added a no-length array syntax
for the same purpose:

struct a {
	int start;
	char buffer[];
};

I'd suggest you use that since it allows you to do a sizeof() on the
struct variable to get the size of the structure header without the
trailing element screwing with it.

On 5/14/20 10:11 AM, Oliver O'Halloran wrote:
> On Mon, 2020-05-11 at 16:31 -0500, Eric Richter wrote:
>> From: Nayna Jain <nayna@linux.ibm.com>
>>
>> As part of secureboot key management, the scheme for handling key updates
>> is derived from tianocore reference implementation[1]. The wrappers for
>> holding the signed update is the Authentication Header and for holding
>> the public key certificate is ESL (EFI Signature List), both derived from
>> tianocore reference implementation[1].
>>
>> This patch adds the support to process update queue. This involves:
>> 1. Verification of the update signature using the key authorized as per the
>> key hierarchy
>> 2. Handling addition/deletion of the keys
>> 3. Support for dbx (blacklisting of hashes)
>> 4. Validation checks for the updates
>> 5. Supporting multiple ESLs for single variable both for update/verification
>> 6. Timestamp check
>> 7. Allowing only single PK
>> 8. Failure Handling
>> 9. Resetting keystore if the hardware key hash changes
>>
>> [1] https://github.com/tianocore/edk2-staging.git
>>
>> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
>> Signed-off-by: Eric Richter <erichte@linux.ibm.com>
>> ---
>> V4:
>>   - fixed a typo in the hw-key-hash mismatch prlog
>>   - replace PRIORITY with PROTECTED
>>   - adjust most lines to 80 columns, some exceptions and prlog statements
>>       may still run over
>>
>>   doc/secvar/edk2.rst                         |  49 ++
>>   include/secvar.h                            |   1 +
>>   libstb/secvar/backend/Makefile.inc          |   4 +-
>>   libstb/secvar/backend/edk2-compat-process.c | 717 ++++++++++++++++++++
>>   libstb/secvar/backend/edk2-compat-process.h |  61 ++
>>   libstb/secvar/backend/edk2-compat-reset.c   | 115 ++++
>>   libstb/secvar/backend/edk2-compat-reset.h   |  24 +
>>   libstb/secvar/backend/edk2-compat.c         | 262 +++++++
>>   libstb/secvar/backend/edk2.h                | 243 +++++++
>>   9 files changed, 1474 insertions(+), 2 deletions(-)
>>   create mode 100644 doc/secvar/edk2.rst
>>   create mode 100644 libstb/secvar/backend/edk2-compat-process.c
>>   create mode 100644 libstb/secvar/backend/edk2-compat-process.h
>>   create mode 100644 libstb/secvar/backend/edk2-compat-reset.c
>>   create mode 100644 libstb/secvar/backend/edk2-compat-reset.h
>>   create mode 100644 libstb/secvar/backend/edk2-compat.c
>>   create mode 100644 libstb/secvar/backend/edk2.h
>>
>> diff --git a/doc/secvar/edk2.rst b/doc/secvar/edk2.rst
>> new file mode 100644
>> index 00000000..1e4cc9e3
>> --- /dev/null
>> +++ b/doc/secvar/edk2.rst
>> @@ -0,0 +1,49 @@
>> +.. _secvar/edk2:
>> +
>> +Skiboot edk2-compatible Secure Variable Backend
>> +===============================================
>> +
>> +Overview
>> +--------
>> +
>> +The edk2 secure variable backend for skiboot borrows from edk2 concepts
>> +such as the three key hierarchy (PK, KEK, and db), and a similar
>> +structure. In general, variable updates must be signed with a key
>> +of a higher level. So, updates to the db must be signed with a key stored
>> +in the KEK; updates to the KEK must be signed with the PK. Updates to the
>> +PK must be signed with the previous PK (if any).
>> +
>> +Variables are stored in the efi signature list format, and updates are a
>> +signed variant that includes an authentication header.
>> +
>> +If no PK is currently enrolled, the system is considered to be in "Setup
>> +Mode". Any key can be enrolled without signature checks. However, once a
>> +PK is enrolled, the system switches to "User Mode", and each update must
>> +now be signed according to the hierarchy. Furthermore, when in "User
>> +Mode", the backend initialized the ``os-secure-mode`` device tree flag,
>> +signaling to the kernel that we are in secure mode.
>> +
>> +Updates are processed sequentially, in the order that they were provided
>> +in the update queue. If any update fails to validate, appears to be
>> +malformed, or any other error occurs, NO updates will not be applied.
>> +This includes updates that may have successfully applied prior to the
>> +error. The system will continue in an error state, reporting the error
>> +reason via the ``update-status`` device tree property.
>> +
>> +P9 Special Case for the Platform Key
>> +------------------------------------
>> +
>> +Due to the powerful nature of the platform key and the lack of lockable
>> +flash, the edk2 backend will store the PK in TPM NV rather than PNOR on
>> +P9 systems. (TODO expand on this)
>> +
>> +Update Status Return Codes
>> +--------------------------
>> +
>> +TODO, edk2 driver needs to actually return these properly first
>> +
>> +
>> +Device Tree Bindings
>> +--------------------
>> +
>> +TODO
>> diff --git a/include/secvar.h b/include/secvar.h
>> index 7a45db2b..4f40c115 100644
>> --- a/include/secvar.h
>> +++ b/include/secvar.h
>> @@ -28,6 +28,7 @@ struct secvar_backend_driver {
>>   };
>>   
>>   extern struct secvar_storage_driver secboot_tpm_driver;
>> +extern struct secvar_backend_driver edk2_compatible_v1;
>>   
>>   int secvar_main(struct secvar_storage_driver, struct secvar_backend_driver);
>>   
>> diff --git a/libstb/secvar/backend/Makefile.inc b/libstb/secvar/backend/Makefile.inc
>> index 6f491a63..bc987f69 100644
>> --- a/libstb/secvar/backend/Makefile.inc
>> +++ b/libstb/secvar/backend/Makefile.inc
>> @@ -1,11 +1,11 @@
>>   # SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later
>>   # -*-Makefile-*-
>>   
>> -SECVAR_BACKEND_DIR = libstb/secvar/backend
>> +SECVAR_BACKEND_DIR = $(SRC)/libstb/secvar/backend
>>   
>>   SUBDIRS += $(SECVAR_BACKEND_DIR)
>>   
>> -SECVAR_BACKEND_SRCS =
>> +SECVAR_BACKEND_SRCS = edk2-compat.c edk2-compat-process.c edk2-compat-reset.c
>>   SECVAR_BACKEND_OBJS = $(SECVAR_BACKEND_SRCS:%.c=%.o)
>>   SECVAR_BACKEND = $(SECVAR_BACKEND_DIR)/built-in.a
>>   
>> diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c
>> new file mode 100644
>> index 00000000..60ebb0b2
>> --- /dev/null
>> +++ b/libstb/secvar/backend/edk2-compat-process.c
>> @@ -0,0 +1,717 @@
>> +// SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later
>> +/* Copyright 2020 IBM Corp. */
>> +#ifndef pr_fmt
>> +#define pr_fmt(fmt) "EDK2_COMPAT: " fmt
>> +#endif
>> +
>> +#include <opal.h>
>> +#include <string.h>
>> +#include <time.h>
>> +#include <unistd.h>
>> +#include <stdint.h>
>> +#include <ccan/endian/endian.h>
>> +#include <mbedtls/error.h>
>> +#include <device.h>
>> +#include "libstb/crypto/pkcs7/pkcs7.h"
>> +#include "edk2.h"
>> +#include "../secvar.h"
>> +#include "edk2-compat-process.h"
>> +
>> +bool setup_mode;
>> +
>> +int update_variable_in_bank(struct secvar *secvar, const char *data,
>> +			    uint64_t dsize, struct list_head *bank)
>> +{
>> +	struct secvar_node *node;
>> +
>> +	node = find_secvar(secvar->key, secvar->key_len, bank);
>> +	if (!node)
>> +		return OPAL_EMPTY;
>> +
>> +        /* Reallocate the data memory, if there is change in data size */
>> +	if (node->size < dsize)
>> +		if (realloc_secvar(node, dsize))
>> +			return OPAL_NO_MEM;
>> +
>> +	if (dsize && data)
>> +		memcpy(node->var->data, data, dsize);
>> +	node->var->data_size = dsize;
>> +
>> +        /* Clear the volatile bit only if updated with positive data size */
>> +	if (dsize)
>> +		node->flags &= ~SECVAR_FLAG_VOLATILE;
>> +	else
>> +		node->flags |= SECVAR_FLAG_VOLATILE;
>> +
>> +        /* Is it required to be set everytime ? */
> you tell me
>
>> +	if ((!strncmp(secvar->key, "PK", 3))
>> +	     || (!strncmp(secvar->key, "HWKH", 5)))
>> +		node->flags |= SECVAR_FLAG_PROTECTED;
>> +
>> +	return 0;
>> +}
>> +
>> +/* Converts utf8 string to ucs2 */
>> +static char *utf8_to_ucs2(const char *key, size_t keylen)
>> +{
>> +	int i;
>> +	char *str;
>> +
>> +	str = zalloc(keylen * 2);
>> +	if (!str)
>> +		return NULL;
>> +
>> +	for (i = 0; i < keylen*2; key++) {
>> +		str[i++] = *key;
>> +		str[i++] = '\0';
>> +	}
>> +
>> +	return str;
>> +}
> This is completely broken for anything that doesn't fall into the ASCII
> subset of UTF-8.

Yes, it is the minimal implementation to convert ASCII keys into wide 
characters for the purpose of efitools compatibility. Currently, we are 
supporting only ASCII key names and we also do not allow user created 
variables in this version.

>
>> +/* Returns the authority that can sign the given key update */
>> +static void get_key_authority(const char *ret[3], const char *key)
>> +{
>> +	int i = 0;
>> +
>> +	if (key_equals(key, "PK")) {
>> +		ret[i++] = "PK";
>> +	} else if (key_equals(key, "KEK")) {
>> +		ret[i++] = "PK";
>> +	} else if (key_equals(key, "db") || key_equals(key, "dbx")) {
>> +		ret[i++] = "KEK";
>> +		ret[i++] = "PK";
>> +	}
>> +
>> +	ret[i] = NULL;
>> +}
>> +
>> +/* Returns the size of the complete ESL. */
>> +static int get_esl_signature_list_size(char *buf, size_t buflen)
> buf should be const
>> +{
>> +	EFI_SIGNATURE_LIST list;
>> +
>> +	if (buflen < sizeof(EFI_SIGNATURE_LIST))
>> +		return OPAL_PARAMETER;
>> +
>> +	memcpy(&list, buf, sizeof(EFI_SIGNATURE_LIST));
>> +
>> +	prlog(PR_DEBUG, "size of signature list size is %u\n",
>> +			le32_to_cpu(list.SignatureListSize));
>> +
>> +	return le32_to_cpu(list.SignatureListSize);
>> +}
>> +
>> +/* Copies the certificate from the ESL into cert buffer and returns the size
>> + * of the certificate
>> + */
>> +static int get_esl_cert(char *buf, size_t buflen, char **cert)
> buf should be const
>> +{
>> +	size_t sig_data_offset;
>> +	size_t size;
>> +	EFI_SIGNATURE_LIST list;
>> +
>> +	if (buflen < sizeof(EFI_SIGNATURE_LIST))
>> +		return OPAL_PARAMETER;
>> +
>> +	memcpy(&list, buf, sizeof(EFI_SIGNATURE_LIST));
> This looks familiar
>
>> +
>> +	size = le32_to_cpu(list.SignatureSize) - sizeof(uuid_t);
>> +	/* No certificate in the ESL */
>> +	if (size <= 0)
>> +		return OPAL_PERMISSION;
> size_t is unsigned so checking for less than zero doesn't do anything.
> If list.SignatureSize is too small the calculation will underflow and
> probably lead to a memory allocation failure later on.
>
>> +	if (!cert)
>> +		return OPAL_PARAMETER;
> Passing a null pointer here is a programming error, use an assert.
>
>> +	*cert = zalloc(size);
>> +	if (!(*cert))
>> +		return OPAL_NO_MEM;
>> +
>> +	prlog(PR_DEBUG,"size of signature list size is %u\n",
>> +			le32_to_cpu(list.SignatureListSize));
>> +	prlog(PR_DEBUG, "size of signature header size is %u\n",
>> +			le32_to_cpu(list.SignatureHeaderSize));
>> +	prlog(PR_DEBUG, "size of signature size is %u\n",
>> +			le32_to_cpu(list.SignatureSize));
>> +
>> +	sig_data_offset = sizeof(list) + le32_to_cpu(list.SignatureHeaderSize)
>> +		+ 16 * sizeof(uint8_t);
>> +	if (sig_data_offset > buflen) {
>> +		free(*cert);
>> +		return OPAL_PARAMETER;
>> +	}
> The free() here is only required because you're performing the
> allocation before verifying that the input buffer is long enough.
> Reverse the order, it's less code and less error prone since you don't
> have to unwind the allocation.
>
>> +	memcpy(*cert, buf + sig_data_offset, size);
>> +
>> +	return size;
>> +}
>> +
>> +/* Extracts size of the PKCS7 signed data embedded in the
>> + * struct Authentication 2 Descriptor Header.
>> + */
>> +static int get_pkcs7_len(struct efi_variable_authentication_2 *auth)
> auth should be const
>
>> +{
>> +	uint32_t dw_length;
>> +	size_t size;
>> +
>> +	if (!auth)
>> +		return OPAL_PARAMETER;
> assert
>
>> +
>> +	dw_length = le32_to_cpu(auth->auth_info.hdr.dw_length);
>> +	size = dw_length - (sizeof(auth->auth_info.hdr.dw_length)
>> +			+ sizeof(auth->auth_info.hdr.w_revision)
>> +			+ sizeof(auth->auth_info.hdr.w_certificate_type)
>> +			+ sizeof(auth->auth_info.cert_type));
>> +
>> +	return size;
>> +}
>> +
>> +int get_auth_descriptor2(void *buf, size_t buflen, char **auth_buffer)
> buf should be const
>
>> +{
>> +	struct efi_variable_authentication_2 *auth = NULL;
>> +	size_t auth_buffer_size;
>> +	int len;
>> +
>> +	if (buflen < sizeof(struct efi_variable_authentication_2))
>> +			return OPAL_PARAMETER;
>> +
>> +	auth = buf;
> You can initalise auth to buf in the declaration above.
>
>> +
>> +	len = get_pkcs7_len(auth);
>> +
>> +	/* We need PKCS7 data else there is no signature */
>> +	if (len <= 0)
>> +		return OPAL_PARAMETER;
>> +
>> +	if (!auth_buffer)
>> +		return OPAL_PARAMETER;
> should be an assert. Also, do your parameter validation before you
> start doing any actual work.
>
>> +
>> +	auth_buffer_size = sizeof(auth->timestamp) + sizeof(auth->auth_info.hdr)
>> +			   + sizeof(auth->auth_info.cert_type) + len;
>> +
>> +	if (auth_buffer_size <= 0)
>> +		return OPAL_PARAMETER;
>> +
>> +	*auth_buffer = zalloc(auth_buffer_size);
>> +	if (!(*auth_buffer))
>> +		return OPAL_NO_MEM;
>> +
>> +	memcpy(*auth_buffer, buf, auth_buffer_size);
> why are we making a copy of this?
>
>> +
>> +	return auth_buffer_size;
>> +}
>> +
>> +int validate_esl_list(char *key, char *esl, size_t size)
>> +{
>> +	int count = 0;
>> +	int signing_cert_size;
>> +	char *signing_cert = NULL;
>> +	mbedtls_x509_crt x509;
>> +	char *x509_buf = NULL;
>> +	int eslvarsize = size;
>> +	int rc = OPAL_SUCCESS;
>> +	int eslsize;
>> +	int offset = 0;
>> +
>> +	while (eslvarsize > 0) {
>> +		prlog(PR_DEBUG, "esl var size size is %d offset is %d\n", eslvarsize, offset);
>> +		if (eslvarsize < sizeof(EFI_SIGNATURE_LIST))
>> +			break;
>> +
>> +		/* Calculate the size of the ESL */
>> +		eslsize = get_esl_signature_list_size(esl, eslvarsize);
>> +		/* If could not extract the size */
>> +		if (eslsize <= 0) {
>> +			prlog(PR_ERR, "Invalid size of the ESL\n");
>> +			rc = OPAL_PARAMETER;
>> +			break;
>> +		}
>> +
>> +		/* Extract the certificate from the ESL */
>> +		signing_cert_size = get_esl_cert(esl,
>> +						 eslvarsize,
>> +						 &signing_cert);
>> +		if (signing_cert_size < 0) {
>> +			rc = signing_cert_size;
>> +			break;
>> +		}
>> +
>> +		mbedtls_x509_crt_init(&x509);
>> +		rc = mbedtls_x509_crt_parse(&x509,
>> +					    signing_cert,
>> +					    signing_cert_size);
>> +
>> +		/* If failure in parsing the certificate, exit */
>> +		if(rc) {
>> +			prlog(PR_INFO, "X509 certificate parsing failed %04x\n", rc);
>> +			rc = OPAL_PARAMETER;
>> +			break;
>> +		}
>> +
>> +		x509_buf = zalloc(CERT_BUFFER_SIZE);
>> +		rc = mbedtls_x509_crt_info(x509_buf,
>> +					   CERT_BUFFER_SIZE,
>> +					   "CRT:",
>> +					   &x509);
>> +		prlog(PR_INFO, "%s ", x509_buf);
>> +
>> +		/* If failure in reading the certificate, exit */
>> +		if (rc < 0) {
>> +			prlog(PR_INFO, "Failed to show X509 certificate info %04x\n", rc);
>> +			rc = OPAL_PARAMETER;
>> +			free(x509_buf);
>> +			break;
>> +		}
>> +		rc = 0;
>> +
>> +		free(x509_buf);
>> +		x509_buf = NULL;
>> +		count++;
>> +
>> +		/* Look for the next ESL */
>> +		offset = offset + eslsize;
>> +		eslvarsize = eslvarsize - eslsize;
>> +		mbedtls_x509_crt_free(&x509);
>> +		free(signing_cert);
>> +		/* Since we are going to allocate again in the next iteration */
>> +		signing_cert = NULL;
>> +	}
>> +
>> +	if (rc == OPAL_SUCCESS) {
>> +		if (key_equals(key, "PK") && (count > 1)) {
>> +			prlog(PR_ERR, "PK can only be one\n");
>> +			rc = OPAL_PARAMETER;
>> +		} else {
>> +			rc = count;
>> +		}
>> +	}
>> +
>> +	prlog(PR_INFO, "Total ESLs are %d\n", rc);
>> +	return rc;
>> +}
>> +
>> +/* Get the timestamp for the last update of the give key */
>> +static struct efi_time *get_last_timestamp(const char *key, char *last_timestamp)
>> +{
>> +	u8 off;
>> +
>> +	if (!last_timestamp)
>> +		return NULL;
>> +
>> +	if (!strncmp(key, "PK", 3))
>> +		off = 0;
>> +	else if (!strncmp(key, "KEK", 4))
>> +		off = 1;
>> +	else if (!strncmp(key, "db", 3))
>> +		off = 2;
>> +	else if (!strncmp(key, "dbx", 4))
>> +		off = 3;
>> +	else
>> +		return NULL;
>> +
>> +	return &((struct efi_time *)last_timestamp)[off];
>> +}
> I don't know what you're doing here, but I don't like it.
>
>
>> +int update_timestamp(char *key, struct efi_time *timestamp, char *last_timestamp)
>> +{
>> +	struct efi_time *prev;
>> +
>> +	prev = get_last_timestamp(key, last_timestamp);
>> +	if (prev == NULL)
>> +		return OPAL_INTERNAL_ERROR;
>> +
>> +	memcpy(prev, timestamp, sizeof(struct efi_time));
>> +
>> +	prlog(PR_DEBUG, "updated prev year is %d month %d day %d\n",
>> +			le16_to_cpu(prev->year), prev->month, prev->day);
>> +
>> +	return OPAL_SUCCESS;
>> +}
>> +
>> +int check_timestamp(char *key, struct efi_time *timestamp,
>> +		    char *last_timestamp)
>> +{
>> +	struct efi_time *prev;
>> +
>> +	prev = get_last_timestamp(key, last_timestamp);
>> +	if (prev == NULL)
>> +		return OPAL_INTERNAL_ERROR;
>> +
>> +	prlog(PR_DEBUG, "timestamp year is %d month %d day %d\n",
>> +			le16_to_cpu(timestamp->year), timestamp->month,
>> +			timestamp->day);
>> +	prlog(PR_DEBUG, "prev year is %d month %d day %d\n",
>> +			le16_to_cpu(prev->year), prev->month, prev->day);
>> +	if (le16_to_cpu(timestamp->year) > le16_to_cpu(prev->year))
>> +		return OPAL_SUCCESS;
>> +	if (le16_to_cpu(timestamp->year) < le16_to_cpu(prev->year))
>> +		return OPAL_PERMISSION;
>> +
>> +	if (timestamp->month > prev->month)
>> +		return OPAL_SUCCESS;
>> +	if (timestamp->month < prev->month)
>> +		return OPAL_PERMISSION;
>> +
>> +	if (timestamp->day > prev->day)
>> +		return OPAL_SUCCESS;
>> +	if (timestamp->day < prev->day)
>> +		return OPAL_PERMISSION;
>> +
>> +	if (timestamp->hour > prev->hour)
>> +		return OPAL_SUCCESS;
>> +	if (timestamp->hour < prev->hour)
>> +		return OPAL_PERMISSION;
>> +
>> +	if (timestamp->minute > prev->minute)
>> +		return OPAL_SUCCESS;
>> +	if (timestamp->minute < prev->minute)
>> +		return OPAL_PERMISSION;
>> +
>> +	if (timestamp->second > prev->second)
>> +		return OPAL_SUCCESS;
>> +
>> +	/* Time less than or equal to is considered as replay*/
>> +	if (timestamp->second <= prev->second)
>> +		return OPAL_PERMISSION;
> You could simplify this a lot by unpacking the timestamp bytes into a
> uint64 with the following format: 0xYYYYMMDDHHMMSS, then the two can be
> compared with one numeric test.
>
>> +	/* nanosecond, timezone, daylight and pad2 are meant to be zero */
> I'd verify that, but that's just me.

It is taken from here - 
https://kernel.googlesource.com/pub/scm/linux/kernel/git/jejb/efitools/+/refs/heads/master/include/efiauthenticated.h#249, 
since we also use the same tool.

Looks like I missed to add "pad1" as well in the comment. :-)


>
>> +
>> +	return OPAL_SUCCESS;
>> +}
>> +
>> +/* Extract PKCS7 from the authentication header */
>> +static int get_pkcs7(struct efi_variable_authentication_2 *auth,
>> +		     mbedtls_pkcs7 **pkcs7)
> auth should be const
>
>> +{
>> +	char *checkpkcs7cert = NULL;
>> +	int len;
>> +	int rc;
>> +
>> +	len = get_pkcs7_len(auth);
>> +	if (len <= 0)
>> +		return OPAL_PARAMETER;
>> +
>> +	if (!pkcs7)
>> +		return OPAL_PARAMETER;
>> +
>> +	*pkcs7 = malloc(sizeof(struct mbedtls_pkcs7));
>> +	if (!(*pkcs7))
>> +		return OPAL_NO_MEM;
>> +
>> +	mbedtls_pkcs7_init(*pkcs7);
>> +	rc = mbedtls_pkcs7_parse_der(
>> +			(const unsigned char *)auth->auth_info.cert_data,
>> +			(const unsigned int)len, *pkcs7);
> Why are those casts necessary?
>
>> +	if (rc) {
>> +		prlog(PR_ERR, "Parsing pkcs7 failed %04x\n", rc);
>> +		mbedtls_pkcs7_free(*pkcs7);
>> +		return rc;
>> +	}
>> +
>> +	checkpkcs7cert = zalloc(CERT_BUFFER_SIZE);
>> +	if (!checkpkcs7cert) {
>> +		mbedtls_pkcs7_free(*pkcs7);
>> +		return OPAL_NO_MEM;
>> +	}
>> +
>> +	rc = mbedtls_x509_crt_info(checkpkcs7cert, CERT_BUFFER_SIZE, "CRT:",
>> +			&((*pkcs7)->signed_data.certs));
>> +	if (rc < 0) {
>> +		prlog(PR_ERR, "Failed to parse the certificate in PKCS7 structure\n");
>> +		rc = OPAL_PARAMETER;
>> +	} else {
>> +		rc = OPAL_SUCCESS;
>> +		prlog(PR_DEBUG, "%s \n", checkpkcs7cert);
>> +	}
>> +
>> +	free(checkpkcs7cert);
>> +	mbedtls_pkcs7_free(*pkcs7);
>> +
>> +	return rc;
>> +}
>> +
>> +/* Verify the PKCS7 signature on the signed data. */
>> +static int verify_signature(struct efi_variable_authentication_2 *auth,
>> +			    char *newcert, size_t new_data_size,
>> +			    struct secvar *avar)
> const where necessary
>
>> +{
>> +	mbedtls_pkcs7 *pkcs7 = NULL;
>> +	mbedtls_x509_crt x509;
>> +	char *signing_cert = NULL;
>> +	char *x509_buf = NULL;
>> +	int signing_cert_size;
>> +	int rc;
>> +	char *errbuf;
>> +	int eslvarsize;
>> +	int eslsize;
>> +	int offset = 0;
>> +
>> +	if (!auth)
>> +		return OPAL_PARAMETER;
>> +
>> +	/* Extract the pkcs7 from the auth structure */
>> +	rc  = get_pkcs7(auth, &pkcs7);
>> +	/* Failure to parse pkcs7 implies bad input. */
>> +	if (rc != OPAL_SUCCESS)
>> +		return OPAL_PARAMETER;
>> +
>> +	prlog(PR_INFO, "Load the signing certificate from the keystore");
>> +
>> +	eslvarsize = avar->data_size;
>> +
>> +	/* Variable is not empty */
>> +	while (eslvarsize > 0) {
>> +		prlog(PR_DEBUG, "esl var size size is %d offset is %d\n", eslvarsize, offset);
>> +		if (eslvarsize < sizeof(EFI_SIGNATURE_LIST))
>> +			break;
>> +
>> +		/* Calculate the size of the ESL */
>> +		eslsize = get_esl_signature_list_size(avar->data + offset,
>> +						      eslvarsize);
>> +		/* If could not extract the size */
>> +		if (eslsize <= 0) {
>> +			rc = OPAL_PARAMETER;
>> +			break;
>> +		}
>> +
>> +		/* Extract the certificate from the ESL */
>> +		signing_cert_size = get_esl_cert(avar->data + offset,
>> +						 eslvarsize, &signing_cert);
>> +		if (signing_cert_size < 0) {
>> +			rc = signing_cert_size;
>> +			break;
>> +		}
>> +
>> +		mbedtls_x509_crt_init(&x509);
>> +		rc = mbedtls_x509_crt_parse(&x509,
>> +					    signing_cert,
>> +					    signing_cert_size);
>> +
>> +		/* This should not happen, unless something corrupted in PNOR */
>> +		if(rc) {
>> +			prlog(PR_INFO, "X509 certificate parsing failed %04x\n", rc);
>> +			rc = OPAL_INTERNAL_ERROR;
>> +			break;
>> +		}
>> +
>> +		x509_buf = zalloc(CERT_BUFFER_SIZE);
>> +		rc = mbedtls_x509_crt_info(x509_buf,
>> +					   CERT_BUFFER_SIZE,
>> +					   "CRT:",
>> +					   &x509);
>> +
>> +		/* This should not happen, unless something corrupted in PNOR */
>> +		if (rc < 0) {
>> +			free(x509_buf);
>> +			rc = OPAL_INTERNAL_ERROR;
>> +			break;
>> +		}
>> +
>> +		prlog(PR_INFO, "%s \n", x509_buf);
>> +		free(x509_buf);
>> +		x509_buf = NULL;
>> +
>> +		/* Verify the signature */
>> +		rc = mbedtls_pkcs7_signed_data_verify(pkcs7, &x509, newcert,
>> +						      new_data_size);
>> +
>> +		/* If you find a signing certificate, you are done */
>> +		if (rc == 0) {
>> +			prlog(PR_INFO, "Signature Verification passed\n");
>> +			mbedtls_x509_crt_free(&x509);
>> +			break;
>> +		}
>> +
>> +		errbuf = zalloc(MBEDTLS_ERR_BUFFER_SIZE);
>> +		mbedtls_strerror(rc, errbuf, MBEDTLS_ERR_BUFFER_SIZE);
>> +		prlog(PR_INFO, "Signature Verification failed %02x %s\n",
>> +				rc, errbuf);
>> +		free(errbuf);
>> +
>> +		/* Look for the next ESL */
>> +		offset = offset + eslsize;
>> +		eslvarsize = eslvarsize - eslsize;
>> +		mbedtls_x509_crt_free(&x509);
>> +		free(signing_cert);
>> +		/* Since we are going to allocate again in the next iteration */
>> +		signing_cert = NULL;
>> +
>> +	}
>> +
>> +	free(signing_cert);
>> +	mbedtls_pkcs7_free(pkcs7);
>> +	free(pkcs7);
>> +
>> +	return rc;
>> +}
>> +
>> +/* Create the single buffer
>> + * name || vendor guid || attributes || timestamp || newcontent
>> + * which is submitted as signed by the user.
>> + * Returns number of bytes in the new buffer, else negative error
>> + * code.
>> + */
> I'd be happier if you incrementally computed the hash on each field
> rather than copying everything constantly.
>
>> +static int get_data_to_verify(char *key, char *new_data, size_t new_data_size,
>> +			      char **buffer, size_t *buffer_size,
>> +			      struct efi_time *timestamp)
> consts
>> +{
>> +	le32 attr = cpu_to_le32(SECVAR_ATTRIBUTES);
>> +	size_t offset = 0;
>> +	size_t varlen;
>> +	char *wkey;
>> +	uuid_t guid;
>> +
>> +	if (key_equals(key, "PK")
>> +	    || key_equals(key, "KEK"))
>> +		guid = EFI_GLOBAL_VARIABLE_GUID;
>> +	else if (key_equals(key, "db")
>> +	    || key_equals(key, "dbx"))
>> +		guid = EFI_IMAGE_SECURITY_DATABASE_GUID;
>> +	else
>> +		return OPAL_INTERNAL_ERROR;
>> +
>> +	/* Convert utf8 name to ucs2 width */
>> +	varlen = strlen(key) * 2;
>> +	wkey = utf8_to_ucs2(key, strlen(key));
>> +
>> +	/* Prepare the single buffer */
>> +	*buffer_size = varlen + UUID_SIZE + sizeof(attr)
>> +		       + sizeof(struct efi_time) + new_data_size;
>> +	*buffer = zalloc(*buffer_size);
>> +	if (!*buffer)
>> +		return OPAL_NO_MEM;
>> +
>> +	memcpy(*buffer + offset, wkey, varlen);
>> +	offset = offset + varlen;
>> +	memcpy(*buffer + offset, &guid, sizeof(guid));
>> +	offset = offset + sizeof(guid);
>> +	memcpy(*buffer + offset, &attr, sizeof(attr));
>> +	offset = offset + sizeof(attr);
>> +	memcpy(*buffer + offset, timestamp , sizeof(struct efi_time));
>> +	offset = offset + sizeof(struct efi_time);
>> +
>> +	memcpy(*buffer + offset, new_data, new_data_size);
>> +	offset = offset + new_data_size;
>> +
>> +	free(wkey);
>> +
>> +	return offset;
>> +}
>> +
>> +bool is_pkcs7_sig_format(void *data)
> const
>> +{
>> +	struct efi_variable_authentication_2 *auth = data;
>> +	uuid_t pkcs7_guid = EFI_CERT_TYPE_PKCS7_GUID;
>> +
>> +	if(!(memcmp(&auth->auth_info.cert_type, &pkcs7_guid, 16) == 0))
>> +		return false;
>> +	return true;
> That's one way to do it. Alternatively:
>
> 	if (memcmp(&auth->auth_info.cert_type, &pkcs7_guid, 16) != 0)
> 		return false;
> 	return true;
>
> Or:
> 	if (memcmp(&auth->auth_info.cert_type, &pkcs7_guid, 16))
> 		return false;
> 	return true;
>
> Or even:
>
> 	return !memcmp(&auth->auth_info.cert_type, &pkcs7_guid, 16);
>
>
>> +int process_update(struct secvar_node *update, char **newesl,
>> +		   int *new_data_size, struct efi_time *timestamp,
>> +		   struct list_head *bank, char *last_timestamp)
>> +{
>> +	struct efi_variable_authentication_2 *auth = NULL;
>> +	char *auth_buffer = NULL;
> If you want a pointer to an arbitrary buffer use a void pointer rather
> than a char. Voids will automaticly coerce to another pointer type so
> you don't have to manually cast it. Also, this should be const.
>
>> +	int auth_buffer_size = 0;
>> +	const char *key_authority[3];
>> +	char *tbhbuffer = NULL;
>> +	size_t tbhbuffersize = 0;
>> +	struct secvar_node *anode = NULL;
>> +	int rc = 0;
>> +	int i;
>> +
>> +	auth_buffer_size = get_auth_descriptor2(update->var->data,
>> +						update->var->data_size,
>> +						&auth_buffer);
>> +	if ((auth_buffer_size < 0)
>> +	     || (update->var->data_size < auth_buffer_size)) {
>> +		prlog(PR_ERR, "Invalid auth buffer size\n");
>> +		rc = auth_buffer_size;
>> +		goto out;
>> +	}
>> +
>> +	auth = (struct efi_variable_authentication_2 *)auth_buffer;
>> +
>> +	if (!timestamp) {
>> +		rc = OPAL_INTERNAL_ERROR;
>> +		goto out;
>> +	}
>> +
>> +	memcpy(timestamp, auth_buffer, sizeof(struct efi_time));
> *timestamp = auth->timestamp?
>
>> +
>> +	rc = check_timestamp(update->var->key, timestamp, last_timestamp);
>> +	/* Failure implies probably an older command being resubmitted */
>> +	if (rc != OPAL_SUCCESS) {
>> +		prlog(PR_INFO, "Timestamp verification failed for key %s\n", update->var->key);
>> +		goto out;
>> +	}
>> +
>> +	/* Calculate the size of new ESL data */
>> +	*new_data_size = update->var->data_size - auth_buffer_size;
>> +	if (*new_data_size < 0) {
>> +		prlog(PR_ERR, "Invalid new ESL (new data content) size\n");
>> +		rc = OPAL_PARAMETER;
>> +		goto out;
>> +	}
>> +	*newesl = zalloc(*new_data_size);
>> +	if (!(*newesl)) {
>> +		rc = OPAL_NO_MEM;
>> +		goto out;
>> +	}
>> +	memcpy(*newesl, update->var->data + auth_buffer_size, *new_data_size);
>> +
>> +	/* Validate the new ESL is in right format */
>> +	rc = validate_esl_list(update->var->key, *newesl, *new_data_size);
>> +	if (rc < 0) {
>> +		prlog(PR_ERR, "ESL validation failed for key %s with error %04x\n",
>> +		      update->var->key, rc);
>> +		goto out;
>> +	}
>> +
>> +	if (setup_mode) {
>> +		rc = OPAL_SUCCESS;
>> +		goto out;
>> +	}
>> +
>> +	/* Prepare the data to be verified */
>> +	rc = get_data_to_verify(update->var->key, *newesl, *new_data_size,
>> +				&tbhbuffer, &tbhbuffersize, timestamp);
>> +
>> +	/* Get the authority to verify the signature */
>> +	get_key_authority(key_authority, update->var->key);
>> +	i = 0;
>> +
>> +	/* Try for all the authorities that are allowed to sign.
>> +	 * For eg. db/dbx can be signed by both PK or KEK
>> +	 */
>> +	while (key_authority[i] != NULL) {
> use a for loop
>
>> +		prlog(PR_DEBUG, "key is %s\n", update->var->key);
>> +		prlog(PR_DEBUG, "key authority is %s\n", key_authority[i]);
>> +		anode = find_secvar(key_authority[i],
>> +				    strlen(key_authority[i]) + 1,
>> +				    bank);
> considering most of the calls to find_secvar() use a string literal and
> hardcode the key length you could clean things up by moving the
> strlen() into find_secvar() so the prototype is just find_secvar(key,
> bank).
>
> For the few cases that need to provide the explicit length they can use
> a helper that allows an explicit length to be passed in.

find_secvar() is a generic call, but key format is defined by backend, 
so the reason to do this is to keep the provision for non-null 
terminated key names as well. In any case, if this is more preferred, 
then we are ok to change.

>
>> +		if (!anode || !anode->var->data_size) {
>> +			i++;
>> +			continue;
>> +		}
>> +
>> +		/* Verify the signature */
>> +		rc = verify_signature(auth, tbhbuffer, tbhbuffersize,
>> +				      anode->var);
>> +
>> +		/* Break if signature verification is successful */
>> +		if (rc == OPAL_SUCCESS)
>> +			break;
>> +		i++;
>> +	}
>> +
>> +out:
>> +	free(auth_buffer);
>> +	free(tbhbuffer);
> So... are either of these copies actually modified? Just return const
> pointer to the input buffer.
>
>> +
>> +	return rc;
>> +}
>
>
> diff --git a/libstb/secvar/backend/edk2-compat.c
> b/libstb/secvar/backend/edk2-compat.c
>> new file mode 100644
>> index 00000000..334aca3e
>> --- /dev/null
>> +++ b/libstb/secvar/backend/edk2-compat.c
>> @@ -0,0 +1,262 @@
>> +// SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later
>> +/* Copyright 2020 IBM Corp. */
>> +#ifndef pr_fmt
>> +#define pr_fmt(fmt) "EDK2_COMPAT: " fmt
>> +#endif
>> +
>> +#include <opal.h>
>> +#include <string.h>
>> +#include <time.h>
>> +#include <unistd.h>
>> +#include <stdint.h>
>> +#include <skiboot.h>
>> +#include <ccan/endian/endian.h>
>> +#include <mbedtls/error.h>
>> +#include "libstb/crypto/pkcs7/pkcs7.h"
>> +#include "edk2.h"
>> +#include "../secvar.h"
>> +#include "edk2-compat-process.h"
>> +#include "edk2-compat-reset.h"
>> +
>> +struct list_head staging_bank;
>> +
>> +/*
>> + * Initializes supported variables as empty if not loaded from
>> + * storage. Variables are initialized as volatile if not found.
>> + * Updates should clear this flag.
>> + * Returns OPAL Error if anything fails in initialization
>> + */
>> +static int edk2_compat_pre_process(struct list_head *variable_bank,
>> +				   struct list_head *update_bank __unused)
>> +{
>> +	struct secvar_node *pkvar;
>> +	struct secvar_node *kekvar;
>> +	struct secvar_node *dbvar;
>> +	struct secvar_node *dbxvar;
>> +	struct secvar_node *tsvar;
>> +
>> +	pkvar = find_secvar("PK", 3, variable_bank);
>> +	if (!pkvar) {
>> +		pkvar = new_secvar("PK", 3, NULL, 0, SECVAR_FLAG_VOLATILE
>> +				| SECVAR_FLAG_PROTECTED);
>> +		if (!pkvar)
>> +			return OPAL_NO_MEM;
>> +
>> +		list_add_tail(variable_bank, &pkvar->link);
>> +	}
>> +	if (pkvar->var->data_size == 0)
>> +		setup_mode = true;
>> +	else
>> +		setup_mode = false;
>> +
>> +	kekvar = find_secvar("KEK", 4, variable_bank);
>> +	if (!kekvar) {
>> +		kekvar = new_secvar("KEK", 4, NULL, 0, SECVAR_FLAG_VOLATILE);
>> +		if (!kekvar)
>> +			return OPAL_NO_MEM;
>> +
>> +		list_add_tail(variable_bank, &kekvar->link);
>> +	}
>> +
>> +	dbvar = find_secvar("db", 3, variable_bank);
>> +	if (!dbvar) {
>> +		dbvar = new_secvar("db", 3, NULL, 0, SECVAR_FLAG_VOLATILE);
>> +		if (!dbvar)
>> +			return OPAL_NO_MEM;
>> +
>> +		list_add_tail(variable_bank, &dbvar->link);
>> +	}
>> +
>> +	dbxvar = find_secvar("dbx", 4, variable_bank);
>> +	if (!dbxvar) {
>> +		dbxvar = new_secvar("dbx", 4, NULL, 0, SECVAR_FLAG_VOLATILE);
>> +		if (!dbxvar)
>> +			return OPAL_NO_MEM;
>> +
>> +		list_add_tail(variable_bank, &dbxvar->link);
>> +	}
>> +
>> +	/* Should only ever happen on first boot. Timestamp is
>> +	 * initialized with all zeroes. */
>> +	tsvar = find_secvar("TS", 3, variable_bank);
>> +	if (!tsvar) {
>> +		tsvar = alloc_secvar(sizeof(struct efi_time) * 4);
>> +		if (!tsvar)
>> +			return OPAL_NO_MEM;
>> +
>> +		memcpy(tsvar->var->key, "TS", 3);
>> +		tsvar->var->key_len = 3;
>> +		tsvar->var->data_size = sizeof(struct efi_time) * 4;
>> +		memset(tsvar->var->data, 0, tsvar->var->data_size);
>> +		list_add_tail(variable_bank, &tsvar->link);
>> +	}
>> +
>> +	return OPAL_SUCCESS;
>> +};
> What's the point of these null variables? If you're doing it to ensure
> that find_secvar() or whatever doesn't return null pointers it's not
> really buying you anything since you still have to check for zero
> length data.

They are actually empty variables (zero length data) and not null 
variables. Empty variables are not persisted to the storage. So, these 
are created to expose empty variables to the userspace via sysfs so that 
user can update. We need this as we do not allow user to create 
variables in this version.

>
>> +
>> +static int edk2_compat_process(struct list_head *variable_bank,
>> +			       struct list_head *update_bank)
>> +{
>> +	struct secvar_node *node = NULL;
>> +	struct secvar_node *tsvar = NULL;
>> +	struct efi_time timestamp;
>> +	char *newesl = NULL;
>> +	int neweslsize;
>> +	int rc = 0;
>> +
>> +	prlog(PR_INFO, "Setup mode = %d\n", setup_mode);
>> +
>> +	/* Check HW-KEY-HASH */
>> +	if (!setup_mode) {
>> +		rc = verify_hw_key_hash();
>> +		if (rc != OPAL_SUCCESS) {
>> +			prlog(PR_ERR, "Hardware key hash verification mismatch\n");
>> +			rc = reset_keystore(variable_bank);
>> +			if (rc)
>> +				goto cleanup;
>> +			setup_mode = true;
>> +			goto cleanup;
>> +		}
>> +	}
>> +
>> +	/* Return early if we have no updates to process */
>> +	if (list_empty(update_bank)) {
>> +		return OPAL_EMPTY;
>> +	}
>> +
>> +	/* Make a working copy of variable bank that is updated
>> +	 * during process */
>> +	list_head_init(&staging_bank);
>> +	copy_bank_list(&staging_bank, variable_bank);
>> +
>> +	/* Loop through each command in the update bank.
>> +	 * If any command fails, it just loops out of the update bank.
>> +	 * It should also clear the update bank.
>> +	 */
>> +
>> +	list_for_each(update_bank, node, link) {
>> +
>> +		/* Submitted data is auth_2 descriptor + new ESL data
>> +		 * Extract the auth_2 2 descriptor
>> +		 */
>> +		prlog(PR_INFO, "Update for %s\n", node->var->key);
>> +
>> +		tsvar = find_secvar("TS", 3, &staging_bank);
> Is looking this up each loop iteration necessary?
>
>> +
>> +		/* We cannot find timestamp variable, did someone tamper it? */
>> +	        if (!tsvar) {
>> +			rc = OPAL_PERMISSION;
>> +			break;
>> +		}
>> +
>> +		rc = process_update(node, &newesl,
>> +				    &neweslsize, &timestamp,
>> +				    &staging_bank,
>> +				    tsvar->var->data);
>> +		if (rc) {
>> +			prlog(PR_ERR, "Update processing failed with rc %04x\n", rc);
>> +			break;
>> +		}
>> +
>> +		/*
>> +		 * If reached here means, signature is verified so update the
>> +		 * value in the variable bank
>> +		 */
>> +		rc = update_variable_in_bank(node->var,
>> +					     newesl,
>> +					     neweslsize,
>> +					     &staging_bank);
>> +		if (rc) {
>> +			prlog(PR_ERR, "Updating the variable data failed %04x\n", rc);
>> +			break;
>> +		}
>> +
>> +		free(newesl);
>> +		/* Update the TS variable with the new timestamp */
>> +		rc = update_timestamp(node->var->key,
>> +				      &timestamp,
>> +				      tsvar->var->data);
> Is having all the timestamps in a single secvar an actual requirement
> or is that just how you've decided to implement the timestamp tracking?
>
> Seem like it'd be easier all around if you tracked it on a per-variable
> basis and stored it as a part of the serialised data format rather than
> wedging them together into a variable. Wedging them also creates the
> awkward data flow we're seeing here where process_update() needs to
> return the timestamp via out pointers.

The main reason is to expose the timestamps to the userspace via sysfs. 
It should be ok to make them per-variable basis, but might involve 
parsing either at API level to return data or user level to get ESLs. We 
think it would be simpler to keep it separate.


>
>> +		if (rc) {
>> +			prlog (PR_ERR, "Variable updated, but timestamp updated failed %04x\n", rc);
>> +			break;
>> +		}
>> +
>> +		/* If the PK is updated, update the secure boot state of the
>> +		 * system at the end of processing */
>> +		if (key_equals(node->var->key, "PK")) {
>> +			/* PK is tied to a particular firmware image by mapping
>> +			 * it with hw-key-hash of that firmware. When PK is
>> +			 * updated, hw-key-hash is updated. And when PK is
>> +			 * deleted, delete hw-key-hash as well */
>> +			if(neweslsize == 0) {
>> +				setup_mode = true;
>> +				delete_hw_key_hash(&staging_bank);
>> +			} else  {
>> +				setup_mode = false;
>> +				add_hw_key_hash(&staging_bank);
>> +			}
>> +			prlog(PR_DEBUG, "setup mode is %d\n", setup_mode);
>> +		}
>> +	}
>> +
>> +	if (rc == 0) {
>> +		/* Update the variable bank with updated working copy */
>> +		clear_bank_list(variable_bank);
>> +		copy_bank_list(variable_bank, &staging_bank);
> Isn't this leaking all the variables in staging bank?
>
>> +	}
>> +
>> +cleanup:
>> +	/* For any failure in processing update queue, we clear the update bank
>> +	 * and return failure */
>> +	clear_bank_list(update_bank);
>> +
>> +	return rc;
>> +}
>> +
>> +static int edk2_compat_post_process(struct list_head *variable_bank,
>> +				    struct list_head *update_bank __unused)
>> +{
>> +	struct secvar_node *hwvar;
>> +	if (!setup_mode) {
>> +		secvar_set_secure_mode();
>> +		prlog(PR_INFO, "Enforcing OS secure mode\n");
>> +		/* HW KEY HASH is no more needed after this point. It is already
>> +		 * visible to userspace via device-tree, so exposing via sysfs is
>> +		 * just a duplication. Remove it from in-memory copy. */
>> +		hwvar = find_secvar("HWKH", 5, variable_bank);
>> +		if (!hwvar) {
>> +			prlog(PR_ERR, "cannot find hw-key-hash, should not happen\n");
>> +			return OPAL_INTERNAL_ERROR;
>> +		}
>> +		list_del(&hwvar->link);
>> +		dealloc_secvar(hwvar);
>> +	}
>> +
>> +	return OPAL_SUCCESS;
>> +}
>> +
>> +static int edk2_compat_validate(struct secvar *var)
>> +{
>> +
>> +	/* Checks if the update is for supported
>> +	 * Non-volatile secure variables */
>> +	if (!key_equals(var->key, "PK")
>> +			&& !key_equals(var->key, "KEK")
>> +			&& !key_equals(var->key, "db")
>> +			&& !key_equals(var->key, "dbx"))
>> +		return OPAL_PARAMETER;
> Being unable to support user variables is a bit crap. oh well.

It is in plan for the future.


>
>> +	/* Check that signature type is PKCS7 */
>> +	if (!is_pkcs7_sig_format(var->data))
>> +		return OPAL_PARAMETER;
>> +
>> +	return OPAL_SUCCESS;
>> +};
>> +
>> +struct secvar_backend_driver edk2_compatible_v1 = {
>> +	.pre_process = edk2_compat_pre_process,
>> +	.process = edk2_compat_process,
>> +	.post_process = edk2_compat_post_process,
>> +	.validate = edk2_compat_validate,
>> +	.compatible = "ibm,edk2-compat-v1",
>> +};
>> diff --git a/libstb/secvar/backend/edk2.h b/libstb/secvar/backend/edk2.h
>> new file mode 100644
>> index 00000000..fb0121af
>> --- /dev/null
>> +++ b/libstb/secvar/backend/edk2.h
>> *snip*
>> +/*
>> + * Certificate which encapsulates a GUID-specific digital signature
>> + */
>> +struct win_certificate_uefi_guid {
>> +	/*
>> +	 * This is the standard win_certificate header, where w_certificate_type
>> +	 * is set to WIN_CERT_TYPE_EFI_GUID.
>> +	 */
>> +	struct win_certificate hdr;
>> +	/*
>> +	 * This is the unique id which determines the format of the cert_data.
>> +	 */
>> +	uuid_t cert_type;
>> +	/*
>> +	 * The following is the certificate data. The format of the data is
>> +	 * determined by the @cert_type. If @cert_type is
>> +	 * EFI_CERT_TYPE_RSA2048_SHA256_GUID, the @cert_data will be
>> +	 * EFI_CERT_BLOCK_RSA_2048_SHA256 structure.
>> +	 */
>> +	u8 cert_data[1];
> I'm guessing this is TianoCore-ism, but using a length-one array at the
> end of a struct to indicate trailing data is something that went out of
> fashion in the 90s. GCC has an extension to allow zero-length arrays at
> the end of a struct for this purpose that was added decades ago. That
> extension was deprecated by C99 which added a no-length array syntax
> for the same purpose:
>
> struct a {
> 	int start;
> 	char buffer[];
> };
>
> I'd suggest you use that since it allows you to do a sizeof() on the
> struct variable to get the size of the structure header without the
> trailing element screwing with it.
>
>
Yes, agree. However, it is not exactly as part of TianoCore-ism, but 
mainly we just borrowed the structure "as is" from the Tianocore headers 
as listed in the license of this file. I think there should not be any 
issue in changing, but will check before doing that.

We will fix all other comments. Thanks for your feedback.

Thanks & Regards,

      - Nayna