[Skiboot] [PATCH v2 4/9] libstb/secvar: add secvar api implementation
Oliver O'Halloran
oohall at gmail.com
Thu Jul 4 20:19:45 AEST 2019
On Tue, 2019-06-25 at 17:02 -0500, Eric Richter wrote:
> 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_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.
>
> V2:
> - removed opal_secvar_backend, replaced by DT node
> - removed unnecessary argument casting
> - all calls return OPAL_RESOURCE if secvar failed to init
>
> Signed-off-by: Eric Richter <erichte at linux.ibm.com>
> ---
> ccan/list/list.h | 38 +++++++
> include/opal-api.h | 6 +-
> libstb/secvar/Makefile.inc | 2 +-
> libstb/secvar/secvar_api.c | 215 +++++++++++++++++++++++++++++++++++++
> 4 files changed, 259 insertions(+), 2 deletions(-)
> create mode 100644 libstb/secvar/secvar_api.c
>
> 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 */
> diff --git a/include/opal-api.h b/include/opal-api.h
> index 0b0ae196..df1803df 100644
> --- a/include/opal-api.h
> +++ b/include/opal-api.h
> @@ -232,7 +232,11 @@
> #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_LAST 176
>
> #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..36c27c15
> --- /dev/null
> +++ b/libstb/secvar/secvar_api.c
> @@ -0,0 +1,215 @@
> +/* 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(char *key, uint64_t key_len, void *metadata, uint64_t *metadata_size, void *data, uint64_t *data_size)
make it const char *key to mark it as an input rather than an output.
I'm kind of wondering if it's really necessary to make the metadata
explicitly part of the API since there's nothing stopping you from
putting the metadata in a seperate key and using some backend-specific
handshaking. You could do seomthing like:
secvar_get("KEY-NAME", <blah>);
secvar_get("KEY-NAME-edk-metadata", <blah>);
and it'll do what you need just fine. The kernel side and the secvar
backend need to agree on a protocol for updates, but there's nothing
stopping you from tweaking the intermediate representation.
It's also possible we might need additional metadata channels for a
future backend so baking one into the OPAL API seems a bit premature.
Food for thought.
> +{
> + struct secvar_node *node;
> + int64_t rc = OPAL_SUCCESS;
> +
> + if (!secvar_enabled)
> + return OPAL_UNSUPPORTED;
> + if (!secvar_ready)
> + return OPAL_RESOURCE;
> + 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(char *key, uint64_t key_len, uint64_t *metadata_size, uint64_t *data_size)
Is there a compelling reason to have this as a seperate function,
rather than doing something like:
opal_secvar_get("key", strlen("key"), NULL, &md_len, NULL, &d_len);
the kernel can define a helper if it wants to.
> +static int64_t opal_secvar_get_next(char *key, uint64_t *key_len, uint64_t key_size)
Rename key_size to key_buf_size so it's clear what it is. having to
read code to work out how key_len and key_size are different is
annoying. The fact key_len is doing double duty as an input and an
output doesn't help either.
> +{
> + struct secvar_node *node;
> + int i = 0;
> +
> + if (!secvar_enabled)
> + return OPAL_UNSUPPORTED;
> + if (!secvar_ready)
> + return OPAL_RESOURCE;
> + if (!key_len)
> + return OPAL_PARAMETER;
> + if (key_size == 0)
> + return OPAL_PARAMETER;
> + if (*key_len > SECVAR_MAX_KEY_LEN)
> + return OPAL_PARAMETER;
> + if (*key_len > key_size)
> + return OPAL_PARAMETER;
> + if (!key)
> + return OPAL_PARAMETER;
> +
> + // This should fall through if *key_len == 0, otherwise check for empty
> + for (i = 0; i < *key_len; i++) {
> + // If the buffer is not empty, we assume it is a key
> + if (key[i] != 0) {
> + goto have_key;
just break out of the loop when you hit a non-zero char, then you can
do:
if(key)
node = find_secvar(key, *key_len, &variable_bank);
else
node = list_top(&variable_bank, struct secvar_node, link);
and the control flow is nice and linear. goto has it's uses, but this
ain't one of them. Also, a later patch introduces is_key_empty() as a
helper, using it here might help.
> + // Non-empty string
> + node = find_secvar(key, *key_len, &variable_bank);
> + if (!node)
> + return OPAL_PARAMETER;
> +
> + node = list_next(&variable_bank, node, link);
> +
> +send_var:
> + if (!node)
> + return OPAL_EMPTY;
> +
> + if (key_size < node->var->key_len) {
> + *key_len = node->var->key_len;
> + return OPAL_PARTIAL;
> + }
> +
> + *key_len = node->var->key_len;
> + memcpy(key, node->var->key, node->var->key_len);
> +
> + return OPAL_SUCCESS;
> +}
> +opal_call(OPAL_SECVAR_GET_NEXT, opal_secvar_get_next, 3);
> +static int64_t opal_secvar_enqueue_update(char *key, uint64_t key_len, void *metadata, uint64_t metadata_size, void *data, uint64_t data_size)
const char *key
> +{
> + struct secvar_node *node;
> +
> + if (!secvar_enabled)
> + return OPAL_UNSUPPORTED;
> + if (!secvar_ready)
> + return OPAL_RESOURCE;
> + if (!key)
> + return OPAL_PARAMETER;
> + if (key_len == 0)
> + return OPAL_PARAMETER;
> + if (key_len > SECVAR_MAX_KEY_LEN)
> + return OPAL_PARAMETER;
> + if (metadata && (metadata_size > SECVAR_MAX_METADATA_SIZE))
> + return OPAL_PARAMETER;
> + if (!data)
> + return OPAL_PARAMETER;
> + if (data_size == 0)
> + return OPAL_PARAMETER; // we don't support variable deletion this way
how does deletion work? i think i asked last time, but i forgot the
answer.
> + if (data_size > SECVAR_MAX_DATA_SIZE)
> + return OPAL_PARAMETER;
> +
> + // Key should not be empty
> + if (is_key_empty(key, key_len)) {
> + return OPAL_PARAMETER;
> + }
no braces around single line if statements
> +
> + node = zalloc(sizeof(struct secvar_node));
> + if (!node)
> + return OPAL_NO_MEM;
> +
> + node->var = zalloc(sizeof(struct secvar));
> + if (!node->var)
> + return OPAL_NO_MEM;
bailing here leaks node above.
> + node->flags = SECVAR_FLAG_DYN_ALLOC;
> +
> + memcpy(node->var->key, key, key_len);
> + node->var->key_len = key_len;
> + memcpy(node->var->data, data, data_size);
> + node->var->data_size = data_size;
> +
> + if (metadata) {
> + memcpy(node->var->metadata, metadata, metadata_size);
> + node->var->metadata_size = metadata_size;
> + }
> + else {
should be: } else {
> + memset(node->var->metadata, 0x00, SECVAR_MAX_METADATA_SIZE);
> + node->var->metadata_size = 0;
node was zalloc()ed above, so setting things to zero is unnecessary.
put in a comment if you feel it needs to be explicit.
> + }
> +
> + list_add_tail(&update_bank, &node->link);
> +
> + if (!secvar_storage.write_bank)
> + return OPAL_HARDWARE;
Seems pointless to let it come this far if there's no backend. Why not
move this check to where the rest of the sanity checks are in the
prelude?
> +
> + secvar_storage.write_bank(&update_bank, SECVAR_UPDATE_BANK);
> +
> + return OPAL_SUCCESS;
> +}
> +opal_call(OPAL_SECVAR_ENQUEUE_UPDATE, opal_secvar_enqueue_update, 6);
More information about the Skiboot
mailing list