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

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

On 9/2/25 11:15 AM, Jared Rossi wrote:
> 
> 
> On 8/28/25 10:46 AM, Zhuoying Cai wrote:
>> On 8/27/25 7:14 PM, Farhan Ali wrote:
>>> [snip...]
>>>
>>> +
>>> +static S390IPLCertificate *init_cert_x509(size_t size, uint8_t *raw, Error **errp)
>>> +{
>>> +    S390IPLCertificate *q_cert = NULL;
>>> +    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;
>>> +    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++;
>>> +}
>>> +
>>> Do we need to free qcert here after we copied the contents to
>>> cert_store->certs[cert_store->count]? Also AFAIU we copy the certificate
>>> file contents into QEMU memory, but don't free it. Just wanted to
>>> clarify, do we need the file contents in QEMU memory for ccw-bios and
>>> guest kernel use? Thanks Farhan
>>>
>>>
>> Yes, the file contents need to remain in QEMU memory for both ccw-bios
>> and guest kernel use.
>>
>> * ccw-bios: The certificate used to verify the component is retrieved in
>> the BIOS via DIAG320, with its address stored in the IPL Information
>> Report Block.
>>
>> * guest kernel: Certificates can also be retrieved via DIAG320 and made
>> available to the guest kernel keyring.
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8cf57d7217c32133d25615324c0ab4aaacf4d9c4
> Something still doesn't seem right to me.  As far as I can tell the cert 
> store will be reinitialized each time the guest reboots, which sounds 
> like it will cause problems if there is no corresponding free somewhere.
> 
> What is the expected behavior during a reboot?  Should the guest A)  
> parse the cert paths again and reinitialize, or B)  read the entries in 
> the previously created cert store?
> 
> If A, then the cert store needs to be cleared/freed in some way each time.
> 
> If B, then the cert store should not be reinitialized.
> 
>> [snip...]
> Regards,
> Jared Rossi

I think option B sounds more accurate. Since the cert store can't be
modified during reboot, it's reasonable to read from the previously
created cert store.