Re: [PATCH v3 03/28] hw/s390x/ipl: Create certificate store

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Jun 04, 2025 at 05:56:31PM -0400, Zhuoying Cai wrote:
> Create a certificate store for boot certificates used for secure IPL.
> 
> Load certificates from the boot-certificate parameter of s390-ccw-virtio
> machine type option into the cert store.
> 
> Currently, only x509 certificates in DER format and uses SHA-256 hashing
> algorithm are supported, as these are the types required for secure boot
> on s390.
> 
> Signed-off-by: Zhuoying Cai <zycai@linux.ibm.com>
> ---
>  hw/s390x/cert-store.c       | 247 ++++++++++++++++++++++++++++++++++++
>  hw/s390x/cert-store.h       |  39 ++++++
>  hw/s390x/ipl.c              |   9 ++
>  hw/s390x/ipl.h              |   3 +
>  hw/s390x/meson.build        |   1 +
>  include/hw/s390x/ipl/qipl.h |   3 +
>  6 files changed, 302 insertions(+)
>  create mode 100644 hw/s390x/cert-store.c
>  create mode 100644 hw/s390x/cert-store.h
> 
> diff --git a/hw/s390x/cert-store.c b/hw/s390x/cert-store.c
> new file mode 100644
> index 0000000000..562fa22241
> --- /dev/null
> +++ b/hw/s390x/cert-store.c
> @@ -0,0 +1,247 @@
> +/*
> + * 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 "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"
> +
> +static const char *s390_get_boot_certificates(void)
> +{
> +    return S390_CCW_MACHINE(qdev_get_machine())->boot_certificates;
> +}
> +
> +static size_t cert2buf(char *path, size_t max_size, char **cert_buf)
> +{
> +    size_t size;
> +
> +    /*
> +     * maximum allowed size of the certificate file to avoid consuming excessive memory
> +     * (malformed or maliciously large files)
> +     */
> +    if (!g_file_get_contents(path, cert_buf, &size, NULL) ||
> +        size == 0 || size > max_size) {

By the time this 'size > max_size' check is performed, the file
is already loaded into memory, which is fairly pointless. In
existing code loading certs we don't enforce any max size. This
data comes from the host admin who QEMU has to assume is trusted,
so the size check is not required.

> +        return 0;
> +    }
> +
> +    return size;
> +}
> +
> +static S390IPLCertificate *init_cert_x509_der(size_t size, char *raw)
> +{
> +    S390IPLCertificate *q_cert = NULL;
> +    int key_id_size;
> +    int hash_size;
> +    int is_der;
> +    int hash_type;
> +    Error *err = NULL;
> +
> +    is_der = qcrypto_check_x509_cert_fmt((uint8_t *)raw, size,
> +                                         QCRYPTO_CERT_FMT_DER, &err);
> +    /* return early if GNUTLS is not enabled */
> +    if (is_der == -ENOTSUP) {
> +        error_report("GNUTLS is not enabled");
> +        return NULL;
> +    }
> +    if (is_der != 0) {
> +        error_report("The certificate is not in DER format");
> +        return NULL;
> +    }

As mentioned in the other patch, we should exclusively accept PEM
format as the public interface. If we need DER format internllay,
gnutls_x509_cert_export can convert it into DER for us.

> +
> +    hash_type = qcrypto_get_x509_signature_algorithm((uint8_t *)raw, size, &err);
> +    if (hash_type != QCRYPTO_SIG_ALGO_RSA_SHA256) {
> +        error_report("The certificate does not use SHA-256 hashing");
> +        return NULL;
> +    }
> +
> +    key_id_size = qcrypto_get_x509_keyid_len(QCRYPTO_KEYID_FLAGS_SHA256);
> +    if (key_id_size == 0) {
> +        error_report("Failed to get certificate key ID size");
> +        return NULL;
> +    }
> +
> +    hash_size = qcrypto_get_x509_hash_len(QCRYPTO_HASH_ALGO_SHA256);
> +    if (hash_size == 0) {
> +        error_report("Failed to get certificate hash size");
> +        return NULL;
> +    }

Pointless method call when we have a QCRYPTO_HASH_DIGEST_LEN_SHA256
constant.

> +
> +    q_cert = g_new(S390IPLCertificate, 1);

g_new0 to guarantee zero initialization of all fields.

> +    q_cert->size = size;
> +    q_cert->key_id_size = key_id_size;
> +    q_cert->hash_size = hash_size;
> +    q_cert->raw = raw;
> +    q_cert->format = QCRYPTO_CERT_FMT_DER;
> +    q_cert->hash_type = QCRYPTO_SIG_ALGO_RSA_SHA256;
> +
> +    return q_cert;
> +}
> +
> +static int check_path_type(const char *path)
> +{
> +    struct stat path_stat;
> +
> +    if (stat(path, &path_stat) != 0) {
> +        perror("stat");
> +        return -1;
> +    }
> +
> +    if (S_ISDIR(path_stat.st_mode)) {
> +        return S_IFDIR;
> +    } else if (S_ISREG(path_stat.st_mode)) {
> +        return S_IFREG;
> +    } else {
> +        return -1;
> +    }
> +}

This helper isn't making the code any simpler - use the
stat() & S_ISXXX checks inline where needed.

