[Skiboot] [RFC PATCH v2 8/9] secvar/backend: add edk2 derived key updates processing

Oliver O'Halloran oohall at gmail.com
Thu Jul 4 19:17:56 AEST 2019


On Tue, 2019-06-25 at 17:02 -0500, Eric Richter wrote:
> From: Nayna Jain <nayna at linux.ibm.com>
> 
> As part of secureboot key management, the scheme for key updates
> processing is derived from tianocore reference implementation[1].
> This includes the verification of key updates signed in the form of
> PKCS7 structure.
> 
> This patch adds the PKCS7 verification support for the signed
> updates processed by the user. It also adds the preprocessing code
> which initializes the empty non-volatile variables and the secureboot
> state of the system.
> 
> This patch is still a work-in-progress, for example. it still needs
> to add the support for post-processing steps and better failure handling.
> 
> V2:
>  - fixed memcpy based on sizeof(keylen) rather than the value
>  - added version and compatible values to driver struct
>  - renamed to edk2-compat
> 
> [1] https://github.com/tianocore/edk2-staging.git
> 
> Signed-off-by: Nayna Jain <nayna at linux.ibm.com>
> Signed-off-by: Eric Richter <erichte at linux.ibm.com>
> ---
>  include/secvar.h                              |   1 +
>  libstb/secvar/backend/Makefile.inc            |   2 +-
>  libstb/secvar/backend/edk2-compat/data.h      |  70 +++
>  .../secvar/backend/edk2-compat/edk2-compat.c  | 536 ++++++++++++++++++
>  .../backend/{edk2 => edk2-compat}/edk2.h      |   0
>  libstb/secvar/secvar.h                        |   3 +
>  6 files changed, 611 insertions(+), 1 deletion(-)
>  create mode 100644 libstb/secvar/backend/edk2-compat/data.h
>  create mode 100644 libstb/secvar/backend/edk2-compat/edk2-compat.c
>  rename libstb/secvar/backend/{edk2 => edk2-compat}/edk2.h (100%)
> 
> diff --git a/include/secvar.h b/include/secvar.h
> index ebc5d399..1e125009 100644
> --- a/include/secvar.h
> +++ b/include/secvar.h
> @@ -42,6 +42,7 @@ struct secvar_backend_driver {
>  };
>  
>  extern struct secvar_storage_driver secboot_p9_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 7a7ca1f7..ab6e5b9e 100644
> --- a/libstb/secvar/backend/Makefile.inc
> +++ b/libstb/secvar/backend/Makefile.inc
> @@ -4,7 +4,7 @@ SECVAR_BACKEND_DIR = libstb/secvar/backend
>  
>  SUBDIRS += $(SECVAR_BACKEND_DIR)
>  
> -SECVAR_BACKEND_SRCS =
> +SECVAR_BACKEND_SRCS = ./edk2-compat/edk2-compat.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/data.h b/libstb/secvar/backend/edk2-compat/data.h
> new file mode 100644
> index 00000000..fe927537
> --- /dev/null
> +++ b/libstb/secvar/backend/edk2-compat/data.h
> @@ -0,0 +1,70 @@
> +/**
> + * This file defines the static empty supported variables.
> + * This sort of acts as whitelist for us. It also allows user
> + * to view the variables supported even if empty.
> + */
> +
> +
> +#ifndef SECVAR_DATA_H
> +#define SECVAR_DATA_H
> +
> +#include "../../secvar.h"
> +
> +struct secvar pk = {
> +	.key = {'P', 'K', '\0'},
> +	.key_len = 3,
> +	.metadata = {'P', 0, 'K', 0, 0x8b, 0xe4, 0xdf, 0x61, 0x93, 0xca, 0x11, \
> +		     0xd2, 0xaa, 0x0d, 0x00, 0xe0, 0x98, 0x03, 0x2b, 0x8c, \
> +		     0x00, 0x00, 0x00, 0x07},
> +	.metadata_size = 24,
> +	.data = {},
> +	.data_size = 0,
> +};
> +
> +struct secvar kek = {
> +	.key = {'K', 'E', 'K', '\0'},
> +	.key_len = 4,
> +	.metadata = {'K', 0, 'E', 0, 'K', 0, 0x8b, 0xe4, 0xdf, 0x61, 0x93, \
> +		      0xca, 0x11, 0xd2, 0xaa, 0x0d, 0x00, 0xe0, 0x98, 0x03, \
> +		      0x2b, 0x8c, 0x00, 0x00, 0x00, 0x07},
> +	.metadata_size = 26,
> +	.data = {},
> +	.data_size = 0,
> +};
> +
> +struct secvar db = {
> +	.key = {'d', 'b', '\0'},
> +	.key_len = 3,
> +	.metadata = {'d', 0, 'b', 0, 0xd7, 0x19, 0xb2, 0xcb, 0x3d, 0x3a, 0x45, \
> +		     0x96, 0xa3, 0xbc, 0xda, 0xd0, 0x0e, 0x67, 0x65, 0x6f, \
> +		     0x00, 0x00, 0x01, 0x07},
> +	.metadata_size = 24,
> +	.data = {},
> +	.data_size = 0,
> +};
> +
> +struct secvar setup = {
> +	.key = {'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', '\0'},
> +	.key_len = 10,
> +	.metadata = {'S', 0, 'e', 0, 't', 0, 'u', 0, 'p', 0, 'M', 0, 'o', 0, \
> +		     'd', 0, 'e', 0, 0x8b, 0xe4, 0xdf, 0x61, 0x93, 0xca, 0x11, \
> +		     0xd2, 0xaa, 0x0d, 0x00, 0xe0, 0x98, 0x03, 0x2b, 0x8c, \
> +		     0x00, 0x00, 0x00, 0x06},
> +	.metadata_size = 38,
> +	.data = {0x01},
> +	.data_size = 1,
> +};
> +
> +struct secvar secureboot = {
> +	.key = {'S', 'e', 'c', 'u', 'r', 'e', 'B', 'o', 'o', 't', '\0'},
> +	.key_len = 11,
> +	.metadata = {'S', 0, 'e', 0, 'c', 0, 'u', 0, 'r', 0, 'e', 0, 'B', 0, \
> +		     'o', 0, 'o', 0, 't', 0, 0x8b, 0xe4, 0xdf, 0x61, 0x93, \
> +		     0xca, 0x11, 0xd2, 0xaa, 0x0d, 0x00, 0xe0, 0x98, 0x03, \
> +		     0x2b, 0x8c, 0x00, 0x00, 0x00, 0x06},
> +	.metadata_size = 40,
> +	.data = {0x00},
> +	.data_size = 1,
> +};
> +
> +#endif

