Thanks for reviewing!
On 1/24/26 1:13 PM, Collin Walling wrote:
> On 12/8/25 16:32, 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 <[email protected]>
>> ---
>> docs/system/s390x/secure-ipl.rst | 34 +++
>> pc-bios/s390-ccw/Makefile | 3 +-
>> pc-bios/s390-ccw/bootmap.c | 44 +++-
>> 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 | 383 +++++++++++++++++++++++++++++++
>> pc-bios/s390-ccw/secure-ipl.h | 94 ++++++++
>> 10 files changed, 638 insertions(+), 5 deletions(-)
>> create mode 100644 pc-bios/s390-ccw/secure-ipl.c
>> create mode 100644 pc-bios/s390-ccw/secure-ipl.h
>>
>> diff --git a/docs/system/s390x/secure-ipl.rst
>> b/docs/system/s390x/secure-ipl.rst
>> index 0a02f171b4..8958a51f0b 100644
>> --- a/docs/system/s390x/secure-ipl.rst
>> +++ b/docs/system/s390x/secure-ipl.rst
>> @@ -18,3 +18,37 @@ Note: certificate files must have a .pem extension.
>> .. code-block:: shell
>>
>> qemu-system-s390x -machine
>> s390-ccw-virtio,boot-certs.0.path=/.../qemu/certs,boot-certs.1.path=/another/path/cert.pem
>> ...
>> +
>> +
>> +IPL Modes
>> +=========
>> +Multiple IPL modes are available to differentiate between the various IPL
>> +configurations. These modes are mutually exclusive and enabled based on the
>> +``boot-certs`` option on the QEMU command line.
>> +
>> +Normal Mode
>> +-----------
>> +
>> +The absence of certificates will attempt to IPL a guest without secure IPL
>> +operations. No checks are performed, and no warnings/errors are reported.
>> +This is the default mode.
>> +
>> +Configuration:
>> +
>> +.. code-block:: shell
>> +
>> + qemu-system-s390x -machine s390-ccw-virtio ...
>> +
>> +Audit Mode
>> +----------
>> +
>> +With *only* the presence of certificates in the store, it is assumed that
>> secure
>> +boot operations should be performed with errors reported as warnings. As
>> such,
>> +the secure IPL operations will be performed, and any errors that stem from
>> these
>> +operations will result in a warning.
>
> I think both sentences are saying the same thing. How about:
>
> "When the certificate store is populated with at least one certificate
> and no additional secure IPL parameters are provided on the command
> line, then secure IPL will proceed in "audit mode". All secure IPL
> operations will be performed with signature verification errors reported
> as non-disruptive warnings."
>
>> +
>> +Configuration:
>> +
>> +.. code-block:: shell
>> +
>> + qemu-system-s390x -machine
>> s390-ccw-virtio,boot-certs.0.path=/.../qemu/certs,boot-certs.1.path=/another/path/cert.pem
>> ...
>> diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile
>> index a0f24c94a8..603761a857 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 9a03eab6ed..342772860b 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 */
>> @@ -617,7 +618,7 @@ static int ipl_eckd(void)
>> * Returns: length of the segment on success,
>> * negative value on error.
>> */
>> -static int zipl_load_segment(ComponentEntry *entry, uint64_t address)
>> +int zipl_load_segment(ComponentEntry *entry, uint64_t address)
>> {
>> const int max_entries = (MAX_SECTOR_SIZE / sizeof(ScsiBlockPtr));
>> ScsiBlockPtr *bprs = (void *)sec;
>> @@ -736,9 +737,22 @@ static int zipl_run(ScsiBlockPtr *pte)
>> /* Load image(s) into RAM */
>> entry = (ComponentEntry *)(&header[1]);
>>
>> - rc = zipl_run_normal(&entry, tmp_sec);
>> - if (rc) {
>> - return rc;
>> + switch (boot_mode) {
>> + case ZIPL_BOOT_MODE_SECURE_AUDIT:
>> + rc = zipl_run_secure(&entry, tmp_sec);
>> + if (rc) {
>> + return rc;
>> + }
>> + break;
>> + case ZIPL_BOOT_MODE_NORMAL:
>> + rc = zipl_run_normal(&entry, tmp_sec);
>> + if (rc) {
>> + return rc;
>> + }
>> + break;
>> + default:
>> + puts("Unknown boot mode");
>> + return -1;
>> }
>
> Each case has a common pattern. Move the if (rc) check outside the
> switch block.
>
>>
>> if (entry->component_type != ZIPL_COMP_ENTRY_EXEC) {
>> @@ -1103,17 +1117,35 @@ static int zipl_load_vscsi(void)
>> * IPL starts here
>> */
>>
>> +ZiplBootMode zipl_mode(uint8_t hdr_flags)
>> +{
>> + bool sipl_set = hdr_flags & DIAG308_IPIB_FLAGS_SIPL;
>> + bool iplir_set = hdr_flags & DIAG308_IPIB_FLAGS_IPLIR;
>> +
>> + if (!sipl_set && iplir_set) {
>> + return ZIPL_BOOT_MODE_SECURE_AUDIT;
>> + }
>> +
>> + return ZIPL_BOOT_MODE_NORMAL;
>> +}
>
> What about making this a void function `set_boot_mode()` which gets
> invoked in main just after the iplb is stored. This would also remove
> the need for the "UNSPECIFIED" mode, since this variable would get
> resolved early on.
>
> Perhaps it makes more sense to default to NORMAL unless the parameters
> specify otherwise?
>
I agree that NORMAL can be the default, so the UNSPECIFIED mode can be
removed. It may be clearer to keep this as a value-returning helper and
rename it to get_boot_mode() (or similar). The function simply derives
the boot mode from the IPLB flags, and main can assign it directly after
the IPLB is stored:
boot_mode = get_boot_mode(iplb->hdr_flags);
>> +
>> void zipl_load(void)
>> {
>> VDev *vdev = virtio_get_device();
>>
>> if (vdev->is_cdrom) {
>> + if (boot_mode == ZIPL_BOOT_MODE_SECURE_AUDIT) {
>> + panic("Secure boot from ISO image is not supported!");
>> + }
>> ipl_iso_el_torito();
>> puts("Failed to IPL this ISO image!");
>> return;
>> }
>>
>> if (virtio_get_device_type() == VIRTIO_ID_NET) {
>> + if (boot_mode == ZIPL_BOOT_MODE_SECURE_AUDIT) {
>> + panic("Virtio net boot device does not support secure boot!");
>> + }
>> netmain();
>> puts("Failed to IPL from this network!");
>> return;
>> @@ -1124,6 +1156,10 @@ void zipl_load(void)
>> return;
>> }
>>
>> + if (boot_mode == ZIPL_BOOT_MODE_SECURE_AUDIT) {
>> + panic("ECKD boot device does not support secure boot!");
>> + }
>> +
>
> For all cases here in zipl_load, an IPL_assert is much cleaner. e.g.
>
> IPL_assert(boot_mode == ZIPL_BOOT_MODE_NORMAL, "message");
>
> Which will also cover the need to check for the other boot modes in a
> future patch.
>
> Additionally, I'm not sure if "ECKD boot device" is correct here.
> Couldn't the IPL devices after this point actually be virtio-blk,
> virtio-scsi, etc? Perhaps a generic "Boot device does not support
> secure IPL" or something is sufficient.
>
Yes, IPL devices after this point are virtio-blk/virtio-scsi using the
ECKD scheme. Maybe we could rephrase the message to something like:
"Secure boot with the ECKD scheme is not supported!"
>> switch (virtio_get_device_type()) {
>> case VIRTIO_ID_BLOCK:
>> zipl_load_vblk();
>> diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
>> index 95943441d3..dc2783faa2 100644
>> --- a/pc-bios/s390-ccw/bootmap.h
>> +++ b/pc-bios/s390-ccw/bootmap.h
>> @@ -88,9 +88,18 @@ typedef struct BootMapTable {
>> BootMapPointer entry[];
>> } __attribute__ ((packed)) BootMapTable;
>>
>> +#define DER_SIGNATURE_FORMAT 1
>> +
>> +typedef struct SignatureInformation {
>> + uint8_t format;
>> + uint8_t reserved[3];
>> + uint32_t sig_len;
>> +} SignatureInformation;
>> +
>> typedef union ComponentEntryData {
>> uint64_t load_psw;
>> uint64_t load_addr;
>> + SignatureInformation sig_info;
>> } ComponentEntryData;
>>
>> typedef struct ComponentEntry {
>> @@ -113,6 +122,8 @@ typedef struct ScsiMbr {
>> ScsiBlockPtr pt; /* block pointer to program table */
>> } __attribute__ ((packed)) ScsiMbr;
>>
>> +int zipl_load_segment(ComponentEntry *entry, uint64_t address);
>> +
>> #define ZIPL_MAGIC "zIPL"
>> #define ZIPL_MAGIC_EBCDIC "\xa9\xc9\xd7\xd3"
>> #define IPL1_MAGIC "\xc9\xd7\xd3\xf1" /* == "IPL1" in EBCDIC */
>> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
>> index c9328f1c51..8aabce115f 100644
>> --- a/pc-bios/s390-ccw/main.c
>> +++ b/pc-bios/s390-ccw/main.c
>> @@ -28,6 +28,7 @@ IplParameterBlock *iplb;
>> bool have_iplb;
>> static uint16_t cutype;
>> LowCore *lowcore; /* Yes, this *is* a pointer to address 0 */
>> +ZiplBootMode boot_mode;
>>
>> #define LOADPARM_PROMPT "PROMPT "
>> #define LOADPARM_EMPTY " "
>> @@ -272,9 +273,17 @@ static int virtio_setup(void)
>>
>> static void ipl_boot_device(void)
>> {
>> + if (boot_mode == ZIPL_BOOT_MODE_UNSPECIFIED) {
>> + boot_mode = zipl_mode(iplb->hdr_flags);
>> + }
>> +
>> switch (cutype) {
>> case CU_TYPE_DASD_3990:
>> case CU_TYPE_DASD_2107:
>> + if (boot_mode == ZIPL_BOOT_MODE_SECURE_AUDIT) {
>> + panic("Passthrough (vfio) CCW device does not support secure
>> boot!");
>> + }
>> +
>
> IPL_assert
>
>> dasd_ipl(blk_schid, cutype);
>> break;
>> case CU_TYPE_VIRTIO:
>> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
>> index b1dc35cded..c2ba40d067 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
>>
>> #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,16 @@ int virtio_read(unsigned long sector, void *load_addr);
>> /* bootmap.c */
>> void zipl_load(void);
>>
>> +typedef enum ZiplBootMode {
>> + ZIPL_BOOT_MODE_UNSPECIFIED = 0,
>> + ZIPL_BOOT_MODE_NORMAL = 1,
>> + ZIPL_BOOT_MODE_SECURE_AUDIT = 2,
>> +} ZiplBootMode;
>> +
>> +extern ZiplBootMode boot_mode;
>> +
>> +ZiplBootMode zipl_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..0b03c3164f 100644
>> --- a/pc-bios/s390-ccw/sclp.c
>> +++ b/pc-bios/s390-ccw/sclp.c
>> @@ -113,6 +113,50 @@ void sclp_get_loadparm_ascii(char *loadparm)
>> }
>> }
>>
>> +static void sclp_get_fac134(uint8_t *fac134)
>> +{
>> +
>
> Remove extra newline.
>
>> + 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;
>> +}
>> +
>> +/*
>> + * Get fac_ipl (byte 136 and byte 137 of the SCLP Read Info block)
>> + * for IPL device facilities.
>> + */
>> +static void sclp_get_fac_ipl(uint16_t *fac_ipl)
>> +{
>> +
>> + 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)) {
>> + *fac_ipl = sccb->fac_ipl;
>> + }
>> +}
>
> Hmm. The number of SCLP service calls could be reduced by doing a
> singular read scp info call early and storing all the fields needed to
> be accessed later.
>
> But maybe that is outside the scope of these patches.
>
>> +
>> +bool sclp_is_sipl_on(void)
>> +{
>> + uint16_t fac_ipl = 0;
>> +
>> + sclp_get_fac_ipl(&fac_ipl);
>> + return fac_ipl & SCCB_FAC_IPL_SIPL_BIT;
>> +}
>> +
>> int sclp_read(char *str, size_t count)
>> {
>> ReadEventData *sccb = (void *)_sccb;
>> diff --git a/pc-bios/s390-ccw/sclp.h b/pc-bios/s390-ccw/sclp.h
>> index 64b53cad29..cf147f4634 100644
>> --- a/pc-bios/s390-ccw/sclp.h
>> +++ b/pc-bios/s390-ccw/sclp.h
>> @@ -50,6 +50,8 @@ typedef struct SCCBHeader {
>> } __attribute__((packed)) SCCBHeader;
>>
>> #define SCCB_DATA_LEN (SCCB_SIZE - sizeof(SCCBHeader))
>> +#define SCCB_FAC134_DIAG320_BIT 0x4
>> +#define SCCB_FAC_IPL_SIPL_BIT 0x4000
>>
>> typedef struct ReadInfo {
>> SCCBHeader h;
>> @@ -57,6 +59,10 @@ typedef struct ReadInfo {
>> uint8_t rnsize;
>> uint8_t reserved[13];
>> uint8_t loadparm[LOADPARM_LEN];
>> + uint8_t reserved1[102];
>> + uint8_t fac134;
>> + uint8_t reserved2;
>> + uint16_t fac_ipl;
>> } __attribute__((packed)) ReadInfo;
>>
>> typedef struct SCCB {
>> diff --git a/pc-bios/s390-ccw/secure-ipl.c b/pc-bios/s390-ccw/secure-ipl.c
>> new file mode 100644
>> index 0000000000..c1c5bc682a
>> --- /dev/null
>> +++ b/pc-bios/s390-ccw/secure-ipl.c
>> @@ -0,0 +1,383 @@
>> +/*
>> + * 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 <[email protected]>
>> + *
>> + * 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)));
>> +
>> +VCStorageSizeBlock *zipl_secure_get_vcssb(void)
>> +{
>> + VCStorageSizeBlock *vcssb;
>> + int rc;
>> +
>> + if (!sclp_is_diag320_on() || !is_cert_store_facility_supported()) {
>> + puts("Certificate Store Facility is not supported by the
>> hypervisor!");
>> + return NULL;
>> + }
>> +
>> + vcssb = (VCStorageSizeBlock *)vcssb_data;
>> + /* avoid retrieving vcssb multiple times */
>> + if (vcssb->length >= VCSSB_MIN_LEN) {
>> + return vcssb;
>> + }
>
> Change the order of operations here to check if vcssb has been retrieved
> before checking diag320 stuff. Otherwise, every time
> `zipl_secure_get_vcssb()` is invoked, both an sclp service call and a
> diag320 call is always performed, which is unnecessary if the VCSSB has
> already been stored.
>
>> +
>> + vcssb->length = VCSSB_MIN_LEN;
>> + rc = diag320(vcssb, DIAG_320_SUBC_QUERY_VCSI);
>> + if (rc != DIAG_320_RC_OK) {
>> + return NULL;
>> + }
>
> Can do if (diag320(vcssb, DIAG_320_SUBC_QUERY_VCSI) != DIAG_320_RC_OK)
>
> and then get rid of rc
>
>> +
>> + return vcssb;
>> +}
>> +
>> +static uint32_t get_certs_length(void)
>
> The function name is ambiguous. I had to digest the math below to
> understand the intention. How about something like...
>
> `get_total_certs_length()`
>
> ...to help clarify that this function is returning a value to store all
> certs.
>
>> +{
>> + VCStorageSizeBlock *vcssb;
>> + uint32_t len;
>> +
>> + vcssb = zipl_secure_get_vcssb();
>> + if (vcssb == NULL) {
>> + return 0;
>> + }
>> +
>> + len = vcssb->total_vcb_len - VCB_HEADER_LEN - vcssb->total_vc_ct *
>> VCE_HEADER_LEN;
>> +
>
> Could return the line above and get rid of len.
>
>> + return len;
>> +}
>> +
>> +static uint32_t request_certificate(uint8_t *cert, uint8_t index)
>> +{
>> + VCStorageSizeBlock *vcssb;
>> + VCBlock *vcb;
>> + VCEntry *vce;
>> + uint64_t rc = 0;
>> + uint32_t cert_len = 0;
>> + uint32_t max_single_vcb_len;
>> +
>> + /* Get Verification Certificate Storage Size block with DIAG320 subcode
>> 1 */
>> + vcssb = zipl_secure_get_vcssb();
>> + if (vcssb == NULL) {
>> + return 0;
>> + }
>> +
>> + /*
>> + * Request single entry
>> + * Fill input fields of single-entry VCB
>> + */
>> + max_single_vcb_len = ROUND_UP(vcssb->max_single_vcb_len, PAGE_SIZE);
>> + vcb = malloc(max_single_vcb_len);
>> + vcb->in_len = max_single_vcb_len;
>> + vcb->first_vc_index = index + 1;
>> + vcb->last_vc_index = index + 1;
>
> There needs to be a better way to account for the index origin
> differences. See my comments below for `verify_signature()`.
>
>> +
>> + rc = diag320(vcb, DIAG_320_SUBC_STORE_VC);
>> + if (rc != DIAG_320_RC_OK) {
>> + goto out;
>> + }
>
> if (!diag320(vcb, DIAG_320_SUBC_STORE_VC))
>
> then get rid of rc
>
>> +
>> + if (vcb->out_len == VCB_HEADER_LEN) {
>> + puts("No certificate entry");
>> + goto out;
>> + }
>> + if (vcb->remain_ct != 0) {
>> + puts("Not enough memory to store all requested certificates");
>> + goto out;
>> + }
>> +
>> + vce = (VCEntry *)vcb->vce_buf;
>> + if (!(vce->flags & DIAG_320_VCE_FLAGS_VALID)) {
>> + puts("Invalid certificate");
>> + goto out;
>> + }
>> +
>> + cert_len = vce->cert_len;
>> + memcpy(cert, (uint8_t *)vce + vce->cert_offset, vce->cert_len);
>> +
>> +out:
>> + free(vcb);
>> + return cert_len;
>> +}
>> +
>> +static void cert_list_add(IplSignatureCertificateList *certs, int
>> cert_index,
>> + uint8_t *cert, uint64_t cert_len)
>
> s/certs/cert_list
> s/cert/cert_addr
>
>> +{
>> + if (cert_index > MAX_CERTIFICATES - 1) {
>> + printf("Warning: Ignoring cert entry [%d] because it's over %d
>> entires\n",
>> + cert_index + 1, MAX_CERTIFICATES);
>> + return;
>> + }
>> +
>> + certs->cert_entries[cert_index].addr = (uint64_t)cert;
>> + certs->cert_entries[cert_index].len = cert_len;
>> + certs->ipl_info_header.len += sizeof(certs->cert_entries[cert_index]);
>> +}
>> +
>> +static void comp_list_add(IplDeviceComponentList *comps, int comp_index,
>> + int cert_index, uint64_t comp_addr,
>> + uint64_t comp_len, uint8_t flags)
>
> s/comps/comp_list
>
>> +{
>> + if (comp_index > MAX_CERTIFICATES - 1) {
>> + printf("Warning: Ignoring comp entry [%d] because it's over %d
>> entires\n",
>> + comp_index + 1, MAX_CERTIFICATES);
>> + return;
>> + }
>> +
>> + comps->device_entries[comp_index].addr = comp_addr;
>> + comps->device_entries[comp_index].len = comp_len;
>> + comps->device_entries[comp_index].flags = flags;
>> + comps->device_entries[comp_index].cert_index = cert_index;
>> + comps->ipl_info_header.len += sizeof(comps->device_entries[comp_index]);
>> +}
>> +
>> +static int update_iirb(IplDeviceComponentList *comps,
>> IplSignatureCertificateList *certs)
>> +{
>> + IplInfoReportBlock *iirb;
>> + IplDeviceComponentList *iirb_comps;
>> + IplSignatureCertificateList *iirb_certs;
>> + uint32_t iirb_hdr_len;
>> + uint32_t comps_len;
>> + uint32_t certs_len;
>> +
>> + if (iplb->len % 8 != 0) {
>> + panic("IPL parameter block length field value is not multiple of 8
>> bytes");
>> + }
>> +
>> + iirb_hdr_len = sizeof(IplInfoReportBlockHeader);
>> + comps_len = comps->ipl_info_header.len;
>> + certs_len = certs->ipl_info_header.len;
>> + if ((comps_len + certs_len + iirb_hdr_len) >
>> sizeof(IplInfoReportBlock)) {
>> + puts("Not enough space to hold all components and certificates in
>> IIRB");
>> + return -1;
>> + }
>
> Shouldn't this also be a panic? If there's not enough space for the
> IIRB, then that should be more serious than just warning in audit mode.
>
>> +
>> + /* IIRB immediately follows IPLB */
>> + iirb = &ipl_data.iirb;
>> + iirb->hdr.len = iirb_hdr_len;
>> +
>> + /* Copy IPL device component list after IIRB Header */
>> + iirb_comps = (IplDeviceComponentList *) iirb->info_blks;
>> + memcpy(iirb_comps, comps, comps_len);
>> +
>> + /* Update IIRB length */
>> + iirb->hdr.len += comps_len;
>> +
>> + /* Copy IPL sig cert list after IPL device component list */
>> + iirb_certs = (IplSignatureCertificateList *) (iirb->info_blks +
>> +
>> iirb_comps->ipl_info_header.len);
>> + memcpy(iirb_certs, certs, certs_len);
>> +
>> + /* Update IIRB length */
>> + iirb->hdr.len += certs_len;
>> +
>> + return 0;
>> +}
>> +
>> +static bool secure_ipl_supported(void)
>> +{
>> + if (!sclp_is_sipl_on()) {
>> + puts("Secure IPL Facility is not supported by the hypervisor!");
>> + return false;
>> + }
>> +
>> + if (!is_secure_ipl_extension_supported()) {
>> + puts("Secure IPL extensions are not supported by the hypervisor!");
>> + return false;
>> + }
>> +
>> + if (!sclp_is_diag320_on() || !is_cert_store_facility_supported()) {
>> + puts("Certificate Store Facility is not supported by the
>> hypervisor!");
>> + return false;
>> + }
>> +
>> + return true;
>> +}
>> +
>> +static void init_lists(IplDeviceComponentList *comps,
>> IplSignatureCertificateList *certs)
>> +{
>> + comps->ipl_info_header.ibt = IPL_IBT_COMPONENTS;
>> + comps->ipl_info_header.len = sizeof(comps->ipl_info_header);
>> +
>> + certs->ipl_info_header.ibt = IPL_IBT_CERTIFICATES;
>> + certs->ipl_info_header.len = sizeof(certs->ipl_info_header);
>> +}
>> +
>> +static uint32_t zipl_load_signature(ComponentEntry *entry, uint64_t sig_sec)
>> +{
>
> Needs to be type int, otherwise returning -1 below doesn't make sense.
>
>> + uint32_t sig_len;
>> +
>> + if (zipl_load_segment(entry, 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;
>
> Could return entry->compdat.sig_info.sig_len, then sig_len goes away.
>
>> +}
>> +
>> +/*
>> + * Returns: 1 - New certificate requested and added to cert_table and cert
>> list
>> + * 0 - Certificate already exists in the cert_table
>> + * -1 - Error while requesting certificate
>> + */
>> +static int handle_certificate(int *cert_table, uint8_t **cert,
>> + uint64_t cert_len, uint8_t cert_table_idx,
>> + IplSignatureCertificateList *certs, int
>> cert_entry_idx)
>> +{
>> + if (cert_table[cert_table_idx] != -1) {
>> + return 0;
>> + }
>> +
>> + if (!request_certificate(*cert, cert_table_idx)) {
>> + puts("Could not get certificate");
>> + return -1;
>> + }
>> +
>> + cert_list_add(certs, cert_entry_idx, *cert, cert_len);
>> + cert_table[cert_table_idx] = cert_entry_idx;
>> + return 1;
>> +}
>> +
>> +int zipl_run_secure(ComponentEntry **entry_ptr, uint8_t *tmp_sec)
>> +{
>> + IplDeviceComponentList comps;
>> + IplSignatureCertificateList certs;
>
> It would read a little easier if these were comp_list and cert_list
>
>> + ComponentEntry *entry = *entry_ptr;
>> + uint8_t *cert = NULL;
>> + uint64_t *sig = NULL;
>> + int cert_entry_idx = 0;
>> + int comp_entry_idx = 0;
>> + uint64_t comp_addr;
>> + int comp_len;
>> + uint32_t sig_len = 0;
>> + uint64_t cert_len = -1;
>> + uint8_t cert_table_idx = -1;
>> + 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 (cert_table_idx):
>> + * index of certificate from qemu cert store used for
>> verification
>> + * cert_table value (cert_entry_idx):
>> + * index of cert entry in cert list that contains the
>> certificate
>> + */
>> + int cert_table[MAX_CERTIFICATES] = { [0 ... MAX_CERTIFICATES - 1] = -1};
>
> If you agree with the suggestion below for verify_signature to adjust
> the returned index, then the cert_table can be used a bit differently:
>
> Make this an array of booleans. Use the index returned by
> verify_signature. Value true means that the cert has already been added
> to the IplSignatureCertificateList. False means it has yet to be added
> to the list.
>
If verify_signature adjusted the returned index, I don’t quite see why
cert_table should become a boolean array. We still need to track the
certificate index in IplSignatureCertificateList, since that index is
used later as the IPL-Signature-Certificate Index (SCI) in
IplDeviceComponentList. That’s what cert_table currently provides.
With a boolean array, that index information would be lost. Am I missing
something?
> Remove the mention of "qemu cert store", as the BIOS technically does
> not know anything about the underlying cert store or its implementation.
>
>> + int rc;
>> + int signed_count = 0;
>> +
>> + if (!secure_ipl_supported()) {
>> + return -1;
>> + }
>> +
>> + init_lists(&comps, &certs);
>> + certs_len = get_certs_length();
>> + cert = malloc(certs_len);
>
> cert = malloc(get_total_certs_length());
>
> Then get rid of the certs_len variable.
>
>> + 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_table_idx);
>> +
>> + if (verified) {
>> + rc = handle_certificate(cert_table, &cert, cert_len,
>> + cert_table_idx, &certs,
>> cert_entry_idx);
>
> Check cert_table here before calling handle_certificate. It would
> contain all the cert_table logic to zipl_run_secure and make it a little
> bit easier to understand. Also less variables to pass around.
>
>> + if (rc == -1) {
>> + goto out;
>> + }
>> +
>> + /* increment for the next certificate */
>> + if (rc == 1) {
>> + cert_entry_idx++;
>> + cert += cert_len;
>> + }
>> +
>> + puts("Verified component");
>> + comp_list_add(&comps, comp_entry_idx,
>> cert_table[cert_table_idx],
>> + comp_addr, comp_len,
>> + S390_IPL_COMPONENT_FLAG_SC |
>> S390_IPL_COMPONENT_FLAG_CSV);
>> + } else {
>> + comp_list_add(&comps, comp_entry_idx, -1,
>> + comp_addr, comp_len,
>> + S390_IPL_COMPONENT_FLAG_SC);
>> + zipl_secure_handle("Could not verify component");
>> + }
>
> This can be cleaned up a bit. Set cert_index and flags within the
> if/else block. Call `comp_list_add()` once after the block.
>
>> +
>> + comp_entry_idx++;
>> + 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;
>> +}
>> diff --git a/pc-bios/s390-ccw/secure-ipl.h b/pc-bios/s390-ccw/secure-ipl.h
>> new file mode 100644
>> index 0000000000..a6fc1ac8de
>> --- /dev/null
>> +++ b/pc-bios/s390-ccw/secure-ipl.h
>> @@ -0,0 +1,94 @@
>> +/*
>> + * S/390 Secure IPL
>> + *
>> + * Copyright 2025 IBM Corp.
>> + * Author(s): Zhuoying Cai <[email protected]>
>> + *
>> + * 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);
>> +int zipl_run_secure(ComponentEntry **entry_ptr, uint8_t *tmp_sec);
>> +
>> +static inline void zipl_secure_handle(const char *message)
>
> This could be implemented like an assert, where the first param is the
> condition.
>
> e.g.
>
> `zipl_secure_handle(signed_count > 0, "Secure boot is on, but components
> are not signed");`
>
> Then some if checks / extra indentation goes away.
>
>> +{
>> + switch (boot_mode) {
>> + case ZIPL_BOOT_MODE_SECURE_AUDIT:
>> + IPL_check(false, message);
>> + break;
>> + default:
>> + break;
>> + }
>> +}
>> +
>> +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 bool is_cert_store_facility_supported(void)
>> +{
>> + uint32_t d320_ism;
>> +
>> + diag320(&d320_ism, DIAG_320_SUBC_QUERY_ISM);
>> + return (d320_ism & DIAG_320_ISM_QUERY_SUBCODES) &&
>> + (d320_ism & DIAG_320_ISM_QUERY_VCSI) &&
>> + (d320_ism & DIAG_320_ISM_STORE_VC);
>> +}
>
> Don't need to check if DIAG_320_ISM_QUERY_SUBCODES is supported (implied
> if query ISM returns OK).
>
> Can clean this up by doing `return d320_ism & (DIAG_320_ISM_QUERY_VCSI |
> DIAG_320_ISM_STORE_VC);`
>
> Merge this function with `sclp_is_diag320_on()`, checking the read scp
> facility bit first before calling diag320.
>
>> +
>> +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 bool is_secure_ipl_extension_supported(void)
>
> "is_signature_verif_supported()" or something like that reads better.
>
>> +{
>> + uint64_t d508_subcodes;
>> +
>> + d508_subcodes = _diag508(NULL, DIAG_508_SUBC_QUERY_SUBC);
>> + 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)
>> +{
>> + Diag508SigVerifBlock svb;
>> +
>> + svb.length = sizeof(Diag508SigVerifBlock);
>> + svb.version = 0;
>> + svb.comp_len = comp_len;
>> + svb.comp_addr = comp_addr;
>> + svb.sig_len = sig_len;
>> + svb.sig_addr = sig_addr;
>> +
>> + if (_diag508(&svb, DIAG_508_SUBC_SIG_VERIF) == DIAG_508_RC_OK) {
>> + *cert_len = svb.cert_len;
>> + *cert_idx = svb.cert_store_index;
>
> Adjust for different indexing origins here, accounting for what DIAG320
> will later need when retrieving certs:
>
> ```
> /*
> * DIAG 508 utilizes an index origin of 0 when indexing the cert store.
> * The cert_idx will be used for DIAG 320 data structures, which expects
> * an index origin of 1. Account for the offset here so it's easier to
> * manage later.
> */
> *cert_idx = svb.cert_store_index + 1;
> ```
>
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> +#endif /* _PC_BIOS_S390_CCW_SECURE_IPL_H */
>