> +static S390IPLCertificate *init_cert(char *paths)

s/paths/path/ as this is a single file being used.

> +{
> +    char *buf;
> +    char vc_name[VC_NAME_LEN_BYTES];
> +    g_autofree gchar *filename;

All g_autofree variables *must* be initialized at time
of declaration. 

> +    size_t size;
> +    S390IPLCertificate *qcert = NULL;
> +
> +    filename = g_path_get_basename(paths);
> +
> +    size = cert2buf(paths, CERT_MAX_SIZE, &buf);
> +    if (size == 0) {
> +        error_report("Failed to load certificate: %s", paths);
> +        g_free(buf);
> +        return NULL;
> +    }
> +
> +    qcert = init_cert_x509_der(size, buf);
> +    if (qcert == NULL) {
> +        error_report("Failed to initialize certificate: %s", paths);
> +        g_free(buf);
> +        return NULL;
> +    }
> +
> +    /*
> +     * Left justified certificate name with padding on the right with blanks.
> +     * Convert certificate name to EBCDIC.
> +     */
> +    strpadcpy(vc_name, VC_NAME_LEN_BYTES, filename, ' ');

What purpose does the 'vc_name' serve ? Are there expectations
on the user for naming of the cert fiels ?

> +    ebcdic_put(qcert->vc_name, vc_name, VC_NAME_LEN_BYTES);
> +
> +    return qcert;
> +}
> +
> +static void update_cert_store(S390IPLCertificateStore *cert_store,
> +                              S390IPLCertificate *qcert)
> +{
> +    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(qcert->key_id_size, 4);
> +    hash_buf_size = ROUND_UP(qcert->hash_size, 4);
> +    cert_buf_size = ROUND_UP(qcert->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] = *qcert;
> +    cert_store->total_bytes += data_buf_size;
> +    cert_store->count++;
> +}
> +
> +static GPtrArray *get_cert_paths(void)
> +{
> +    const char *path;
> +    gchar **paths;

g_auto(GStrv) paths = NULL;

to automatically free this

> +    gchar **paths_copy;

and use this as the working variable

> +    int path_type;
> +    GDir *dir = NULL;
> +    gchar *cert_path;
> +    const gchar *filename;
> +    GPtrArray *cert_path_builder;
> +
> +    cert_path_builder = g_ptr_array_new();

g_autoptr(GPtrArray) cert_path_builder = g_ptr_array_new_fill(0, g_free)

so that this get freed automatically in error paths, along
with any paths stored within it.

> +
> +    path = s390_get_boot_certificates();
> +    if (path == NULL) {
> +        return cert_path_builder;
> +    }

Calling this variable 'path' is misleading given it is
a list of paths.

> +
> +    paths = g_strsplit(path, ":", -1);
> +    /* save the original pointer for freeing later */
> +    paths_copy = paths;
> +    while (*paths) {
> +        /* skip empty certificate path */
> +        if (!strcmp(*paths, "")) {

IMHO this should be reported as a fatal user configuration
error.

> +            paths += 1;
> +            continue;
> +        }
> +
> +        cert_path = NULL;
> +        path_type = check_path_type(*paths);

Just call stat() directly here, and report a fatal error
if returns non-zero instead of ignoring the error

> +        if (path_type == S_IFREG) {
> +            cert_path = g_strdup(*paths);
> +            g_ptr_array_add(cert_path_builder, cert_path);

Drop the intermediate cert_path variable as it
serves no purpose

> +        } else if (path_type == S_IFDIR) {
> +            dir = g_dir_open(*paths, 0, NULL);

Pass an "GError" object not NULL and treat failure as
fatal.

> +
> +            if (dir) {
> +                while ((filename = g_dir_read_name(dir))) {
> +                    cert_path = g_build_filename(*paths, filename, NULL);
> +                    g_ptr_array_add(cert_path_builder, (gpointer) cert_path);
> +                }

The (gpointer) cast serves no purpose.

> +
> +                g_dir_close(dir);
> +            }
> +        }


> +
> +        paths += 1;
> +    }
> +
> +    g_strfreev(paths_copy);

Redundant when using g_auto

> +    return cert_path_builder;

return g_steal_pointer(&cert_path_builder)


> +}
> +
> +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);

s/true/TRUE/ - GLib does not accept stdbool constants

> +        return;
> +    }
> +
> +    cert_store->max_cert_size = 0;
> +    cert_store->total_bytes = 0;
> +
> +    for (int i = 0; i < cert_path_builder->len; i++) {
> +        if (i > MAX_CERTIFICATES - 1) {
> +            printf("Warning: Maximum %d certificates are allowed,"
> +                            " ignoring certificate #%d and beyond\n",
> +                            MAX_CERTIFICATES, i + 1);

Is this limit mandated by some s390x hardware design spec ?

If so, this should be a fatal error, not a warning.

If not, then it would be better to accept all the user
provided certs, no matter how many.

> +            break;
> +        }



> +
> +        S390IPLCertificate *qcert = init_cert((char *) cert_path_builder->pdata[i]);
> +        if (qcert) {
> +            update_cert_store(cert_store, qcert);
> +        }
> +    }
> +
> +    g_ptr_array_free(cert_path_builder, true);
> +}

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|