If you absolutely have to define variables in a header file then make
sure they're marked as static. If the file gets included in multiple
places each resulting .o file will have a copy of these symbols. You
can "fix" that by making them as static, but it's better if header
files just have an extern declaration and put the definition in a .c
file somewhere.

> diff --git a/libstb/secvar/backend/edk2-compat/edk2-compat.c b/libstb/secvar/backend/edk2-compat/edk2-compat.c
> new file mode 100644
> index 00000000..c87e5755
> --- /dev/null
> +++ b/libstb/secvar/backend/edk2-compat/edk2-compat.c
> @@ -0,0 +1,536 @@
> +/*
> + * Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved. This
> + * program and the accompanying materials are licensed and made available
> + * under the terms and conditions of the 2-Clause BSD License which
> + * accompanies this distribution.
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above copyright notice,
> + *    this list of conditions and the following disclaimer.
> + *
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + * 
> + * Some of the concepts in this file are derived from the edk2-staging[1] repo
> + * of tianocore reference implementation
> + * [1] https://github.com/tianocore/edk2-staging
> + * 
> + * Copyright 2019 IBM Corp.
> + */
> +
> +#include <opal.h>
> +#include <string.h>
> +#include <time.h>
> +#include <unistd.h>
> +#include <stdint.h>
> +#include <ccan/endian/endian.h>
> +//#include <verify_sig.h>
> +//#include <pkcs7.h>
> +#include "data.h"
> +#include "edk2.h"
> +#include "opal-api.h"
> +#include "../../secvar.h"
> +

