Re: [PATCH v1 02/24] hw/s390x/ipl: Create certificate store

[Thomas Huth <thuth@redhat.com>]

On 08/04/2025 17.55, Zhuoying Cai wrote:
> Create a certificate store for boot certificates used for secure IPL.
> 
> Load certificates from the -boot-certificate 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>
> ---
...
> +static size_t cert2buf(char *path, size_t max_size, char **cert_buf)
> +{
> +    size_t size;
> +    g_autofree char *buf;
> +    buf = g_malloc(max_size);
> +
> +    if (!g_file_get_contents(path, &buf, &size, NULL) ||
> +        size == 0 || size > max_size) {
> +        return 0;
> +    }
> +
> +    *cert_buf = g_steal_pointer(&buf);
> +
> +    return size;
> +}

This function looks quite wrong to me. Why is there a g_malloc() in here if 
g_file_get_contents() already allocates the memory?

And why do we need a max_size here? If there is a reason, please add a 
proper comment in the source code.

> +#ifdef CONFIG_GNUTLS
> +int g_init_cert(uint8_t *raw_cert, size_t cert_size, gnutls_x509_crt_t *g_cert)

Please don't use a "g_" prefix here - otherwise that way the function could 
be confused with the functions from the glib.

> +{
> +    int rc;
> +
> +    if (gnutls_x509_crt_init(g_cert) < 0) {
> +        return -1;
> +    }
> +
> +    gnutls_datum_t datum_cert = {raw_cert, cert_size};
> +    rc = gnutls_x509_crt_import(*g_cert, &datum_cert, GNUTLS_X509_FMT_DER);
> +    if (rc) {
> +        gnutls_x509_crt_deinit(*g_cert);
> +        return rc;
> +    }
> +
> +    return 0;
> +}
> +#endif /* CONFIG_GNUTLS */
> +
> +static int init_cert_x509_der(size_t size, char *raw, S390IPLCertificate **qcert)

I'd maybe rather use "S390IPLCertificate *" as return type instead of "int" 
and return a NULL in case of errors.

> +{
> +#ifdef CONFIG_GNUTLS
> +    gnutls_x509_crt_t g_cert = NULL;
> +    g_autofree S390IPLCertificate *q_cert;
> +    size_t key_id_size;
> +    size_t hash_size;
> +    int rc;
> +
> +    rc = g_init_cert((uint8_t *)raw, size, &g_cert);
> +    if (rc) {
> +        if (rc == GNUTLS_E_ASN1_TAG_ERROR) {
> +            error_report("The certificate is not in DER format");
> +        }
> +        return -1;
> +    }
> +
> +    rc = gnutls_x509_crt_get_key_id(g_cert, GNUTLS_KEYID_USE_SHA256, NULL, &key_id_size);

Is that documented somewhere that you can call gnutls_x509_crt_get_key_id() 
like this? The docs that I found about this function do not say anything 
about passing NULL here, they rather recommend to use a buffer of size 20 by 
default?

> +    if (rc != GNUTLS_E_SHORT_MEMORY_BUFFER) {
> +        error_report("Failed to get certificate key ID size");
> +        goto out;
> +    }
> +
> +    rc = gnutls_x509_crt_get_fingerprint(g_cert, GNUTLS_DIG_SHA256, NULL, &hash_size);

For this function, the NULL pointer handling is documented, so here it seems 
to be OK.

> +    if (rc != GNUTLS_E_SHORT_MEMORY_BUFFER) {
> +        error_report("Failed to get certificate hash size");
> +        goto out;
> +    }
> +
> +    q_cert = g_malloc(sizeof(*q_cert));

Please use g_new() for allocating memory for structures instead.

> +    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 = GNUTLS_X509_FMT_DER;
> +    *qcert = g_steal_pointer(&q_cert);

If there is no "return" between the allocation and the final "return 0", you 
can also drop the g_autofree and g_steal_pointer from this function.

> +    gnutls_x509_crt_deinit(g_cert);
> +
> +    return 0;
> +out:
> +    gnutls_x509_crt_deinit(g_cert);
> +    return -1;
> +#else
> +    error_report("Cryptographic library is not enabled")
> +    return -1;
> +#endif /* #define CONFIG_GNUTLS */
> +}
> +
> +static int check_path_type(const char *path)
> +{
> +    struct stat path_stat;
> +
> +    stat(path, &path_stat);
> +
> +    if (S_ISDIR(path_stat.st_mode)) {
> +        return S_IFDIR;
> +    } else if (S_ISREG(path_stat.st_mode)) {
> +        return S_IFREG;
> +    } else {
> +        return -1;
> +    }
> +}
> +
> +static int init_cert(char *paths, S390IPLCertificate **qcert)

