Re: [PATCH v2 17/25] pc-bios/s390-ccw: Add signature verification for secure IPL in audit mode

[Thomas Huth <thuth@redhat.com>]

On 09/05/2025 00.50, 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>
> ---
>   pc-bios/s390-ccw/Makefile     |   3 +-
>   pc-bios/s390-ccw/bootmap.c    | 192 +++++++++++++++++++++++++++++++++-
>   pc-bios/s390-ccw/bootmap.h    |   9 ++
>   pc-bios/s390-ccw/main.c       |   9 ++
>   pc-bios/s390-ccw/s390-ccw.h   |  14 +++
>   pc-bios/s390-ccw/sclp.c       |  44 ++++++++
>   pc-bios/s390-ccw/sclp.h       |   6 ++
>   pc-bios/s390-ccw/secure-ipl.c | 175 +++++++++++++++++++++++++++++++
>   pc-bios/s390-ccw/secure-ipl.h | 109 +++++++++++++++++++
>   9 files changed, 558 insertions(+), 3 deletions(-)
>   create mode 100644 pc-bios/s390-ccw/secure-ipl.c
>   create mode 100644 pc-bios/s390-ccw/secure-ipl.h
> 
> diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile
> index dc69dd484f..fedb89a387 100644
> --- a/pc-bios/s390-ccw/Makefile
> +++ b/pc-bios/s390-ccw/Makefile
> @@ -34,7 +34,8 @@ QEMU_DGFLAGS = -MMD -MP -MT $@ -MF $(@D)/$(*F).d
>   .PHONY : all clean build-all distclean
>   
>   OBJECTS = start.o main.o bootmap.o jump2ipl.o sclp.o menu.o netmain.o \
> -	  virtio.o virtio-net.o virtio-scsi.o virtio-blkdev.o cio.o dasd-ipl.o
> +	  virtio.o virtio-net.o virtio-scsi.o virtio-blkdev.o cio.o dasd-ipl.o \
> +	  secure-ipl.o
>   
>   SLOF_DIR := $(SRC_PATH)/../../roms/SLOF
>   
> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> index 3dd09fda7e..06cea0929a 100644
> --- a/pc-bios/s390-ccw/bootmap.c
> +++ b/pc-bios/s390-ccw/bootmap.c
> @@ -15,6 +15,7 @@
>   #include "bootmap.h"
>   #include "virtio.h"
>   #include "bswap.h"
> +#include "secure-ipl.h"
>   
>   #ifdef DEBUG
>   /* #define DEBUG_FALLBACK */
> @@ -34,6 +35,13 @@ static uint8_t sec[MAX_SECTOR_SIZE*4] __attribute__((__aligned__(PAGE_SIZE)));
>   const uint8_t el_torito_magic[] = "EL TORITO SPECIFICATION"
>                                     "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0";
>   
> +/* sector for storing certificates */
> +static uint8_t certs_sec[CERT_MAX_SIZE * MAX_CERTIFICATES];

If I calculated correctly, that's a buffer of 512 kB... That's quite huge 
already. Would it be possible to malloc() it only if we really need this 
instead of statically allocating it?

