[Skiboot] [PATCH 3/7] libstb/secvar: add secvar api implementation
Stewart Smith
stewart at linux.ibm.com
Fri Jun 14 10:53:10 AEST 2019
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.
Is there a reason for having this be separate calls to the calls to
iterate through variables?
> 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?
> 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 ?
> +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.
--
Stewart Smith
OPAL Architect, IBM.
More information about the Skiboot
mailing list