Re: [PATCH v5 04/29] hw/s390x/ipl: Create certificate store

[Collin Walling <walling@linux.ibm.com>]

On 8/18/25 17:42, 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>
> ---
>  hw/s390x/cert-store.c       | 201 ++++++++++++++++++++++++++++++++++++
>  hw/s390x/cert-store.h       |  38 +++++++
>  hw/s390x/ipl.c              |   9 ++
>  hw/s390x/ipl.h              |   3 +
>  hw/s390x/meson.build        |   1 +
>  include/hw/s390x/ipl/qipl.h |   2 +
>  6 files changed, 254 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..81e748a912
> --- /dev/null
> +++ b/hw/s390x/cert-store.c
> @@ -0,0 +1,201 @@
> +/*
> + * 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 BootCertPathList *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) || size == 0) {
> +        return 0;
> +    }
> +
> +    return size;
> +}

Seems redundant to check if size == 0 here.  If g_file_get_contents
succeeds and size is set to 0, there's no difference between returning 0
or returning size.

> +
> +static S390IPLCertificate *init_cert_x509(size_t size, uint8_t *raw, Error **errp)
> +{
> +    S390IPLCertificate *q_cert = NULL;

I think the term "q_cert" is from an older implementation when we called
this a "QEMUCertStore".  Since we're doing this for s390 right now, I
think it makes sense to just to call it "cert".

> +    g_autofree uint8_t *cert_der = NULL;
> +    size_t der_len = size;
> +    int rc;
> +
> +    rc = qcrypto_x509_convert_cert_der(raw, size, &cert_der, &der_len, errp);
> +    if (rc != 0) {
> +        return NULL;
> +    }
> +
> +    q_cert = g_new0(S390IPLCertificate, 1);
> +    q_cert->size = size;
> +    q_cert->der_size = der_len;
> +    q_cert->key_id_size = QCRYPTO_HASH_DIGEST_LEN_SHA256;
> +    q_cert->hash_size = QCRYPTO_HASH_DIGEST_LEN_SHA256;

Setting the sizes here doesn't seem right:
1. it's making an assumption about the cert's data, and
2. these fields are never accessed in any subsequent patch

Patch 9 will explicitly use this enum when retrieving the cert and
fingerprint, but this data is neither checked nor extracted from the
S390IPLCertificate var.

I think it makes sense to get rid of the above two lines and potentially
the two S390IPLCertificate fields as well.

> +    q_cert->raw = raw;
> +
> +    return q_cert;
> +}
> +
> +static S390IPLCertificate *init_cert(char *path)
> +{
> +    char *buf;
> +    size_t size;
> +    char vc_name[VC_NAME_LEN_BYTES];
> +    g_autofree gchar *filename = NULL;
> +    S390IPLCertificate *qcert = NULL;
> +    Error *local_err = NULL;
> +
> +    filename = g_path_get_basename(path);
> +
> +    size = cert2buf(path, &buf);
> +    if (size == 0) {
> +        error_report("Failed to load certificate: %s", path);
> +        return NULL;
> +    }
> +
> +    qcert = init_cert_x509(size, (uint8_t *)buf, &local_err);
> +    if (qcert == NULL) {
> +        error_reportf_err(local_err, "Failed to initialize certificate: %s:  ", path);
> +        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, ' ');
> +    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->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] = *qcert;
> +    cert_store->total_bytes += data_buf_size;
> +    cert_store->count++;
> +}
> +
> +static GPtrArray *get_cert_paths(void)
> +{
> +    BootCertPathList *path_list = NULL;
> +    BootCertPathList *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");
> +            exit(1);
> +        }
> +
> +        struct stat st;
> +        if (stat(cert_path, &st) != 0) {
> +            error_report("Failed to stat path '%s': %s", cert_path, g_strerror(errno));
> +            exit(1);
> +        }
> +
> +        if (S_ISREG(st.st_mode)) {
> +            if (g_str_has_suffix(cert_path, ".pem")) {
> +                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);
> +                exit(1);
> +            }
> +
> +            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);
> +        }
> +    }
> +
> +    qapi_free_BootCertPathList(path_list);
> +    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);
> +        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) {
> +            error_report("Maximum %d certificates are allowed", MAX_CERTIFICATES);

nit: reword to "Cert store exceeds maximum of %d certificates"

> +            exit(1);
> +        }
> +
> +        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);
> +}
> diff --git a/hw/s390x/cert-store.h b/hw/s390x/cert-store.h
> new file mode 100644
> index 0000000000..f030c8846c
> --- /dev/null
> +++ b/hw/s390x/cert-store.h
> @@ -0,0 +1,38 @@
> +/*
> + * S390 certificate store
> + *
> + * Copyright 2025 IBM Corp.
> + * Author(s): Zhuoying Cai <zycai@linux.ibm.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef HW_S390_CERT_STORE_H
> +#define HW_S390_CERT_STORE_H
> +
> +#include "hw/s390x/ipl/qipl.h"
> +#include "crypto/x509-utils.h"
> +
> +#define VC_NAME_LEN_BYTES  64
> +
> +struct S390IPLCertificate {
> +    uint8_t vc_name[VC_NAME_LEN_BYTES];
> +    size_t  size;
> +    size_t  der_size;
> +    size_t  key_id_size;
> +    size_t  hash_size;

From above: I don't think these two fields are needed?

> +    uint8_t *raw;
> +};
> +typedef struct S390IPLCertificate S390IPLCertificate;
> +
> +struct S390IPLCertificateStore {
> +    uint16_t count;
> +    size_t   max_cert_size;
> +    size_t   total_bytes;
> +    S390IPLCertificate certs[MAX_CERTIFICATES];
> +} QEMU_PACKED;
> +typedef struct S390IPLCertificateStore S390IPLCertificateStore;
> +
> +void s390_ipl_create_cert_store(S390IPLCertificateStore *cert_store);
> +
> +#endif
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 2f082396c7..186be923d7 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -35,6 +35,7 @@
>  #include "qemu/option.h"
>  #include "qemu/ctype.h"
>  #include "standard-headers/linux/virtio_ids.h"
> +#include "cert-store.h"
>  
>  #define KERN_IMAGE_START                0x010000UL
>  #define LINUX_MAGIC_ADDR                0x010008UL
> @@ -422,6 +423,13 @@ void s390_ipl_convert_loadparm(char *ascii_lp, uint8_t *ebcdic_lp)
>      }
>  }
>  
> +S390IPLCertificateStore *s390_ipl_get_certificate_store(void)
> +{
> +    S390IPLState *ipl = get_ipl_device();
> +
> +    return &ipl->cert_store;
> +}
> +
>  static bool s390_build_iplb(DeviceState *dev_st, IplParameterBlock *iplb)
>  {
>      CcwDevice *ccw_dev = NULL;
> @@ -717,6 +725,7 @@ void s390_ipl_prepare_cpu(S390CPU *cpu)
>  
>      if (!ipl->kernel || ipl->iplb_valid) {
>          cpu->env.psw.addr = ipl->bios_start_addr;
> +        s390_ipl_create_cert_store(&ipl->cert_store);
>          if (!ipl->iplb_valid) {
>              ipl->iplb_valid = s390_init_all_iplbs(ipl);
>          } else {
> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> index 8f83c7da29..bee72dfbb3 100644
> --- a/hw/s390x/ipl.h
> +++ b/hw/s390x/ipl.h
> @@ -13,6 +13,7 @@
>  #ifndef HW_S390_IPL_H
>  #define HW_S390_IPL_H
>  
> +#include "cert-store.h"
>  #include "cpu.h"
>  #include "exec/target_page.h"
>  #include "system/address-spaces.h"
> @@ -35,6 +36,7 @@ int s390_ipl_pv_unpack(struct S390PVResponse *pv_resp);
>  void s390_ipl_prepare_cpu(S390CPU *cpu);
>  IplParameterBlock *s390_ipl_get_iplb(void);
>  IplParameterBlock *s390_ipl_get_iplb_pv(void);
> +S390IPLCertificateStore *s390_ipl_get_certificate_store(void);
>  
>  enum s390_reset {
>      /* default is a reset not triggered by a CPU e.g. issued by QMP */
> @@ -64,6 +66,7 @@ struct S390IPLState {
>      IplParameterBlock iplb;
>      IplParameterBlock iplb_pv;
>      QemuIplParameters qipl;
> +    S390IPLCertificateStore cert_store;
>      uint64_t start_addr;
>      uint64_t compat_start_addr;
>      uint64_t bios_start_addr;
> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
> index 8866012ddc..80d3d4a74d 100644
> --- a/hw/s390x/meson.build
> +++ b/hw/s390x/meson.build
> @@ -17,6 +17,7 @@ s390x_ss.add(files(
>    'sclpcpu.c',
>    'sclpquiesce.c',
>    'tod.c',
> +  'cert-store.c',
>  ))
>  s390x_ss.add(when: 'CONFIG_KVM', if_true: files(
>    'tod-kvm.c',
> diff --git a/include/hw/s390x/ipl/qipl.h b/include/hw/s390x/ipl/qipl.h
> index 6824391111..e505f44020 100644
> --- a/include/hw/s390x/ipl/qipl.h
> +++ b/include/hw/s390x/ipl/qipl.h
> @@ -20,6 +20,8 @@
>  #define LOADPARM_LEN    8
>  #define NO_LOADPARM "\0\0\0\0\0\0\0\0"
>  
> +#define MAX_CERTIFICATES  64
> +
>  /*
>   * The QEMU IPL Parameters will be stored at absolute address
>   * 204 (0xcc) which means it is 32-bit word aligned but not


-- 
Regards,
  Collin