> +/**
> +static int esl_get_cert_size(unsigned char *buf)
> +{
> +	EFI_SIGNATURE_LIST list;
> +	uint32_t sigsize;
> +
> +	memset(&list, 0, sizeof(EFI_SIGNATURE_LIST));
> +	memcpy(&list, buf, sizeof(EFI_SIGNATURE_LIST));
> +
> +	sigsize = le32_to_cpu(list.SignatureListSize) - sizeof(list)
> +		- le32_to_cpu(list.SignatureHeaderSize);
> +
> +	return sigsize;
> +}
> +

> +static int esl_get_cert(unsigned char *buf, unsigned char **cert)
> +{
> +	int sig_data_offset;
> +	int size;
> +	EFI_SIGNATURE_LIST list;
> +
> +	memset(&list, 0, sizeof(EFI_SIGNATURE_LIST));
> +	memcpy(&list, buf, sizeof(EFI_SIGNATURE_LIST));
> +
> +	sig_data_offset = sizeof(list.SignatureType)
> +		+ sizeof(list.SignatureListSize)
> +		+ sizeof(list.SignatureHeaderSize)
> +		+ sizeof(list.SignatureSize)
> +		+ le32_to_cpu(list.SignatureHeaderSize)
> +		+ 16 * sizeof(uint8_t);
> +
> +	size = le32_to_cpu(list.SignatureSize) - sizeof(EFI_SIGNATURE_LIST);
> +	memcpy(*cert, buf + sig_data_offset, size);
> +
> +	return 0;
> +}
> +**/
Why is cert a char **? You dereference it anyway.

