Re: [PATCH v6 19/28] pc-bios/s390-ccw: Add signature verification for secure IPL in audit mode

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

On 9/30/25 2:42 PM, Farhan Ali wrote:
> 
> On 9/17/2025 4:21 PM, Zhuoying Cai wrote:
>> Enable secure IPL in audit mode, which performs signature verification,
>> but any error does not terminate the boot process. Only warnings will be
>> logged to the console instead.
>>
>> Add a comp_len variable to store the length of a segment in
>> zipl_load_segment. comp_len variable is necessary to store the
>> calculated segment length and is used during signature verification.
>> Return the length on success, or a negative return code on failure.
>>
>> Secure IPL in audit mode requires at least one certificate provided in
>> the key store along with necessary facilities (Secure IPL Facility,
>> Certificate Store Facility and secure IPL extension support).
>>
>> Note: Secure IPL in audit mode is implemented for the SCSI scheme of
>> virtio-blk/virtio-scsi devices.
>>
>> Signed-off-by: Zhuoying Cai<zycai@linux.ibm.com>
>> ---
>>   docs/system/s390x/secure-ipl.rst |  36 +++
>>   pc-bios/s390-ccw/Makefile        |   3 +-
>>   pc-bios/s390-ccw/bootmap.c       |  39 +++-
>>   pc-bios/s390-ccw/bootmap.h       |  11 +
>>   pc-bios/s390-ccw/main.c          |   9 +
>>   pc-bios/s390-ccw/s390-ccw.h      |  15 ++
>>   pc-bios/s390-ccw/sclp.c          |  44 ++++
>>   pc-bios/s390-ccw/sclp.h          |   6 +
>>   pc-bios/s390-ccw/secure-ipl.c    | 371 +++++++++++++++++++++++++++++++
>>   pc-bios/s390-ccw/secure-ipl.h    |  99 +++++++++
>>   10 files changed, 630 insertions(+), 3 deletions(-)
>>   create mode 100644 pc-bios/s390-ccw/secure-ipl.c
>>   create mode 100644 pc-bios/s390-ccw/secure-ipl.h
>>

[...]

