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

[Zhuoying Cai <zycai@linux.ibm.com>]

On 6/6/25 6:31 AM, Daniel P. Berrangé wrote:
> 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 ?
> 

vc_name refers to the name of the certificate, along with other
certificate metadata, which could be retrieved and displayed via the
DIAG320 invocation. This information is available to the Linux root user
through the cert_store keyring.

The certificate name is expected to consist of ASCII characters.

>> [snip..]