[Skiboot] [PATCH 3/7] libstb/secvar: add secvar api implementation

[Eric Richter]

On 6/13/19 7:53 PM, Stewart Smith wrote:
> Eric Richter <erichte at linux.ibm.com> writes:
>> This patch provides the OPAL runtime service frontend for the host OS to
>> retrieve secure variables, and append new ones for processing on the 
>> next reboot. These calls operate on the internal abstraction or utilize
>> the platform-provided driver hooks, and therefore this API should not
>> need to be updated to support changes in storage or backend drivers.
>>
>> Included are the following functions:
>>  - opal_secvar_get()
>>  - opal_secvar_get_size()
>>  - opal_secvar_get_next()
>>  - opal_secvar_enqueue_update()
>>  - opal_secvar_backend()
> 
> These all need additions to doc/ to document the calls and the concepts
> behind them.
> 
>>
>> opal_secvar_get() retrieves the data blob and metadata blob associated
>> with a given key. Either the data blob or the metadata blob are
>> optional. This runtime service only operates on the variable bank.
>>
>> opal_secvar_get_size() returns the size of the data and/or metadata blob
>> associated with a given key. This is a convenience function to retrieve
>> only the size values, and ignore any data blob. This runtime service
>> only operates on the variable bank.
>>
>> opal_secvar_get_next() can be used to iterate through the list of
>> variable keys in the variable bank. Supplying an empty key (or zero key
>> length) returns the key of the first variable in the variable bank.
>> Supplying a valid key returns the key of the next variable in sequence.
>>
>> opal_secvar_enqueue_update() provides a method for the host OS to submit
>> a new variable for processing on next boot, by appending it to the
>> update bank. As this does not affect the variable bank, appending a
>> variable via this runtime service will not affect the output of the
>> previous set of functions. The update queue is only processed during
>> secvar initialization.
> 
> There should probably also be an API for getting enqueued updates?
> 
> Think about adding things to a queue in petitboot, and then wanting to
> check if there's things queued once booted to an OS.

There will need to be one yes, however we have not yet figured out the
kernel/userspace way that this will be expressed. With a pseudo-fs, we
can display each variable as a file, but how are updates to be displayed?
Variable per update? All updates in one variable?

> 
> Is there a reason for having this be separate calls to the calls to
> iterate through variables?
> 

Not sure I follow the question, are you asking about why sequential calls
are needed to _get_next() to iterate all variables, or why we don't return
the data with _get_next()? 

>> opal_secvar_backend() returns an enum value for which processing backend
>> is in use. As the backend determines the format of the variables and how
>> they should be updated, the host OS should query this function prior to
>> any other secvar-related operations. The enum value returned from this
>> function will not change at runtime.
> 
> As discussed on the phone this morning, this seems to be more of a
> schema version to ensure that everybody agrees on the format of the
> secvars that are parsed by skiboot, so is probably more suited to being
> in the device tree.
> 
>> diff --git a/ccan/list/list.h b/ccan/list/list.h
>> index 7cd3a83e..fdeddeb4 100644
>> --- a/ccan/list/list.h
>> +++ b/ccan/list/list.h
>> @@ -523,4 +523,42 @@ static inline struct list_node *list_node_from_off_(void *ptr, size_t off)
>>  	(container_off_var(var, member) +		\
>>  	 check_type(var->member, struct list_node))
>>  
>> +
>> +#if HAVE_TYPEOF
>> +#define list_typeof(var) typeof(var)
>> +#else
>> +#define list_typeof(var) void *
>> +#endif
>> +
>> +
>> +/* Returns member, or NULL if at end of list. */
>> +static inline void *list_entry_or_null(const struct list_head *h,
>> +                                      const struct list_node *n,
>> +                                      size_t off)
>> +{
>> +       if (n == &h->n)
>> +               return NULL;
>> +       return (char *)n - off;
>> +}
>> +
>> +/**
>> + * list_next - get the next entry in a list
>> + * @h: the list_head
>> + * @i: a pointer to an entry in the list.
>> + * @member: the list_node member of the structure
>> + *
>> + * If @i was the last entry in the list, returns NULL.
>> + *
>> + * Example:
>> + *     struct child *second;
>> + *     second = list_next(&parent->children, first, list);
>> + *     if (!second)
>> + *             printf("No second child!\n");
>> + */
>> +#define list_next(h, i, member)                                                \
>> +       ((list_typeof(i))list_entry_or_null(list_debug(h),              \
>> +                                           (i)->member.next,           \
>> +                                           list_off_var_((i), member)))
>> +
>> +
>>  #endif /* CCAN_LIST_H */
> 
> Are these upstream? We don't want to carry patches against CCAN modules
> that aren't upsteam. I'm not sure this is really needed though, we have
> the next pointer, so why not use it?
> 