>> +
>> +static int handle_certificate(int *cert_table, uint8_t **cert,
>> +                             uint64_t cert_len, uint8_t cert_idx,
>> +                             IplSignatureCertificateList *certs, int cert_index)
>> +{
>> +    bool unused;
>> +
>> +    unused = cert_table[cert_idx] == -1;
>> +    if (unused) {
>> +        if (request_certificate(*cert, cert_idx)) {
>> +            cert_list_add(certs, cert_index, *cert, cert_len);
>> +            cert_table[cert_idx] = cert_index;
>> +            *cert += cert_len;
> 
> It's hard to understand why we increment *cert in this function by just 
> looking at the function. But this function is called in the loop in 
> zipl_run_secure, could we move this entire function in zipl_run_secure?
> 
> 

Thanks for the suggestion.

I'm concerned that moving this function into zipl_run_secure() could
make it harder to read, since it's already quite large. I'd prefer to
keep this function separate because it handles all certificate related
operations here and add comments to clarify why *cert is incremented.
I'd be happy to hear additional thoughts.

>> +        } else {
>> +            puts("Could not get certificate");
>> +            return -1;
>> +        }
>> +
>> +        /* increment cert_index for the next cert entry */
>> +        return ++cert_index;
>> +    }
>> +
>> +    return cert_index;
>> +}
>> +
>> +int zipl_run_secure(ComponentEntry **entry_ptr, uint8_t *tmp_sec)
>> +{
>> +    IplDeviceComponentList comps;
>> +    IplSignatureCertificateList certs;
>> +    ComponentEntry *entry = *entry_ptr;
>> +    uint8_t *cert = NULL;
>> +    uint64_t *sig = NULL;
>> +    int cert_index = 0;
>> +    int comp_index = 0;
>> +    uint64_t comp_addr;
>> +    int comp_len;
>> +    uint32_t sig_len = 0;
>> +    uint64_t cert_len = -1;
>> +    uint8_t cert_idx = -1;
> 
> Why do we have 2 variables (cert_idx, cert_index) that are named 
> similar? Can we rename cert_index to cert_table_idx?
> 
> 
>> +    bool verified;
>> +    uint32_t certs_len;
>> +    /*
>> +     * Store indices of cert entry that have already used for signature verification
>> +     * to prevent allocating the same certificate multiple times.
>> +     * cert_table index: index of certificate from qemu cert store used for verification
>> +     * cert_table value: index of cert entry in cert list that contains the certificate
>> +     */
>> +    int cert_table[MAX_CERTIFICATES] = { [0 ... MAX_CERTIFICATES - 1] = -1};
>> +    int signed_count = 0;
>> +
>> +    if (!secure_ipl_supported()) {
>> +        return -1;
>> +    }
>> +
>> +    init_lists(&comps, &certs);
>> +    certs_len = get_certs_length();
>> +    cert = malloc(certs_len);
>> +    sig = malloc(MAX_SECTOR_SIZE);
>> +
>> +    while (entry->component_type != ZIPL_COMP_ENTRY_EXEC) {
>> +        switch (entry->component_type) {
>> +        case ZIPL_COMP_ENTRY_SIGNATURE:
>> +            if (sig_len) {
>> +                goto out;
>> +            }
>> +
>> +            sig_len = zipl_load_signature(entry, (uint64_t)sig);
>> +            if (sig_len < 0) {
>> +                goto out;
>> +            }
>> +            break;
>> +        case ZIPL_COMP_ENTRY_LOAD:
>> +            comp_addr = entry->compdat.load_addr;
>> +            comp_len = zipl_load_segment(entry, comp_addr);
>> +            if (comp_len < 0) {
>> +                goto out;
>> +            }
>> +
>> +            if (!sig_len) {
>> +                break;
>> +            }
>> +
>> +            verified = verify_signature(comp_len, comp_addr, sig_len, (uint64_t)sig,
>> +                                        &cert_len, &cert_idx);
>> +
>> +            if (verified) {
>> +                cert_index = handle_certificate(cert_table, &cert, cert_len, cert_idx,
>> +                                                &certs, cert_index);
>> +                if (cert_index == -1) {
>> +                    goto out;
>> +                }
>> +
>> +                puts("Verified component");
>> +                comp_list_add(&comps, comp_index, cert_table[cert_idx],
>> +                              comp_addr, comp_len,
>> +                              S390_IPL_COMPONENT_FLAG_SC | S390_IPL_COMPONENT_FLAG_CSV);
>> +            } else {
>> +                comp_list_add(&comps, comp_index, -1,
>> +                              comp_addr, comp_len,
>> +                              S390_IPL_COMPONENT_FLAG_SC);
>> +                zipl_secure_handle("Could not verify component");
>> +            }
>> +
>> +            comp_index++;
>> +            signed_count += 1;
>> +            /* After a signature is used another new one can be accepted */
>> +            sig_len = 0;
>> +            break;
>> +        default:
>> +            puts("Unknown component entry type");
>> +            return -1;
>> +        }
>> +
>> +        entry++;
>> +
>> +        if ((uint8_t *)(&entry[1]) > tmp_sec + MAX_SECTOR_SIZE) {
>> +            puts("Wrong entry value");
>> +            return -EINVAL;
>> +        }
>> +    }
>> +
>> +    if (signed_count == 0) {
>> +        zipl_secure_handle("Secure boot is on, but components are not signed");
>> +    }
>> +
>> +    if (update_iirb(&comps, &certs)) {
>> +        zipl_secure_handle("Failed to write IPL Information Report Block");
>> +    }
>> +
>> +    *entry_ptr = entry;
>> +    free(sig);
>> +
>> +    return 0;
>> +out:
>> +    free(cert);
>> +    free(sig);
>> +
>> +    return -1;
>> +}

[...]