diff mbox series

[PULL,14/24] hw/uefi: add var-service-json.c + qapi for NV vars.

Message ID 20250304124815.591749-15-kraxel@redhat.com (mailing list archive)
State New
Headers show
Series [PULL,01/24] Add support for etc/hardware-info fw_cfg file | expand

Commit Message

Gerd Hoffmann March 4, 2025, 12:48 p.m. UTC
Define qapi schema for the uefi variable store state.

Use it and the generated visitor helper functions to store persistent
(EFI_VARIABLE_NON_VOLATILE) variables in JSON format on disk.

Acked-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-ID: <20250225163031.1409078-15-kraxel@redhat.com>
[ incremental fix squashed in ]
Message-ID: <pji24p6oag7cn2rovus7rquo7q2c6tokuquobfro2sqorky7vu@tk7cxud6jw7f>
---
 hw/uefi/var-service-json.c | 243 +++++++++++++++++++++++++++++++++++++
 qapi/meson.build           |   1 +
 qapi/qapi-schema.json      |   1 +
 qapi/uefi.json             |  64 ++++++++++
 4 files changed, 309 insertions(+)
 create mode 100644 hw/uefi/var-service-json.c
 create mode 100644 qapi/uefi.json

Comments

Peter Maydell March 18, 2025, 4:39 p.m. UTC | #1
On Tue, 4 Mar 2025 at 12:49, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> Define qapi schema for the uefi variable store state.
>
> Use it and the generated visitor helper functions to store persistent
> (EFI_VARIABLE_NON_VOLATILE) variables in JSON format on disk.
>
> Acked-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Message-ID: <20250225163031.1409078-15-kraxel@redhat.com>
> [ incremental fix squashed in ]
> Message-ID: <pji24p6oag7cn2rovus7rquo7q2c6tokuquobfro2sqorky7vu@tk7cxud6jw7f>

Hi; Coverity points out some problems in this code
(CID 1593154, 1593157):


