Re: [PATCH v6 04/28] hw/s390x/ipl: Create certificate store

[Thomas Huth <thuth@redhat.com>]

On 18/09/2025 01.21, Zhuoying Cai wrote:
> Create a certificate store for boot certificates used for secure IPL.
> 
> Load certificates from the `boot-certs` parameter of s390-ccw-virtio
> machine type option into the cert store.
> 
> Currently, only X.509 certificates in PEM format are supported, as the
> QEMU command line accepts certificates in PEM format only.
> 
> Signed-off-by: Zhuoying Cai <zycai@linux.ibm.com>
> ---
>   docs/specs/s390x-secure-ipl.rst |  15 +++
>   hw/s390x/cert-store.c           | 213 ++++++++++++++++++++++++++++++++
>   hw/s390x/cert-store.h           |  39 ++++++
>   hw/s390x/ipl.c                  |  19 +++
>   hw/s390x/ipl.h                  |   3 +
>   hw/s390x/meson.build            |   1 +
>   include/hw/s390x/ipl/qipl.h     |   2 +
>   7 files changed, 292 insertions(+)
>   create mode 100644 docs/specs/s390x-secure-ipl.rst
>   create mode 100644 hw/s390x/cert-store.c
>   create mode 100644 hw/s390x/cert-store.h
> 
> diff --git a/docs/specs/s390x-secure-ipl.rst b/docs/specs/s390x-secure-ipl.rst
> new file mode 100644
> index 0000000000..9b1de5c604
> --- /dev/null
> +++ b/docs/specs/s390x-secure-ipl.rst
> @@ -0,0 +1,15 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +s390 Certificate Store and Functions
> +====================================
> +
> +s390 Certificate Store
> +----------------------
> +
> +A certificate store is implemented for s390-ccw guests to retain within
> +memory all certificates provided by the user via the command-line, which
> +are expected to be stored somewhere on the host's file system. The store
> +will keep track of the number of certificates, their respective size,
> +and a summation of the sizes.
> +
> +Note: A maximum of 64 certificates are allowed to be stored in the certificate store.

fit into 80 columns, please.