> +/* sector for storing signatures */
> +static uint8_t sig_sec[MAX_SECTOR_SIZE] __attribute__((__aligned__(PAGE_SIZE)));
> +
> +ipl_print_func_t zipl_secure_print_func;
> +
>   /*
>    * Match two CCWs located after PSW and eight filler bytes.
>    * From libmagic and arch/s390/kernel/head.S.
> @@ -676,6 +684,155 @@ static int zipl_load_segment(ComponentEntry *entry, uint64_t address)
>       return comp_len;
>   }
>   
> +static uint32_t zipl_handle_sig_entry(ComponentEntry *entry)
> +{
> +    uint32_t sig_len;
> +
> +    if (zipl_load_segment(entry, (uint64_t)sig_sec) < 0) {
> +        return -1;
> +    }
> +
> +    if (entry->compdat.sig_info.format != DER_SIGNATURE_FORMAT) {
> +        puts("Signature is not in DER format");
> +        return -1;
> +    }
> +    sig_len = entry->compdat.sig_info.sig_len;
> +
> +    return sig_len;
> +}
> +
> +static int handle_certificate(int *cert_table, uint64_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 (zipl_secure_request_certificate(*cert, cert_idx)) {
> +            zipl_secure_cert_list_add(certs, cert_index, *cert, cert_len);
> +            cert_table[cert_idx] = cert_index;
> +            *cert += cert_len;

So zipl_secure_cert_list_add() checks for the index not going beyond 
MAX_CERTIFICATES, but here you ignore that error and update cert_table and 
*cert anyway? Sounds like a potential bug to me.

> +        } else {
> +            puts("Could not get certificate");
> +            return -1;
> +        }
> +
> +        /* increment cert_index for the next cert entry */
> +        return ++cert_index;
> +    }
> +
> +    return cert_index;
> +}
> +
> +static int zipl_run_secure(ComponentEntry *entry, uint8_t *tmp_sec)
> +{
> +    bool found_signature = false;
> +    IplDeviceComponentList comps;
> +    IplSignatureCertificateList certs;
> +    uint64_t *cert = (uint64_t *)certs_sec;
> +    int cert_index = 0;
> +    int comp_index = 0;
> +    uint64_t comp_addr;
> +    int comp_len;
> +    bool have_sig;
> +    uint32_t sig_len;
> +    uint64_t cert_len = -1;
> +    uint8_t cert_idx = -1;
> +    bool verified;
> +    /*
> +     * 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};
> +    zipl_secure_print_func = zipl_secure_get_print_func(boot_mode);
> +
> +    if (!zipl_secure_ipl_supported()) {
> +        return -1;
> +    }
> +
> +    zipl_secure_init_lists(&comps, &certs);
> +
> +    have_sig = false;
> +    while (entry->component_type == ZIPL_COMP_ENTRY_LOAD ||
> +           entry->component_type == ZIPL_COMP_ENTRY_SIGNATURE) {
> +
> +        if (entry->component_type == ZIPL_COMP_ENTRY_SIGNATURE) {
> +            /* There should never be two signatures in a row */
> +            if (have_sig) {
> +                return -1;
> +            }
> +
> +            sig_len = zipl_handle_sig_entry(entry);
> +            if (sig_len < 0) {
> +                return -1;
> +            }
> +
> +            have_sig = true;
> +        } else {
> +            comp_addr = entry->compdat.load_addr;
> +            comp_len = zipl_load_segment(entry, comp_addr);
> +            if (comp_len < 0) {
> +                return -1;
> +            }
> +
> +            if (have_sig) {
> +                verified = verify_signature(comp_len, comp_addr,
> +                                            sig_len, (uint64_t)sig_sec,
> +                                            &cert_len, &cert_idx);
> +
> +                if (verified) {
> +                    cert_index = handle_certificate(cert_table, &cert,
> +                                                    cert_len, cert_idx,
> +                                                    &certs, cert_index);
> +
> +                    puts("Verified component");
> +                    zipl_secure_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 {
> +                    zipl_secure_comp_list_add(&comps, comp_index, -1,
> +                                              comp_addr, comp_len,
> +                                              S390_IPL_COMPONENT_FLAG_SC);
> +                    zipl_secure_print_func(verified, "Could not verify component");
> +                }
> +
> +                comp_index++;
> +                found_signature = true;
> +                /* After a signature is used another new one can be accepted */
> +                have_sig = false;
> +            }
> +        }
> +
> +        entry++;
> +
> +        if ((uint8_t *)(&entry[1]) > (tmp_sec + MAX_SECTOR_SIZE)) {

Less parentheses please:

         if ((uint8_t *)&entry[1] > tmp_sec + MAX_SECTOR_SIZE) {

> +            puts("Wrong entry value");
> +            return -EINVAL;
> +        }
> +    }
...
> diff --git a/pc-bios/s390-ccw/secure-ipl.h b/pc-bios/s390-ccw/secure-ipl.h
> new file mode 100644
> index 0000000000..4e2328840b
> --- /dev/null
> +++ b/pc-bios/s390-ccw/secure-ipl.h
> @@ -0,0 +1,109 @@
> +/*
> + * S/390 Secure IPL
> + *
> + * Copyright 2025 IBM Corp.
> + * Author(s): Zhuoying Cai <zycai@linux.ibm.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef _PC_BIOS_S390_CCW_SECURE_IPL_H
> +#define _PC_BIOS_S390_CCW_SECURE_IPL_H
> +
> +#include <diag320.h>
> +#include <diag508.h>
> +
> +VCStorageSizeBlock *zipl_secure_get_vcssb(void);
> +uint32_t zipl_secure_request_certificate(uint64_t *cert, uint8_t index);
> +void zipl_secure_cert_list_add(IplSignatureCertificateList *certs, int cert_index,
> +                               uint64_t *cert, uint64_t cert_len);
> +void zipl_secure_comp_list_add(IplDeviceComponentList *comps, int comp_index,
> +                               int cert_index, uint64_t comp_addr,
> +                               uint64_t comp_len, uint8_t flags);
> +int zipl_secure_update_iirb(IplDeviceComponentList *comps,
> +                            IplSignatureCertificateList *certs);
> +bool zipl_secure_ipl_supported(void);
> +void zipl_secure_init_lists(IplDeviceComponentList *comps,
> +                            IplSignatureCertificateList *certs);
> +
> +typedef void (*ipl_print_func_t)(bool, const char *);
> +
> +static inline ipl_print_func_t zipl_secure_get_print_func(ZiplBootMode boot_mode)
> +{
> +    if (boot_mode == ZIPL_SECURE_AUDIT_MODE) {
> +        return &IPL_check;
> +    }
> +
> +    return NULL;
> +}

What is this function really good for?? And why do we need the function 
pointer below??? Aparently, the function pointer can also be NULL, but you 
call it also without checking for NULL first in the various other functions 
of this file ... looks very buggy to me, please rework this!

> +extern ipl_print_func_t zipl_secure_print_func;
> +
> +static inline uint64_t diag320(void *data, unsigned long subcode)
> +{
> +    register unsigned long addr asm("0") = (unsigned long)data;
> +    register unsigned long rc asm("1") = 0;
> +
> +    asm volatile ("diag %0,%2,0x320\n"
> +                  : "+d" (addr), "+d" (rc)
> +                  : "d" (subcode)
> +                  : "memory", "cc");
> +    return rc;
> +}
> +
> +static inline uint64_t get_320_subcodes(uint64_t *ism)
> +{
> +    return diag320(ism, DIAG_320_SUBC_QUERY_ISM);
> +}

For such a simple call, it's likely not worth the effort to have a wrapper 
function.

> +static inline bool is_cert_store_facility_supported(void)
> +{
> +    uint64_t d320_ism;
> +
> +    get_320_subcodes(&d320_ism);
> +    return (d320_ism & DIAG_320_ISM_QUERY_VCSI) &&
> +           (d320_ism & DIAG_320_ISM_STORE_VC);
> +}
> +
> +static inline uint64_t _diag508(void *data, unsigned long subcode)
> +{
> +    register unsigned long addr asm("0") = (unsigned long)data;
> +    register unsigned long rc asm("1") = 0;
> +
> +    asm volatile ("diag %0,%2,0x508\n"
> +                  : "+d" (addr), "+d" (rc)
> +                  : "d" (subcode)
> +                  : "memory", "cc");
> +    return rc;
> +}
> +
> +static inline uint64_t get_508_subcodes(void)
> +{
> +    return _diag508(NULL, DIAG_508_SUBC_QUERY_SUBC);
> +}

dito.

> +static inline bool is_secure_ipl_extension_supported(void)
> +{
> +    uint64_t d508_subcodes;
> +
> +    d508_subcodes = get_508_subcodes();
> +    return d508_subcodes & DIAG_508_SUBC_SIG_VERIF;
> +}
> +
> +static inline bool verify_signature(uint64_t comp_len, uint64_t comp_addr,
> +                                    uint64_t sig_len, uint64_t sig_addr,
> +                                    uint64_t *cert_len, uint8_t *cert_idx)
> +{
> +    Diag508SignatureVerificationBlock svb = {{}, comp_len, comp_addr,
> +                                             sig_len, sig_addr };
> +
> +    if (_diag508(&svb, DIAG_508_SUBC_SIG_VERIF) == DIAG_508_RC_OK) {
> +        *cert_len = svb.csi.len;
> +        *cert_idx = svb.csi.idx;
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
> +#endif /* _PC_BIOS_S390_CCW_SECURE_IPL_H */

  Thomas