> +void uefi_vars_json_load(uefi_vars_state *uv, Error **errp)
> +{
> +    UefiVarStore *vs;
> +    QObject *qobj;
> +    Visitor *v;
> +    char *str;
> +    size_t len;
> +    int rc;
> +
> +    if (uv->jsonfd == -1) {
> +        return;
> +    }
> +
> +    len = lseek(uv->jsonfd, 0, SEEK_END);

lseek() can return -1 on error, but we don't check that
here, so we can proceed forward with a negative length...

> +    if (len == 0) {
> +        return;
> +    }
> +
> +    str = g_malloc(len + 1);

...so we might try to malloc(0) here...

> +    lseek(uv->jsonfd, 0, SEEK_SET);
> +    rc = read(uv->jsonfd, str, len);

...and read() will treat -1 as a very large positive
value, overrunning the buffer.

> +    if (rc != len) {
> +        warn_report("%s: read error", __func__);

If the read failed to read all the data, we need to
bail out now, not proceed to pass the uninitialized
buffer to qobject_from_json().

Coverity doesn't notice it, but the code in
uefi_vars_json_save() is also not handling errors
from lseek() etc correctly.

Do we absolutely need to be operating on a continuously open
filedescriptor here rather than a filename? If we could make
these functions use uv->jsonfile each time then we could use
the glib functions g_file_get_contents() and
g_file_set_contents(), which get all the pesky details of the
error handling right for you.

thanks
-- PMM
Gerd Hoffmann March 19, 2025, 8:29 a.m. UTC | #2
Hi,

> Hi; Coverity points out some problems in this code
> (CID 1593154, 1593157):

I'll send fixes later today.

> Do we absolutely need to be operating on a continuously open
> filedescriptor here rather than a filename? If we could make
> these functions use uv->jsonfile each time then we could use
> the glib functions g_file_get_contents() and
> g_file_set_contents(), which get all the pesky details of the
> error handling right for you.

qemu_create() works only once in case libvirt hands us a
file handle instead of a filename.

take care,
  Gerd
Peter Maydell March 19, 2025, 10:18 a.m. UTC | #3
On Wed, 19 Mar 2025 at 08:29, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > Hi; Coverity points out some problems in this code
> > (CID 1593154, 1593157):
>
> I'll send fixes later today.
>
> > Do we absolutely need to be operating on a continuously open
> > filedescriptor here rather than a filename? If we could make
> > these functions use uv->jsonfile each time then we could use
> > the glib functions g_file_get_contents() and
> > g_file_set_contents(), which get all the pesky details of the
> > error handling right for you.
>
> qemu_create() works only once in case libvirt hands us a
> file handle instead of a filename.

If we have to work on a filehandle, we have the problem
that if the host crashes after the ftruncate() and
before the write() of the new data then we just lost
the contents of the var store. Working on filenames
would let us do "write new file and then rename in to
position". If you need to try to operate on a filehandle
in a "don't lose the data for crashes mid-write" that
gets tricky. Do we care ?

-- PMM
Gerd Hoffmann March 19, 2025, 11:39 a.m. UTC | #4
On Wed, Mar 19, 2025 at 10:18:08AM +0000, Peter Maydell wrote:
> On Wed, 19 Mar 2025 at 08:29, Gerd Hoffmann <kraxel@redhat.com> wrote:
> > qemu_create() works only once in case libvirt hands us a
> > file handle instead of a filename.
> 
> If we have to work on a filehandle, we have the problem
> that if the host crashes after the ftruncate() and
> before the write() of the new data then we just lost
> the contents of the var store. Working on filenames
> would let us do "write new file and then rename in to
> position".

Yes.  Drawback of the libvirt filehandle passing is that
this rename trick is not going to work.

> If you need to try to operate on a filehandle
> in a "don't lose the data for crashes mid-write" that
> gets tricky. Do we care ?

Do we have a chance to care?

Probably this is still better than what we had before.  Pflash updates
are slow and need lots of vmexits, so I guess there is a higher chance
to end up with a corrupted varstore in case of a host reset in a
unpleasent moment.

take care,
  Gerd
diff mbox series

Patch

diff --git a/hw/uefi/var-service-json.c b/hw/uefi/var-service-json.c
new file mode 100644
index 000000000000..761082c11fc1
--- /dev/null
+++ b/hw/uefi/var-service-json.c
@@ -0,0 +1,243 @@ 
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * uefi vars device - serialize non-volatile varstore from/to json,
+ *                    using qapi
+ *
+ * tools which can read/write these json files:
+ *  - https://gitlab.com/kraxel/virt-firmware
+ *  - https://github.com/awslabs/python-uefivars
+ */
+#include "qemu/osdep.h"
+#include "qemu/cutils.h"
+#include "qemu/error-report.h"
+#include "system/dma.h"
+
+#include "hw/uefi/var-service.h"
+
+#include "qobject/qobject.h"
+#include "qobject/qjson.h"
+
+#include "qapi/dealloc-visitor.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/qobject-output-visitor.h"
+#include "qapi/qapi-types-uefi.h"
+#include "qapi/qapi-visit-uefi.h"
+
+static char *generate_hexstr(void *data, size_t len)
+{
+    static const char hex[] = {
+        '0', '1', '2', '3', '4', '5', '6', '7',
+        '8', '9', 'a', 'b', 'c', 'd', 'e', 'f',
+    };
+    uint8_t *src = data;
+    char *dest;
+    size_t i;
+
+    dest = g_malloc(len * 2 + 1);
+    for (i = 0; i < len * 2;) {
+        dest[i++] = hex[*src >> 4];
+        dest[i++] = hex[*src & 15];
+        src++;
+    }
+    dest[i++] = 0;
+
+    return dest;
+}
+
+static UefiVarStore *uefi_vars_to_qapi(uefi_vars_state *uv)
+{
+    UefiVarStore *vs;
+    UefiVariableList **tail;
+    UefiVariable *v;
+    QemuUUID be;
+    uefi_variable *var;
+
+    vs = g_new0(UefiVarStore, 1);
+    vs->version = 2;
+    tail = &vs->variables;
+
+    QTAILQ_FOREACH(var, &uv->variables, next) {
+        if (!(var->attributes & EFI_VARIABLE_NON_VOLATILE)) {
+            continue;
+        }
+
+        v = g_new0(UefiVariable, 1);
+        be = qemu_uuid_bswap(var->guid);
+        v->guid = qemu_uuid_unparse_strdup(&be);
+        v->name = uefi_ucs2_to_ascii(var->name, var->name_size);
+        v->attr = var->attributes;
+
+        v->data = generate_hexstr(var->data, var->data_size);
+
+        if (var->attributes &
+            EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS) {
+            v->time = generate_hexstr(&var->time, sizeof(var->time));
+            if (var->digest && var->digest_size) {
+                v->digest = generate_hexstr(var->digest, var->digest_size);
+            }
+        }
+
+        QAPI_LIST_APPEND(tail, v);
+    }
+    return vs;
+}
+
+static unsigned parse_hexchar(char c)
+{
+    switch (c) {
+    case '0' ... '9': return c - '0';
+    case 'a' ... 'f': return c - 'a' + 0xa;
+    case 'A' ... 'F': return c - 'A' + 0xA;
+    default: return 0;
+    }
+}
+
+static void parse_hexstr(void *dest, char *src, int len)
+{
+    uint8_t *data = dest;
+    size_t i;
+
+    for (i = 0; i < len; i += 2) {
+        *(data++) =
+            parse_hexchar(src[i]) << 4 |
+            parse_hexchar(src[i + 1]);
+    }
+}
+
+static void uefi_vars_from_qapi(uefi_vars_state *uv, UefiVarStore *vs)
+{
+    UefiVariableList *item;
+    UefiVariable *v;
+    QemuUUID be;
+    uefi_variable *var;
+    uint8_t *data;
+    size_t i, len;
+
+    for (item = vs->variables; item != NULL; item = item->next) {
+        v = item->value;
+
+        var = g_new0(uefi_variable, 1);
+        var->attributes = v->attr;
+        qemu_uuid_parse(v->guid, &be);
+        var->guid = qemu_uuid_bswap(be);
+
+        len = strlen(v->name);
+        var->name_size = len * 2 + 2;
+        var->name = g_malloc(var->name_size);
+        for (i = 0; i <= len; i++) {
+            var->name[i] = v->name[i];
+        }
+
+        len = strlen(v->data);
+        var->data_size = len / 2;
+        var->data = data = g_malloc(var->data_size);
+        parse_hexstr(var->data, v->data, len);
+
+        if (v->time && strlen(v->time) == 32) {
+            parse_hexstr(&var->time, v->time, 32);
+        }
+
+        if (v->digest) {
+            len = strlen(v->digest);
+            var->digest_size = len / 2;
+            var->digest = g_malloc(var->digest_size);
+            parse_hexstr(var->digest, v->digest, len);
+        }
+
+        QTAILQ_INSERT_TAIL(&uv->variables, var, next);
+    }
+}
+
+static GString *uefi_vars_to_json(uefi_vars_state *uv)
+{
+    UefiVarStore *vs = uefi_vars_to_qapi(uv);
+    QObject *qobj = NULL;
+    Visitor *v;
+    GString *gstr;
+
+    v = qobject_output_visitor_new(&qobj);
+    if (visit_type_UefiVarStore(v, NULL, &vs, NULL)) {
+        visit_complete(v, &qobj);
+    }
+    visit_free(v);
+    qapi_free_UefiVarStore(vs);
+
+    gstr = qobject_to_json_pretty(qobj, true);
+    qobject_unref(qobj);
+
+    return gstr;
+}
+
+void uefi_vars_json_init(uefi_vars_state *uv, Error **errp)
+{
+    if (uv->jsonfile) {
+        uv->jsonfd = qemu_create(uv->jsonfile, O_RDWR, 0666, errp);
+    }
+}
+
+void uefi_vars_json_save(uefi_vars_state *uv)
+{
+    GString *gstr;
+    int rc;
+
+    if (uv->jsonfd == -1) {
+        return;
+    }
+
+    gstr = uefi_vars_to_json(uv);
+
+    lseek(uv->jsonfd, 0, SEEK_SET);
+    rc = ftruncate(uv->jsonfd, 0);
+    if (rc != 0) {
+        warn_report("%s: ftruncate error", __func__);
+    }
+    rc = write(uv->jsonfd, gstr->str, gstr->len);
+    if (rc != gstr->len) {
+        warn_report("%s: write error", __func__);
+    }
+    fsync(uv->jsonfd);
+
+    g_string_free(gstr, true);
+}
+
+void uefi_vars_json_load(uefi_vars_state *uv, Error **errp)
+{
+    UefiVarStore *vs;
+    QObject *qobj;
+    Visitor *v;
+    char *str;
+    size_t len;
+    int rc;
+
+    if (uv->jsonfd == -1) {
+        return;
+    }
+
+    len = lseek(uv->jsonfd, 0, SEEK_END);
+    if (len == 0) {
+        return;
+    }
+
+    str = g_malloc(len + 1);
+    lseek(uv->jsonfd, 0, SEEK_SET);
+    rc = read(uv->jsonfd, str, len);
+    if (rc != len) {
+        warn_report("%s: read error", __func__);
+    }
+    str[len] = 0;
+
+    qobj = qobject_from_json(str, errp);
+    v = qobject_input_visitor_new(qobj);
+    visit_type_UefiVarStore(v, NULL, &vs, errp);
+    visit_free(v);
+
+    if (!(*errp)) {
+        uefi_vars_from_qapi(uv, vs);
+        uefi_vars_update_storage(uv);
+    }
+
+    qapi_free_UefiVarStore(vs);
+    qobject_unref(qobj);
+    g_free(str);
+}
diff --git a/qapi/meson.build b/qapi/meson.build
index e7bc54e5d047..eadde4db307f 100644
--- a/qapi/meson.build
+++ b/qapi/meson.build
@@ -65,6 +65,7 @@  if have_system
     'pci',
     'rocker',
     'tpm',
+    'uefi',
   ]
 endif
 if have_system or have_tools
diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
index b1581988e4eb..2877aff73d0c 100644
--- a/qapi/qapi-schema.json
+++ b/qapi/qapi-schema.json
@@ -81,3 +81,4 @@ 
 { 'include': 'vfio.json' }
 { 'include': 'cryptodev.json' }
 { 'include': 'cxl.json' }
+{ 'include': 'uefi.json' }
diff --git a/qapi/uefi.json b/qapi/uefi.json
new file mode 100644
index 000000000000..bdfcabe1df4d
--- /dev/null
+++ b/qapi/uefi.json
@@ -0,0 +1,64 @@ 
+# -*- Mode: Python -*-
+# vim: filetype=python
+#
+
+##
+# = UEFI Variable Store
+#
+# The qemu efi variable store implementation (hw/uefi/) uses this to
+# store non-volatile variables in json format on disk.
+#
+# This is an existing format already supported by (at least) two other
+# projects, specifically https://gitlab.com/kraxel/virt-firmware and
+# https://github.com/awslabs/python-uefivars.
+##
+
+##
+# @UefiVariable:
+#
+# UEFI Variable.  Check the UEFI specifification for more detailed
+# information on the fields.
+#
+# @guid: variable namespace GUID
+#
+# @name: variable name, in UTF-8 encoding.
+#
+# @attr: variable attributes.
+#
+# @data: variable value, encoded as hex string.
+#
+# @time: variable modification time.  EFI_TIME struct, encoded as hex
+#     string.  Used only for authenticated variables, where the
+#     EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS attribute bit
+#     is set.
+#
+# @digest: variable certificate digest.  Used to verify the signature
+#     of updates for authenticated variables.  UEFI has two kinds of
+#     authenticated variables.  The secure boot variables ('PK',
+#     'KEK', 'db' and 'dbx') have hard coded signature checking rules.
+#     For other authenticated variables the firmware stores a digest
+#     of the signing certificate at variable creation time, and any
+#     updates must be signed with the same certificate.
+#
+# Since: 10.0
+##
+{ 'struct' : 'UefiVariable',
+  'data' : { 'guid'  : 'str',
+             'name'  : 'str',
+             'attr'  : 'int',
+             'data'  : 'str',
+             '*time' : 'str',
+             '*digest' : 'str'}}
+
+##
+# @UefiVarStore:
+#
+# @version: currently always 2
+#
+# @variables: list of UEFI variables
+#
+# Since: 10.0
+##
+{ 'struct' : 'UefiVarStore',
+  'data' : { 'version'   : 'int',
+             'variables' : [ 'UefiVariable' ] }}