as with previous function, use "S390IPLCertificate *" as return type instead 
of "int" ?

> +{
> +    char *buf;
> +    char vc_name[VC_NAME_LEN_BYTES];
> +    const gchar *filename;
> +    size_t size;
> +
> +    filename = g_path_get_basename(paths);

g_path_get_basename() returns an allocated string. You've finally got to 
free it again to avoid leaking memory. I'd suggest declaring filename with 
g_autofree.

> +    size = cert2buf(paths, CERT_MAX_SIZE, &buf);
> +    if (size == 0) {
> +        error_report("Failed to load certificate: %s", paths);
> +        return -1;
> +    }
> +
> +    if (init_cert_x509_der(size, buf, qcert) < 0) {
> +        error_report("Failed to initialize certificate: %s", paths);
> +        return -1;
> +    }
> +
> +    /*
> +     * 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 0;
> +}
> +
> +static void update_cert_store(S390IPLCertificateStore *cert_store,
> +                              S390IPLCertificate *qcert)
> +{
> +    size_t data_size;
> +
> +    data_size = qcert->size + qcert->key_id_size + qcert->hash_size;
> +
> +    if (cert_store->max_cert_size < data_size) {
> +        cert_store->max_cert_size = data_size;
> +    }
> +
> +    cert_store->certs[cert_store->count] = *qcert;
> +    cert_store->total_bytes += data_size;
> +    cert_store->count++;
> +}
> +
> +static GPtrArray *get_cert_paths(void)
> +{
> +    const char *path;
> +    gchar **paths;
> +    int path_type;
> +    GDir *dir = NULL;
> +    const gchar *filename;
> +    GPtrArray *cert_path_builder;
> +
> +    cert_path_builder = g_ptr_array_new();
> +
> +    path = s390_get_boot_certificates();
> +    if (path == NULL) {
> +        return cert_path_builder;
> +    }
> +
> +    paths = g_strsplit(path, ":", -1);

Free the memory that has been allocated by the g_strsplit at the end of the 
function?

> +    while (*paths) {
> +        /* skip empty certificate path */
> +        if (!strcmp(*paths, "")) {
> +            paths += 1;
> +            continue;
> +        }
> +
> +        path_type = check_path_type(*paths);
> +        if (path_type == S_IFREG) {
> +            g_ptr_array_add(cert_path_builder, (gpointer) *paths);

... that should likely g_strdup(*paths) when we want to free paths at the end.

> +        } else if (path_type == S_IFDIR) {
> +            dir = g_dir_open(*paths, 0, NULL);
> +
> +            while ((filename = g_dir_read_name(dir))) {
> +                gchar *cert_path = NULL;
> +                cert_path = g_build_filename(*paths, filename, NULL);
> +                g_ptr_array_add(cert_path_builder, (gpointer) cert_path);
> +            }
> +
> +            g_dir_close(dir);
> +        }
> +
> +        paths += 1;
> +    }
> +
> +    return 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++) {
> +        S390IPLCertificate *qcert = NULL;
> +        if (init_cert((char *) cert_path_builder->pdata[i], &qcert) < 0) {
> +            continue;

Maybe invert the logic to call update_cert_store() in case of success, than 
you don't need the "continue" anymore?

> +        }
> +
> +        update_cert_store(cert_store, qcert);
> +    }
> +
> +    g_ptr_array_free(cert_path_builder, true);
> +}

  Thomas