On 5/5/26 16:18, 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.
Let's add more context here: "The raw Base64 data is stored, as well as the certificate's size. The binary (DER) size is stored as well, which may later be utilized for secure boot (signature verification)." > > Signed-off-by: Zhuoying Cai <[email protected]> > Reviewed-by: Farhan Ali<[email protected]> > --- > docs/specs/index.rst | 1 + > docs/specs/s390x-secure-ipl.rst | 16 +++ > hw/s390x/cert-store.c | 221 ++++++++++++++++++++++++++++++++ > hw/s390x/cert-store.h | 39 ++++++ > hw/s390x/ipl.c | 10 ++ > hw/s390x/ipl.h | 3 + > hw/s390x/meson.build | 1 + > include/hw/s390x/ipl/qipl.h | 2 + > 8 files changed, 293 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/index.rst b/docs/specs/index.rst > index b7909a108a..76d439782c 100644 > --- a/docs/specs/index.rst > +++ b/docs/specs/index.rst > @@ -40,3 +40,4 @@ guest hardware that is specific to QEMU. > riscv-aia > aspeed-intc > iommu-testdev > + s390x-secure-ipl > diff --git a/docs/specs/s390x-secure-ipl.rst b/docs/specs/s390x-secure-ipl.rst > new file mode 100644 > index 0000000000..1cdf19783f > --- /dev/null > +++ b/docs/specs/s390x-secure-ipl.rst > @@ -0,0 +1,16 @@ > +.. 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. Add: "Each certificate is stroed in an S390IPLCertificate struct, which has a name (converted to EBCDIC), size fields of PEM and DER data, and the raw PEM Base64 data." > + > +Note: A maximum of 64 certificates are allowed to be stored in the > certificate > +store. > diff --git a/hw/s390x/cert-store.c b/hw/s390x/cert-store.c > new file mode 100644 > index 0000000000..a4f15627e9 > --- /dev/null > +++ b/hw/s390x/cert-store.c > @@ -0,0 +1,221 @@ > +/* > + * S390 certificate store implementation > + * > + * Copyright 2025 IBM Corp. > + * Author(s): Zhuoying Cai <[email protected]> > + * > + * 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 BootCertificatesList *s390_get_boot_certs(void) > +{ > + return S390_CCW_MACHINE(qdev_get_machine())->boot_certs; > +} > + > +static S390IPLCertificate *init_cert(char *path, Error **errp) > +{ > + int rc; > + char *buf; What about g_autofree for buf... > + size_t size; > + size_t der_len; > + char name[CERT_NAME_MAX_LEN]; > + g_autofree gchar *filename = NULL; > + S390IPLCertificate *cert = NULL; > + g_autofree uint8_t *cert_der = NULL; > + Error *local_err = NULL; > + > + filename = g_path_get_basename(path); > + > + if (!g_file_get_contents(path, &buf, &size, NULL)) { > + error_setg(errp, "Failed to load certificate: %s", path); > + return NULL; > + } > + > + rc = qcrypto_x509_convert_cert_der((uint8_t *)buf, size, > + &cert_der, &der_len, &local_err); > + if (rc != 0) { > + error_propagate_prepend(errp, local_err, > + "Failed to initialize certificate: %s: ", > path); > + g_free(buf); ...this could go away... > + return NULL; > + } > + > + cert = g_new0(S390IPLCertificate, 1); > + cert->size = size; > + /* > + * Store DER length only - reused for size calculation. > + * cert_der is discarded because DER certificate data will be used once > + * and can be regenerated from cert->raw. > + */ > + cert->der_size = der_len; > + /* store raw pointer - ownership transfers to cert */ > + cert->raw = (uint8_t *)buf; ...then use g_steal_pointer. It'll read better and the comment wouldn't be needed to explain what's happening. > + > + /* > + * Left justified certificate name with padding on the right with blanks. > + * Convert certificate name to EBCDIC. > + */ > + strpadcpy(name, CERT_NAME_MAX_LEN, filename, ' '); > + ebcdic_put(cert->name, name, CERT_NAME_MAX_LEN); > + > + return cert; > +} > + > +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->largest_cert_size < data_buf_size) { > + cert_store->largest_cert_size = data_buf_size; > + } > + > + g_assert(cert_store->count < MAX_CERTIFICATES); > + > + cert_store->certs[cert_store->count] = *cert; Should a g_steal_pointer be used here? > + cert_store->total_bytes += data_buf_size; > + cert_store->count++; > +} > + Functionally, things look good to me. -- Regards, Collin