> diff --git a/hw/s390x/cert-store.c b/hw/s390x/cert-store.c
> new file mode 100644
> index 0000000000..318acfb1f6
> --- /dev/null
> +++ b/hw/s390x/cert-store.c
> @@ -0,0 +1,213 @@
> +/*
> + * S390 certificate store implementation
> + *
> + * Copyright 2025 IBM Corp.
> + * Author(s): Zhuoying Cai <zycai@linux.ibm.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "cert-store.h"
> +#include "qapi/error.h"
> +#include "qemu/error-report.h"
> +#include "qemu/option.h"
> +#include "qemu/config-file.h"
> +#include "hw/s390x/ebcdic.h"
> +#include "hw/s390x/s390-virtio-ccw.h"
> +#include "qemu/cutils.h"
> +#include "crypto/x509-utils.h"
> +#include "qapi/qapi-types-machine-s390x.h"
> +
> +static BootCertificateList *s390_get_boot_certs(void)
> +{
> +    return S390_CCW_MACHINE(qdev_get_machine())->boot_certs;
> +}
> +
> +static size_t cert2buf(char *path, char **cert_buf)
> +{
> +    size_t size;
> +
> +    if (!g_file_get_contents(path, cert_buf, &size, NULL)) {
> +        return 0;
> +    }
> +
> +    return size;
> +}

What's the advantage of these wrapper functions? You seem to only use them 
in one spot, so I'd rather inline the code where it's used ==> less lines of 
code in total, and easier to read.


> +static void update_cert_store(S390IPLCertificateStore *cert_store,
> +                              S390IPLCertificate *cert)
> +{
> +    size_t data_buf_size;
> +    size_t keyid_buf_size;
> +    size_t hash_buf_size;
> +    size_t cert_buf_size;
> +
> +    /* length field is word aligned for later DIAG use */
> +    keyid_buf_size = ROUND_UP(CERT_KEY_ID_LEN, 4);
> +    hash_buf_size = ROUND_UP(CERT_HASH_LEN, 4);
> +    cert_buf_size = ROUND_UP(cert->der_size, 4);
> +    data_buf_size = keyid_buf_size + hash_buf_size + cert_buf_size;
> +
> +    if (cert_store->max_cert_size < data_buf_size) {
> +        cert_store->max_cert_size = data_buf_size;
> +    }
> +
> +    cert_store->certs[cert_store->count] = *cert;
> +    cert_store->total_bytes += data_buf_size;
> +    cert_store->count++;
> +}
> +
> +static GPtrArray *get_cert_paths(void)
> +{
> +    BootCertificateList *path_list = NULL;
> +    BootCertificateList *list = NULL;
> +    gchar *cert_path;
> +    GDir *dir = NULL;
> +    const gchar *filename;
> +    g_autoptr(GError) err = NULL;
> +    g_autoptr(GPtrArray) cert_path_builder = g_ptr_array_new_full(0, g_free);
> +
> +    path_list = s390_get_boot_certs();
> +    if (path_list == NULL) {
> +        return g_steal_pointer(&cert_path_builder);
> +    }
> +
> +    for (list = path_list; list; list = list->next) {
> +        cert_path = list->value->path;
> +
> +        if (g_strcmp0(cert_path, "") == 0) {
> +            error_report("Empty path in certificate path list is not allowed");
> +            goto fail;
> +        }
> +
> +        struct stat st;

QEMU coding style (see docs/devel/style.rst):

"Mixed declarations (interleaving statements and declarations within
blocks) are generally not allowed; declarations should be at the beginning
of blocks."

> +        if (stat(cert_path, &st) != 0) {
> +            error_report("Failed to stat path '%s': %s", cert_path, g_strerror(errno));
> +            goto fail;
> +        }
> +
> +        if (S_ISREG(st.st_mode)) {
> +            if (!g_str_has_suffix(cert_path, ".pem")) {
> +                error_report("Certificate file '%s' must have a .pem extension",
> +                             cert_path);
> +                goto fail;
> +            }
> +
> +            g_ptr_array_add(cert_path_builder, g_strdup(cert_path));
> +        } else if (S_ISDIR(st.st_mode)) {
> +            dir = g_dir_open(cert_path, 0, &err);
> +            if (dir == NULL) {
> +                error_report("Failed to open directory '%s': %s",
> +                             cert_path, err->message);
> +                goto fail;
> +            }
> +
> +            while ((filename = g_dir_read_name(dir))) {
> +                if (g_str_has_suffix(filename, ".pem")) {
> +                    g_ptr_array_add(cert_path_builder,
> +                                    g_build_filename(cert_path, filename, NULL));
> +                }
> +            }
> +
> +            g_dir_close(dir);
> +        } else {
> +            error_report("Path '%s' is neither a file nor a directory", cert_path);
> +            goto fail;
> +        }
> +    }
> +
> +    qapi_free_BootCertificateList(path_list);
> +    return g_steal_pointer(&cert_path_builder);
> +
> +fail:
> +    qapi_free_BootCertificateList(path_list);
> +    exit(1);

Doing "goto fail"s and then a clean-up and exit(1) at the end of the 
function looks weird. I think I'd rather either use "exit(1)" instead of 
"goto fail" thorough the function, or use a return NULL here and do the 
exit(1) in the caller instead (and maybe pass an "Error **errp" to the 
function here, replace all error_report with error_setg and do the 
error_report in the caller instead). Otherwise this looks just unbalanced.

> +}
> +
> +void s390_ipl_create_cert_store(S390IPLCertificateStore *cert_store)
> +{
> +    GPtrArray *cert_path_builder;
> +
> +    cert_path_builder = get_cert_paths();
> +    if (cert_path_builder->len == 0) {
> +        g_ptr_array_free(cert_path_builder, TRUE);
> +        return;
> +    }
> +
> +    if (cert_path_builder->len > MAX_CERTIFICATES - 1) {
> +        error_report("Cert store exceeds maximum of %d certificates", MAX_CERTIFICATES);
> +        g_ptr_array_free(cert_path_builder, TRUE);
> +        exit(1);

So there's a exit(1) here already ... that's another indication that it 
would be nicer to have the exit(1) from get_cert_paths() in this function 
here, too.

> +    }
> +
> +    cert_store->max_cert_size = 0;
> +    cert_store->total_bytes = 0;
> +
> +    for (int i = 0; i < cert_path_builder->len; i++) {
> +        S390IPLCertificate *cert = init_cert((char *) cert_path_builder->pdata[i]);

I'd suggest to do the same with init_cert(), i.e. add an "Error **errp" to 
it's parameter list and do the error_report() here.

> +        if (!cert) {
> +            g_ptr_array_free(cert_path_builder, TRUE);
> +            exit(1);
> +        }
> +
> +        update_cert_store(cert_store, cert);
> +    }
> +
> +    g_ptr_array_free(cert_path_builder, TRUE);
> +}

  Thomas