They are in upstream, however pulling the latest list module managed to
break skiboot entirely last I attempted.

I preferred using this macro when developing as it was more self-documenting
on what it was doing. It is not necessary however, I'll look into a nicer
alternative.

>> diff --git a/include/opal-api.h b/include/opal-api.h
>> index 0b0ae196..1451cb00 100644
>> --- a/include/opal-api.h
>> +++ b/include/opal-api.h
>> @@ -232,7 +232,12 @@
>>  #define OPAL_XIVE_GET_VP_STATE			170 /* Get NVT state */
>>  #define OPAL_NPU_MEM_ALLOC			171
>>  #define OPAL_NPU_MEM_RELEASE			172
>> -#define OPAL_LAST				172
>> +#define OPAL_SECVAR_GET				173
>> +#define OPAL_SECVAR_GET_SIZE			174
>> +#define OPAL_SECVAR_GET_NEXT			175
>> +#define OPAL_SECVAR_ENQUEUE_UPDATE		176
>> +#define OPAL_SECVAR_BACKEND			177
>> +#define OPAL_LAST				177
>>  
>>  #define QUIESCE_HOLD			1 /* Spin all calls at entry */
>>  #define QUIESCE_REJECT			2 /* Fail all calls with OPAL_BUSY */
>> diff --git a/libstb/secvar/Makefile.inc b/libstb/secvar/Makefile.inc
>> index 75870910..50316b48 100644
>> --- a/libstb/secvar/Makefile.inc
>> +++ b/libstb/secvar/Makefile.inc
>> @@ -7,7 +7,7 @@ SUBDIRS += $(SECVAR_DIR)
>>  include $(SECVAR_DIR)/storage/Makefile.inc
>>  include $(SECVAR_DIR)/backend/Makefile.inc
>>  
>> -SECVAR_SRCS = secvar_main.c secvar_util.c
>> +SECVAR_SRCS = secvar_main.c secvar_util.c secvar_api.c
>>  SECVAR_OBJS = $(SECVAR_SRCS:%.c=%.o)
>>  SECVAR = $(SECVAR_DIR)/built-in.a
>>  
>> diff --git a/libstb/secvar/secvar_api.c b/libstb/secvar/secvar_api.c
>> new file mode 100644
>> index 00000000..88275401
>> --- /dev/null
>> +++ b/libstb/secvar/secvar_api.c
>> @@ -0,0 +1,248 @@
>> +/* Copyright 2019 IBM Corp.
>> + *
>> + * Licensed under the Apache License, Version 2.0 (the "License");
>> + * you may not use this file except in compliance with the License.
>> + * You may obtain a copy of the License at
>> + *
>> + *      http://www.apache.org/licenses/LICENSE-2.0
>> + *
>> + * Unless required by applicable law or agreed to in writing, software
>> + * distributed under the License is distributed on an "AS IS" BASIS,
>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
>> + * implied.
>> + * See the License for the specific language governing permissions and
>> + * limitations under the License.
>> + */
>> +
>> +#ifndef pr_fmt
>> +#define pr_fmt(fmt) "SECVAR_API: " fmt
>> +#endif
>> +
>> +#include <opal.h>
>> +#include "secvar.h"
>> +
>> +
>> +static int64_t opal_secvar_get(uint64_t k_key, uint64_t k_key_len,
>> uint64_t k_metadata, uint64_t k_metadata_size, uint64_t k_data,
>> uint64_t k_data_size)
> 
> you can use pointers in the arguments here, you don't need to cast
> things below.
> 
>> +{
>> +	struct secvar_node *node;
>> +
>> +	char *key = (char *) k_key;
>> +	uint64_t key_len = (uint64_t) k_key_len;
>> +	void *metadata = (void*) k_metadata;
>> +	uint64_t *metadata_size = (uint64_t *) k_metadata_size;
>> +	void *data = (void *) k_data;
>> +	uint64_t *data_size = (uint64_t *) k_data_size;
>> +
>> +	int64_t rc = OPAL_SUCCESS;
>> +
>> +	if (!secvar_enabled)
>> +		return OPAL_UNSUPPORTED;
>> +	if (!key)
>> +		return OPAL_PARAMETER;
>> +	if (key_len == 0)
>> +		return OPAL_PARAMETER;
>> +	// buffer and size must both be valid OR both must be NULL
>> +	if ((!!metadata) != (!!metadata_size))
>> +		return OPAL_PARAMETER;
>> +	if ((!!data) != (!!data_size))
>> +		return OPAL_PARAMETER;
>> +
>> +	node = find_secvar(key, key_len, &variable_bank);
>> +
>> +	if (!node)
>> +		return OPAL_EMPTY; // Variable not found, bail early
>> +
>> +	if (metadata) {
>> +		if (*metadata_size < node->var->metadata_size)
>> +			rc = OPAL_PARTIAL;
>> +		else
>> +			memcpy(metadata, node->var->metadata, node->var->metadata_size);
>> +		*metadata_size = node->var->metadata_size;
>> +	}
>> +
>> +	if (data) {
>> +		if (*data_size < node->var->data_size)
>> +			rc = OPAL_PARTIAL;
>> +		else
>> +			memcpy(data, node->var->data, node->var->data_size);
>> +		*data_size = node->var->data_size;
>> +	}
>> +
>> +	return rc;
>> +}
>> +opal_call(OPAL_SECVAR_GET, opal_secvar_get, 6);
>> +
>> +
>> +static int64_t opal_secvar_get_size(uint64_t k_key, uint64_t k_key_len, uint64_t k_metadata_size, uint64_t k_data_size)
>> +{
>> +	struct secvar_node *node;
>> +
>> +	char *key = (char *) k_key;
>> +	uint64_t *metadata_size = (uint64_t *) k_metadata_size;
>> +	uint64_t *data_size = (uint64_t *) k_data_size;
>> +
>> +	if (!secvar_enabled)
>> +		return OPAL_UNSUPPORTED;
>> +	if (!key)
>> +		return OPAL_PARAMETER;
>> +	if (k_key_len == 0)
>> +		return OPAL_PARAMETER;
>> +	if ((!metadata_size) && (!data_size)) // Should return at least one of them
>> +		return OPAL_PARAMETER;
>> +
>> +	node = find_secvar(key, k_key_len, &variable_bank);
>> +	if (!node)
>> +		return OPAL_EMPTY;
>> +
>> +	if (metadata_size)
>> +		*metadata_size = node->var->metadata_size;
>> +	if (data_size)
>> +		*data_size = node->var->data_size;
>> +
>> +	return OPAL_SUCCESS;
>> +}
>> +opal_call(OPAL_SECVAR_GET_SIZE, opal_secvar_get_size, 4);
> 
> Why couldn't this be satisfied by passing NULL for the data parameter
> ofr OPAL_SECVAR_GET ?
> 

It can be. The size function was split out to keep the _get() function slightly
simpler, however I fear that complexity managed to creep back in anyway.

>> +static int64_t opal_secvar_get_next(uint64_t k_key, uint64_t
>> k_key_len, uint64_t k_key_size)
> 
> What's the difference between k_key_len and k_key_size?
> 
> I'm also not sure what's up with the k_ prefixes, but that isn't really
> part of any coding style for skiboot.
> 

To differentiate the argument from the casted local variable. Will be
removed alongside the unnecessary casts.