> +static int update_secureboot_state(void)
> +{
> +	struct secvar_node *setupvar;
> +	struct secvar_node *sbvar;
> +	struct secvar_node *pkvar;
> +	u8 newval;
> +	u8 enable;
> +
> +	/* Wondering what is the best return value if any of these
> +	 * variables are not found.
> +	 */
> +	pkvar = find_secvar((char *)"PK", 3, &variable_bank);

If you find yourself putting in casts to make things work you are
almost certainly doing it wrong. Make the first arg of find_secvar() a
"const char *" rather than "char *" and everything should work properly
without casts.

Also, passing the key length manually is brittle and generally poor API
design. Rename the current find_secvar() to __find_secvar() and make
find_secvar() just take the key string and have it compute the length
for you rather than open coding it.

> +static int add_volatile_variables(void) {
Brace goes on the new line for function definitions.

> +	struct secvar_node *setupvar;
> +	struct secvar_node *sbvar;
> +	int rc;
> +
> +	setupvar = find_secvar((char *)"SetupMode", 10, &variable_bank);
> +	if (!setupvar) {
> +		setupvar = zalloc(sizeof(struct secvar_node));
> +		if (!setupvar)
> +			return OPAL_NO_MEM;
> +
> +		setupvar->var = (struct secvar *)&setup;
> +		setupvar->var->data_size = sizeof(u8);
> +		list_add_tail(&variable_bank, &setupvar->link);
> +	}

Why do we need to make a faked variable for this? Why not just set a
flag or something if we know we're in setup mode.

> +	sbvar = find_secvar((char *)"SecureBoot", 11, &variable_bank);
> +	if (!sbvar) {
> +		sbvar = zalloc(sizeof(struct secvar_node));
> +		if (!sbvar)
> +			return OPAL_NO_MEM;
> +
> +		sbvar->var = (struct secvar *)&secureboot;
> +		sbvar->var->data_size = sizeof(u8);
> +		list_add_tail(&variable_bank, &sbvar->link);
> +	}

Same question.

> +	rc = update_secureboot_state();
> +
> +	return rc;
> +}
> +
> +/**
> + * Initializes the supported variables as empty
> + *
> + * Returns OPAL Error if anything fails in initialization
> + */
> +static int edk2_compat_pre_process(void)
> +{
> +	struct secvar_node *pkvar;
> +	struct secvar_node *kekvar;
> +	struct secvar_node *dbvar;
> +	int rc;
> +
> +	/* First initializes all non-volatile variables */
> +	pkvar = find_secvar((char *)"PK", 3, &variable_bank);
> +	if (!pkvar) {
> +		pkvar = zalloc(sizeof(struct secvar_node));
> +		if (!pkvar)
> +			return OPAL_NO_MEM;
> +
> +		pkvar->var = (struct secvar *)&pk;
> +		list_add_tail(&variable_bank, &pkvar->link);
> +	}
> +

> +	kekvar = find_secvar((char *)"KEK", 4, &variable_bank);
> +	if (!kekvar) {
> +		kekvar = zalloc(sizeof(struct secvar_node));

use zalloc(sizeof(*kekvar)) rather than open coding the type. The
sizeof(*ptr) syntax is preferable since it ensures the allocation is
always correctly sized for type.

> +		if (!kekvar)
> +			return OPAL_NO_MEM;
> +
> +		kekvar->var = (struct secvar *)&kek;
> +		list_add_tail(&variable_bank, &kekvar->link);
> +	}
> +
> +	dbvar = find_secvar((char *)"db", 3, &variable_bank);
> +	if (!dbvar) {
> +		dbvar = zalloc(sizeof(struct secvar_node));
sizeof(*dbvar)
> +		if (!dbvar)
> +			return OPAL_NO_MEM;
> +
> +		dbvar->var = (struct secvar *)&db;
> +		list_add_tail(&variable_bank, &dbvar->link);
> +	}
> +
> +	/* Initializes volatile variables */
> +	rc = add_volatile_variables();
> +
> +	return rc;
> +};
> +
> +/**
> + * Extracts size of the PKCS7 signed data embedded in the
> + * struct Authentication Descriptor 2 Header
> + */
> +static int get_pkcs7_len(struct efi_variable_authentication_2 *auth)
> +{
> +	uint32_t dw_length = le32_to_cpu(auth->auth_info.hdr.dw_length);
> +	int size;
> +
> +	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;
> +}
> +
> +/**
> + * The data submitted by the user is
> + * auth_descriptor_2 + new ESL data
> + * This function returns the size of the auth_descriptor_2
> + */
> +static int get_auth_buffer_size(void *data)
> +{
> +	struct efi_variable_authentication_2 *auth;
> +	uint64_t auth_buffer_size;
> +	int len = 0;
> +
> +	auth = (struct efi_variable_authentication_2 *)data;
> +
> +	len = get_pkcs7_len(auth);
> +
> +	auth_buffer_size = sizeof(struct efi_time)
> +		+ sizeof(u32)
> +		+ sizeof(u16)
> +		+ sizeof(u16)
> +		+ sizeof(struct efi_guid)
> +		+ len;
> +
> +	return auth_buffer_size;
> +}
> +
> +/**
> + * Returns if setup mode is enabled / disabled.
> + */
> +static int is_setup_mode(void)
> +{
> +	struct secvar_node *setup;
> +	u8 val;
> +
> +	setup = find_secvar((char *)"SetupMode", 10, &variable_bank);
> +	memset(&val, 0, sizeof(u8));
> +	memcpy(&val, setup->var->data, sizeof(val));
> +	printf("setup mode value is %d\n", val);
> +	if (val == 1)
> +		return true;
> +	else
> +		return false;
> +
> +	return false;
> +}
> +

> +/**
> + * Update the variable with the new value.
> + */
> +static int add_to_variable_bank(struct secvar *secvar, void *data, uint64_t dsize)
> +{
> +	struct secvar_node *node;
> +
> +	node = find_secvar(secvar->key, secvar->key_len, &variable_bank);
> +	if (!node)
> +		return OPAL_INTERNAL_ERROR;
> +
> +	node->var->data_size = dsize;
> +	memset(node->var->data, 0, sizeof(node->var->data));
> +	memcpy(node->var->data, data, dsize);
> +
> +	return 0;
> +}

bounds checking?

> +
> +/**
> + * Verifies the PKCS7 signature on the signed data.
> + * This function is currently commented because of its dependency on the
> + * crypto library(mbedtls + pkcs7).
> + */
> +/**
> +static int verify_update(void *auth_buffer, unsigned char *newcert,
> +		uint64_t new_data_size, struct secvar *avar)
> +{
> +	struct efi_variable_authentication_2 *auth;
> +	struct pkcs7 *pkcs7;
> +	int len = 0;
> +	int signing_cert_size = 0;
> +	unsigned char *signing_cert;
> +	unsigned char *x509_buf;
> +	mbedtls_x509_crt x509;
> +	int rc = 0;
> +
> +	auth = (struct efi_variable_authentication_2 *) auth_buffer;

void pointers convert to any pointer type without casting so ditch the
cast.

> +
> +	len  = get_pkcs7_len(auth);
> +

> +	pkcs7 = malloc(sizeof(struct pkcs7));
> +	memset(pkcs7, 0, sizeof(struct pkcs7));
zalloc

> +
> +	rc =  pkcs7_parse_message(
> +			(const unsigned char *)auth->auth_info.cert_data,
> +			(const unsigned int)len, pkcs7);

make len and unsigned int and ditch the cast. casting cert_data is
probably avoidable too.

> +
> +	printf("----Load the signing certificate from the keystore----");
> +
> +	signing_cert_size = esl_get_cert_size(avar->data);
> +	signing_cert = malloc(signing_cert_size);
> +	memset(signing_cert, 0, signing_cert_size);
zalloc

> +	esl_get_cert(avar->data, &signing_cert);
> +
> +	printf("\n");
> +	printf("----Print the signing certificate used for verification----\n");
> +	printf("\n");
> +
> +	mbedtls_x509_crt_init(&x509);
> +	rc = mbedtls_x509_crt_parse(&x509, signing_cert, signing_cert_size);
> +	if(rc) {
> +		printf("X509 certificate parsing failed %04x\n", rc);
> +		return rc;
> +	}
> +
> +	x509_buf = malloc(2048);
> +	memset(x509_buf, 0, sizeof(x509_buf));
zalloc

> +	mbedtls_x509_crt_info(x509_buf, 2048, "CRT:", &x509);
> +	printf("%s \n", x509_buf);
> +
> +	printf("----Verify the signature on the new ESL using the signing public key----\n");
> +	rc = verify_buf(signing_cert, signing_cert_size, newcert, new_data_size,
> +			pkcs7->signed_data.signers.sig.p,
> +			pkcs7->signed_data.signers.sig.len);
> +
> +	if (rc)
> +		printf("Signature Verification failed %02x\n", rc);
> +	else
> +		printf("Signature Verification passed\n");
> +
> +	return rc;
> +}
> +**/
> +
> +/**
> + * Create the single buffer (name, vendor guid, attributes,timestamp and
> + * newdata) which was originally signed by the user
> + */
> +static int concatenate_data_tobehashed(struct secvar *unode,
> +		unsigned char *auth_buffer,
> +		unsigned char *new_data,
> +		uint64_t new_data_size,
> +		unsigned char **buffer,
> +		uint64_t *buffer_size)
> +{
> +	unsigned char *tbh_buffer;
> +	int tbh_buffer_size;
> +	int size = 0;
> +
> +	tbh_buffer_size = sizeof(struct efi_time)
> +		+ unode->metadata_size
> +		+ new_data_size;

> +	tbh_buffer = malloc(tbh_buffer_size);
> +
> +	memset(tbh_buffer, 0, tbh_buffer_size);
zalloc

> +	size = size + unode->metadata_size;
> +	size = size + sizeof(struct efi_time);
> +	size = size + new_data_size;
use +=

> +
> +	*buffer = malloc(size);
> +	memset(*buffer, 0, size);
> +	memcpy(*buffer, tbh_buffer, size);
> +	*buffer_size = size;

Er, this is pointless. You allocated tbh_buffer and copied each
component into it, so why make another copy? It's also leaking
tbh_buffer.

> +
> +	return 0;
> +}
> +
> +static int edk2_compat_process(void)
> +{
> +	unsigned char *auth_buffer;
> +	uint64_t auth_buffer_size;
> +	uint64_t new_data_size = 0;
> +	unsigned char *dbcert = NULL;
> +	struct secvar_node *anode = NULL;
> +	struct secvar_node *node = NULL;
> +	unsigned char *tbhbuffer;
> +	uint64_t tbhbuffersize;
> +	int rc;
> +	bool setupmode = is_setup_mode();
> +
> +	printf("setup mode value is %d\n", setupmode);
> +
> +	/* 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. That logic is TODO.
> +	 */
> +	list_for_each(&update_bank, node, link) {
> +		printf("update for the variable %s\n",node->var->key);
> +
> +		/* Submitted data is auth_descriptor_2 + new ESL data
> +		 * Extract the size of auth_descriptor_2
> +		 */
> +		auth_buffer_size = get_auth_buffer_size(node->var->data);

> +		auth_buffer = malloc(auth_buffer_size);
> +		memset(auth_buffer, 0, auth_buffer_size);
> +		memcpy(auth_buffer, node->var->data, auth_buffer_size);
zalloc

> +
> +		if (node->var->data_size < auth_buffer_size)
> +			return OPAL_PARAMETER;
> +
> +		/* Calculate the size of new ESL data */
> +		new_data_size = node->var->data_size - auth_buffer_size;

> +		dbcert = malloc(new_data_size);
> +		memset(dbcert, 0, new_data_size);
zalloc

> +		memcpy(dbcert, node->var->data + auth_buffer_size, new_data_size);
> +

> +		if (setupmode)
> +			printf("Inside setup mode\n");
> +
> +		if (!setupmode) {
> +			printf("Inside user mode\n");

hmm

> +
> +			/* If the update is for PK, verify it with existing PK */
> +			if (memcmp(node->var->key,"PK",node->var->key_len) == 0) {
> +				anode = find_secvar((char *)"PK", 3,
> +						    &variable_bank);
> +				if (anode && (anode->var->data_size == 0))
> +					return OPAL_SECVAR_AUTH_FAILED;
> +			}
> +
> +			/* If the update is for KEK/DB, verify it with PK */
> +			if ((memcmp(node->var->key,"KEK", node->var->key_len) == 0)

Write a helper instead of checking for the name using memcmp().

> +					|| (memcmp(node->var->key, "db",
> +						   node->var->key_len) == 0)) {
> +				anode = find_secvar((char *)"PK", 3,
> +						    &variable_bank);
> +				if ((anode && (anode->var->data_size == 0))
> +						&& (memcmp(node->var->key,
> +							   "KEK",
> +							   node->var->key_len) == 0)) {
> +					printf("validation of %s failed\n", node->var->key);
> +					return OPAL_SECVAR_AUTH_FAILED;
> +				}
> +			}
> +
> +			/* If the update is for db, and previous verification
> +			 * via PK fails, check if it is signed by any of the
> +			 * KEKs
> +			 */
> +			if (memcmp(node->var->key, "db",
> +				   node->var->key_len) == 0) {
> +				anode = find_secvar((char *)"KEK", 4,
> +						    &variable_bank);
> +				if (anode && (anode->var->data_size == 0)) {
> +					printf("validation of %s failed\n", node->var->key);
> +					return OPAL_SECVAR_AUTH_FAILED;
> +				}
> +			}
> +

> +			/* Create the buffer on which signature was generated */
> +			rc = concatenate_data_tobehashed(node->var,
> +							 auth_buffer, dbcert,
> +							 new_data_size,
> +							 &tbhbuffer,
> +							 &tbhbuffersize);

most cryptographic libraries allow you to calculate a hash using
partial buffers so you can do something like:

	hash_init_context(ctx);
	hash(ctx, buf[0..99]);
	hash(ctx, buf[100..199]);
	hash(ctx, buf[200..299]);

and get the same result as:

	hash_init_context(ctx);
	hash(ctx, buf[0..299]);

Could you do that here? It'd save a pile of allocations and all the
buffer shuffling.

> +
> +			/* Verify the signature */
> +			//rc = verify_update(auth_buffer, tbhbuffer,
> +			//		   tbhbuffersize, anode->var);
> +			if (rc)
> +				return OPAL_SECVAR_AUTH_FAILED;

free() tbhbuffer?

> +
> +		}
> +
> +		/*
> +		 * If reached here means, signature is verified so update the
> +		 * value in the variable bank
> +		 */
> +		add_to_variable_bank(node->var, dbcert, new_data_size);
> +
> +		/* If the PK is updated, update the secure boot state of the
> +		 * system */
> +		if (memcmp(node->var->key, "PK",
> +			   node->var->key_len) == 0)
> +			update_secureboot_state();
> +
> +		/* Delete the command processed */
> +		list_del(&node->link);
You should be using the safe list iterator. Also, is there any cleanup
of node or var that needs to be done here?

> +		printf("variable list length is %d\n", list_length(&variable_bank));
> +	}
> +
> +	return rc;
> +}
> +

> +static int edk2_compat_post_process(void)
> +{
> +	//Update the PNOR and TPM hashes.
> +	//Do the Platform Auth

What does platform auth involve?

> +	return 0;
> +};
> +
> +static int edk2_compat_validate(struct secvar *var)
> +{
> +
> +	//Checks if the update is for supported
> +	//Non-volatile secure variales
> +	if (memcmp(var->key, "PK", 3) == 0)
> +		return 1;
> +	if (memcmp(var->key, "KEK", 4) == 0)
> +		return 1;
> +	if (memcmp(var->key, "db", 3) == 0)
> +		return 1;
You can use the helper I mentioned above to clean this up a bit.

> +
> +	//Some more checks needs to be added.
> +	//Do we want to add GUID check ?
> +	//Do we want to check here that the auth descriptor
> +	//has PKCS7 signed data. It implies we should open the data here
> +	//and parse through it. Is that ok ?
> +
> +	return 0;
> +};
> +
> +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",
> +	.version = 1,
> +};

> diff --git a/libstb/secvar/backend/edk2/edk2.h b/libstb/secvar/backend/edk2-compat/edk2.h
> similarity index 100%
> rename from libstb/secvar/backend/edk2/edk2.h
> rename to libstb/secvar/backend/edk2-compat/edk2.h
> diff --git a/libstb/secvar/secvar.h b/libstb/secvar/secvar.h
> index eb4f4558..00cb4d29 100644
> --- a/libstb/secvar/secvar.h
> +++ b/libstb/secvar/secvar.h
> @@ -25,6 +25,9 @@
>  #define SECVAR_MAX_METADATA_SIZE	1024
>  #define SECVAR_MAX_DATA_SIZE		2048
>  
> +//secvar specific error codes
> +#define OPAL_SECVAR_AUTH_FAILED		0x01

If this is for internal use only then ditch the OPAL_ prefix. If it's
for external consumption then it needs to be in opal-api.h.


Oliver



More information about the Skiboot mailing list