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

[Thomas Huth <thuth@redhat.com>]

On 12/02/2026 21.43, 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>
> ---
...
> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
> index b1dc35cded..1b43817af9 100644
> --- a/pc-bios/s390-ccw/s390-ccw.h
> +++ b/pc-bios/s390-ccw/s390-ccw.h
> @@ -39,6 +39,9 @@ typedef unsigned long long u64;
>   #define MIN_NON_ZERO(a, b) ((a) == 0 ? (b) : \
>                               ((b) == 0 ? (a) : (MIN(a, b))))
>   #endif
> +#ifndef ROUND_UP
> +#define ROUND_UP(n, d) (((n) + (d) - 1) & -(0 ? (n) : (d)))
> +#endif

That reason for the "0 ?" is quite hard to understand without any comments 
... it would be nice if you could add a similar remark here as we have it 
already in osdep.h for the corresponding macro.

>   #define ARRAY_SIZE(a) (sizeof(a) / sizeof((a)[0]))
>   
> @@ -64,6 +67,8 @@ void sclp_print(const char *string);
>   void sclp_set_write_mask(uint32_t receive_mask, uint32_t send_mask);
>   void sclp_setup(void);
>   void sclp_get_loadparm_ascii(char *loadparm);
> +bool sclp_is_diag320_on(void);
> +bool sclp_is_sipl_on(void);
>   int sclp_read(char *str, size_t count);
>   
>   /* virtio.c */
> @@ -76,6 +81,15 @@ int virtio_read(unsigned long sector, void *load_addr);
>   /* bootmap.c */
>   void zipl_load(void);
>   
> +typedef enum ZiplBootMode {
> +    ZIPL_BOOT_MODE_NORMAL = 0,
> +    ZIPL_BOOT_MODE_SECURE_AUDIT = 1,
> +} ZiplBootMode;
> +
> +extern ZiplBootMode boot_mode;
> +
> +ZiplBootMode get_boot_mode(uint8_t hdr_flags);
> +
>   /* jump2ipl.c */
>   void write_reset_psw(uint64_t psw);
>   int jump_to_IPL_code(uint64_t address);
> diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c
> index 4a07de018d..f7514b0245 100644
> --- a/pc-bios/s390-ccw/sclp.c
> +++ b/pc-bios/s390-ccw/sclp.c
> @@ -113,6 +113,49 @@ void sclp_get_loadparm_ascii(char *loadparm)
>       }
>   }
>   
> +static void sclp_get_fac134(uint8_t *fac134)
> +{
> +    ReadInfo *sccb = (void *)_sccb;
> +
> +    memset((char *)_sccb, 0, sizeof(ReadInfo));
> +    sccb->h.length = SCCB_SIZE;
> +    if (!sclp_service_call(SCLP_CMDW_READ_SCP_INFO, sccb)) {
> +        *fac134 = sccb->fac134;
> +    }
> +}
> +
> +bool sclp_is_diag320_on(void)
> +{
> +    uint8_t fac134 = 0;
> +
> +    sclp_get_fac134(&fac134);
> +    return fac134 & SCCB_FAC134_DIAG320_BIT;
> +}

Unless you want to use sclp_get_fac134() later in other spots, too, I'd 
maybe merge its code directly into the sclp_is_diag320_on() function instead.

...
> diff --git a/pc-bios/s390-ccw/secure-ipl.c b/pc-bios/s390-ccw/secure-ipl.c
> new file mode 100644
> index 0000000000..1d5c2d40ce
> --- /dev/null
> +++ b/pc-bios/s390-ccw/secure-ipl.c
> @@ -0,0 +1,356 @@
> +/*
> + * S/390 Secure IPL
> + *
> + * Functions to support IPL in secure boot mode (DIAG 320, DIAG 508,
> + * signature verification, and certificate handling).
> + *
> + * For secure IPL overview: docs/system/s390x/secure-ipl.rst
> + * For secure IPL technical: docs/specs/s390x-secure-ipl.rst
> + *
> + * Copyright 2025 IBM Corp.
> + * Author(s): Zhuoying Cai <zycai@linux.ibm.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include <stdlib.h>
> +#include <string.h>
> +#include <stdio.h>
> +#include "bootmap.h"
> +#include "s390-ccw.h"
> +#include "secure-ipl.h"
> +
> +uint8_t vcssb_data[VCSSB_MIN_LEN] __attribute__((__aligned__(PAGE_SIZE)));

Please mark the array as "static" unless you want to use it from another 
file later.

> +VCStorageSizeBlock *zipl_secure_get_vcssb(void)
> +{
> +    VCStorageSizeBlock *vcssb;
> +
> +    vcssb = (VCStorageSizeBlock *)vcssb_data;
> +    /* avoid retrieving vcssb multiple times */
> +    if (vcssb->length >= VCSSB_MIN_LEN) {

So in case we find VCSSB_MIN_LEN, we assume that it is ok ...

> +        return vcssb;
> +    }
> +
> +    if (!is_cert_store_facility_supported()) {
> +        puts("Certificate Store Facility is not supported by the hypervisor!");
> +        return NULL;
> +    }
> +
> +    vcssb->length = VCSSB_MIN_LEN;

... and here we set VCSSB_MIN_LEN ...

> +    if (diag320(vcssb, DIAG_320_SUBC_QUERY_VCSI) != DIAG_320_RC_OK) {

... but in case of errors, we leave the VCSSB_MIN_LEN in the buffer, so the 
next time this function gets called we will assume the buffer is fine, 
though the initialization failed for the first time. ==> I think you need to 
reset vcssb->length here or add some different logic to avoid this situation.

> +        return NULL;
> +    }
> +
> +    return vcssb;
> +}
  Thomas