diff mbox series

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

Message ID 20190625220215.27134-9-erichte@linux.ibm.com
State RFC
Headers show
Series Add Secure Variable Support | expand

Checks

Context Check Description
snowpatch_ozlabs/apply_patch success Successfully applied on branch master (b904cb733750de1bb0e04e5012c391a9c3094d11)
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot fail Test snowpatch/job/snowpatch-skiboot on branch master
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco success Signed-off-by present

Commit Message

Eric Richter June 25, 2019, 10:02 p.m. UTC
From: Nayna Jain <nayna@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@linux.ibm.com>
Signed-off-by: Eric Richter <erichte@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%)

Comments

Oliver O'Halloran July 4, 2019, 9:17 a.m. UTC | #1
On Tue, 2019-06-25 at 17:02 -0500, Eric Richter wrote:
> From: Nayna Jain <nayna@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@linux.ibm.com>
> Signed-off-by: Eric Richter <erichte@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
diff mbox series

Patch

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
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;
+}
+**/
+
+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 (!pkvar)
+		return OPAL_INTERNAL_ERROR;
+	setupvar = find_secvar((char *)"SetupMode", 10, &variable_bank);
+	if (!setupvar)
+		return OPAL_INTERNAL_ERROR;
+	sbvar = find_secvar((char *)"SecureBoot", 11, &variable_bank);
+	if (!sbvar)
+		return OPAL_INTERNAL_ERROR;
+
+	if (pkvar->var->data_size == 0)
+		newval = 1;
+	else
+		newval = 0;
+	memcpy(setupvar->var->data, &newval, sizeof(u8));
+
+	if (newval == 1)
+		enable = 0;
+	else
+		enable = 1;
+	memcpy(sbvar->var->data, &enable, sizeof(u8));
+
+	return 0;
+}
+
+static int add_volatile_variables(void) {
+	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);
+	}
+
+	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);
+	}
+	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));
+		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));
+		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;
+}
+
+/**
+ * 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;
+
+	len  = get_pkcs7_len(auth);
+
+	pkcs7 = malloc(sizeof(struct pkcs7));
+	memset(pkcs7, 0, sizeof(struct pkcs7));
+
+	rc =  pkcs7_parse_message(
+			(const unsigned char *)auth->auth_info.cert_data,
+			(const unsigned int)len, pkcs7);
+
+	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);
+	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));
+	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);
+	memcpy(tbh_buffer + size, unode->metadata, unode->metadata_size);
+	size = size + unode->metadata_size;
+	memcpy(tbh_buffer + size, auth_buffer, sizeof(struct efi_time));
+	size = size + sizeof(struct efi_time);
+	memcpy(tbh_buffer + size, new_data, new_data_size);
+	size = size + new_data_size;
+
+	*buffer = malloc(size);
+	memset(*buffer, 0, size);
+	memcpy(*buffer, tbh_buffer, size);
+	*buffer_size = size;
+
+	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);
+
+		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);
+		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");
+
+			/* 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)
+					|| (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);
+
+			/* Verify the signature */
+			//rc = verify_update(auth_buffer, tbhbuffer,
+			//		   tbhbuffersize, anode->var);
+			if (rc)
+				return OPAL_SECVAR_AUTH_FAILED;
+
+		}
+
+		/*
+		 * 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);
+		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
+	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;
+
+	//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
+
 enum {
 	SECVAR_VARIABLE_BANK,
 	SECVAR_UPDATE_BANK,