Re: [PATCH v16 23/33] pc-bios/s390-ccw: Add signature verification for secure IPL in audit mode
On 7/16/26 12:36, Zhuoying Cai wrote:
> On 7/16/26 10:05 AM, Jared Rossi wrote:
>>
>>
>> On 7/15/26 4:37 PM, Collin Walling wrote:
>>> On 7/7/26 19:40, Zhuoying Cai wrote:
/* should not return */
write_reset_psw(entry->compdat.load_psw);
+
+if (boot_mode == ZIPL_BOOT_MODE_SECURE_AUDIT) {
+update_cert_list(&cert_list);
+update_iirb(&comp_list, &cert_list);
+free(tmp_cert_buf);
+}
+
>>> Sorry if I missed this from the previous rounds of review, but why was
>>> this moved outside of `zipl_run_secure()`? It seems out of place here.
>>>
>>> If there is justification for it, I'd suggest at least moving this chunk
>>> a few lines up to before the `write_reset_psw` call to keep the "IPL
>>> jump" code together. The comment above is meant for the following chunk:
>>>
>>> ```
>>> /* should not return */
>>> write_reset_psw(entry->compdat.load_psw);
>>> jump_to_IPL_code(0);
>>> return -1;
>>> ```
>>>
>>> Otherwise the comment is a bit misleading. I suppose one could argue
>>> the comment should actually be placed abouve `jump_to_IPL_code`, but
>>> let's not bother with that change.
>>>
>>
>> This was something I had asked for, but I don't recall if it was in a
>> review comment or internal discussion, but a brief summary for the sake
>> of posterity...
>>
>> Moving the call to update_iirb() was necessitated by a different change,
>> which was to use a specific memory address range for storing the cert
>> list. The fundamental issue is that we wanted to store the cert list at
>> a calculated address to ensure it is not clobbered before the kernel
>> reads it. My idea was to reclaim the space previously used for chained
>> IPLBs. The chained IPLBs and the cert list are never needed at the same
>> time, so that isn't an issue; however, the impact of reclaiming the
>> space is that we destroy our IPLB chain, meaning, once we write the cert
>> list, if the IPL subsequently fails, we no longer have the fallback IPLB
>> to try the next device in boot order. To deal with this we delayed
>> updating the cert list and IIRB until we are committed to boot from the
>> selected device, that is to say, once we have progressed through the IPL
>> far enough that there is no longer any possibility of falling back to a
>> different device. As such, we wait until we are ready to actually hand
>> over control to the kernel before overwriting any of the IPL data.
>>
>> There is some flexibility with exactly where the calls can be placed,
>> but the idea is that there should not be any paths that return for retry
>> on error after we update the cert list. By placing the update calls
>> directly before the jump we know the only two outcomes remaining are to
>> either successfully hand over control and never return, or for the jump
>> itself to fail, which would be a catastrophic error and necessitate that
>> the IPL is aborted entirely anyway. There is no condition such that we
>> may update the cert list or IIRB and then realize we actually should try
>> booting from a different device instead.
>>
>> Regards,
>> Jared Rossi
>
> Thanks for the clarification, Jared.
>
> I think moving the update_iirb chunk before write_rest_psw() should
> work, as long as it is invoked after committing to the current IPLB and
> does not return for a retry afterward.
>
>
Gotcha, I see the "retry" loop now, and why with the changes to reusing
the qipl.ipl_data necessitates moving this code out of run secure. Had
to look at the greater scope to understand. Thanks.
How about just move the /* should not return */ to the same line as the
`return -1` line so its placement is more accurate. Either in this
patch or in patch 20, where zipl_run is refactored (latter is more
preferable).
With that change as well as an update to the comment in
update_cert_list, I'd feel comfortable with:
Reviewed-by: Collin Walling
No need to change where you placed the chunk of code above. The rest of
my comments are nits.
--
Regards,
Collin
Re: [PATCH v16 23/33] pc-bios/s390-ccw: Add signature verification for secure IPL in audit mode
On 7/16/26 10:05 AM, Jared Rossi wrote:
>
>
> On 7/15/26 4:37 PM, Collin Walling wrote:
>> On 7/7/26 19:40, Zhuoying Cai wrote:
>>> /* should not return */
>>> write_reset_psw(entry->compdat.load_psw);
>>> +
>>> +if (boot_mode == ZIPL_BOOT_MODE_SECURE_AUDIT) {
>>> +update_cert_list(&cert_list);
>>> +update_iirb(&comp_list, &cert_list);
>>> +free(tmp_cert_buf);
>>> +}
>>> +
>> Sorry if I missed this from the previous rounds of review, but why was
>> this moved outside of `zipl_run_secure()`? It seems out of place here.
>>
>> If there is justification for it, I'd suggest at least moving this chunk
>> a few lines up to before the `write_reset_psw` call to keep the "IPL
>> jump" code together. The comment above is meant for the following chunk:
>>
>> ```
>> /* should not return */
>> write_reset_psw(entry->compdat.load_psw);
>> jump_to_IPL_code(0);
>> return -1;
>> ```
>>
>> Otherwise the comment is a bit misleading. I suppose one could argue
>> the comment should actually be placed abouve `jump_to_IPL_code`, but
>> let's not bother with that change.
>>
>
> This was something I had asked for, but I don't recall if it was in a
> review comment or internal discussion, but a brief summary for the sake
> of posterity...
>
> Moving the call to update_iirb() was necessitated by a different change,
> which was to use a specific memory address range for storing the cert
> list. The fundamental issue is that we wanted to store the cert list at
> a calculated address to ensure it is not clobbered before the kernel
> reads it. My idea was to reclaim the space previously used for chained
> IPLBs. The chained IPLBs and the cert list are never needed at the same
> time, so that isn't an issue; however, the impact of reclaiming the
> space is that we destroy our IPLB chain, meaning, once we write the cert
> list, if the IPL subsequently fails, we no longer have the fallback IPLB
> to try the next device in boot order. To deal with this we delayed
> updating the cert list and IIRB until we are committed to boot from the
> selected device, that is to say, once we have progressed through the IPL
> far enough that there is no longer any possibility of falling back to a
> different device. As such, we wait until we are ready to actually hand
> over control to the kernel before overwriting any of the IPL data.
>
> There is some flexibility with exactly where the calls can be placed,
> but the idea is that there should not be any paths that return for retry
> on error after we update the cert list. By placing the update calls
> directly before the jump we know the only two outcomes remaining are to
> either successfully hand over control and never return, or for the jump
> itself to fail, which would be a catastrophic error and necessitate that
> the IPL is aborted entirely anyway. There is no condition such that we
> may update the cert list or IIRB and then realize we actually should try
> booting from a different device instead.
>
> Regards,
> Jared Rossi
Thanks for the clarification, Jared.
I think moving the update_iirb chunk before write_rest_psw() should
work, as long as it is invoked after committing to the current IPLB and
does not return for a retry afterward.
Re: [PATCH v16 23/33] pc-bios/s390-ccw: Add signature verification for secure IPL in audit mode
On 7/15/26 4:37 PM, Collin Walling wrote:
On 7/7/26 19:40, Zhuoying Cai wrote:
/* should not return */
write_reset_psw(entry->compdat.load_psw);
+
+if (boot_mode == ZIPL_BOOT_MODE_SECURE_AUDIT) {
+update_cert_list(&cert_list);
+update_iirb(&comp_list, &cert_list);
+free(tmp_cert_buf);
+}
+
Sorry if I missed this from the previous rounds of review, but why was
this moved outside of `zipl_run_secure()`? It seems out of place here.
If there is justification for it, I'd suggest at least moving this chunk
a few lines up to before the `write_reset_psw` call to keep the "IPL
jump" code together. The comment above is meant for the following chunk:
```
/* should not return */
write_reset_psw(entry->compdat.load_psw);
jump_to_IPL_code(0);
return -1;
```
Otherwise the comment is a bit misleading. I suppose one could argue
the comment should actually be placed abouve `jump_to_IPL_code`, but
let's not bother with that change.
This was something I had asked for, but I don't recall if it was in a
review comment or internal discussion, but a brief summary for the sake
of posterity...
Moving the call to update_iirb() was necessitated by a different change,
which was to use a specific memory address range for storing the cert
list. The fundamental issue is that we wanted to store the cert list at
a calculated address to ensure it is not clobbered before the kernel
reads it. My idea was to reclaim the space previously used for chained
IPLBs. The chained IPLBs and the cert list are never needed at the same
time, so that isn't an issue; however, the impact of reclaiming the
space is that we destroy our IPLB chain, meaning, once we write the cert
list, if the IPL subsequently fails, we no longer have the fallback IPLB
to try the next device in boot order. To deal with this we delayed
updating the cert list and IIRB until we are committed to boot from the
selected device, that is to say, once we have progressed through the IPL
far enough that there is no longer any possibility of falling back to a
different device. As such, we wait until we are ready to actually hand
over control to the kernel before overwriting any of the IPL data.
There is some flexibility with exactly where the calls can be placed,
but the idea is that there should not be any paths that return for retry
on error after we update the cert list. By placing the update calls
directly before the jump we know the only two outcomes remaining are to
either successfully hand over control and never return, or for the jump
itself to fail, which would be a catastrophic error and necessitate that
the IPL is aborted entirely anyway. There is no condition such that we
may update the cert list or IIRB and then realize we actually should try
booting from a different device instead.
Regards,
Jared Rossi
Re: [PATCH v16 23/33] pc-bios/s390-ccw: Add signature verification for secure IPL in audit mode
On 7/7/26 19:40, 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.
>
> 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
> Reviewed-by: Eric Farman
> Reviewed-by: Jared Rossi
Mostly a few nits below, but one important piece regarding the placement
of the update_iirb() call.
> ---
> docs/system/s390x/secure-ipl.rst | 15 ++
> hw/s390x/ipl.c | 9 +
> pc-bios/s390-ccw/Makefile| 2 +-
> pc-bios/s390-ccw/bootmap.c | 27 +++
> pc-bios/s390-ccw/bootmap.h | 9 +
> pc-bios/s390-ccw/jump2ipl.c | 7 +
> pc-bios/s390-ccw/main.c | 19 +-
> pc-bios/s390-ccw/s390-ccw.h | 20 ++
> pc-bios/s390-ccw/sclp.c | 27 +++
> pc-bios/s390-ccw/sclp.h | 6 +
> pc-bios/s390-ccw/secure-ipl.c| 363 +++
> pc-bios/s390-ccw/secure-ipl.h| 115 ++
> 12 files changed, 617 insertions(+), 2 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 9d7d33f5ed..cf6ccf5d57 100644
> --- a/docs/system/s390x/secure-ipl.rst
> +++ b/docs/system/s390x/secure-ipl.rst
> @@ -39,3 +39,18 @@ Configuration:
> .. code-block:: shell
>
> qemu-system-s390x -machine s390-ccw-virtio ...
> +
> +Audit Mode
> +^^
> +
> +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/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 85fe2d3cb4..d0dbf47d74 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -828,6 +828,15 @@ void s390_ipl_prepare_cpu(S390CPU *cpu)
> cpu->env.psw.addr = ipl->bios_start_addr;
> if (!ipl->iplb_valid) {
> ipl->iplb_valid = s390_init_all_iplbs(ipl);
> +
> +/*
> + * Secure IPL without specifying a boot device.
> + * IPLB is not generated if no boot device is defined.
> + */
> +if (s390_has_certificate() && !ipl->iplb_valid) {
> +error_report("No boot device defined for Secure IPL");
> +exit(1);
> +}
> } else {
> ipl->qipl.chain_len = 0;
> }
> diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile
> index 3e5dfb64d5..2109d16781 100644
> --- a/pc-bios/s390-ccw/Makefile
> +++ b/pc-bios/s390-ccw/Makefile
> @@ -35,7 +35,7 @@ QEMU_DGFLAGS = -MMD -MP -MT $@ -MF $(@D)/$(*F).d
>
> 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-ccw.o clp.o pci.o virtio-pci.o
> + virtio-ccw.o clp.o pci.o virtio-pci.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 7791ca179a..276080709d 100644
> --- a/pc-bios/s390-ccw/bootmap.c
> +++ b/pc-bios/s390-ccw/bootmap.c
> @@ -10,11 +10,13 @@
>
> #include
> #include
> +#include
> #include "s390-ccw.h"
> #include "s390-arch.h"
> #include "bootmap.h"
> #include "virtio.h"
> #include "bswap.h"
> +#include "secure-ipl.h"
>
> #ifdef DEBUG
> /* #define DEBUG_FALLBACK */
> @@ -710,6 +712,9 @@ static int zipl_run(ScsiBlockPtr *pte)
> ComponentHeader *header;
> ComponentEntry *entry;
> uint8_t tmp_sec[MAX_SECTOR_SIZE];
> +IplDeviceComponentList comp_list = { 0 };
> +IplSignatureCertificateList cert_list = { 0 };
> +uint8_t *tmp_cert_buf = NULL;
> int rc;
>
> if (virtio_read(pte->blockno, tmp_sec)) {
> @@ -736,6 +741,9 @@ static int zipl_run(ScsiBlockPtr *pte)
> case ZIPL_BOOT_MODE_NORMAL:
> rc = zipl_run_normal(&entry, tmp_sec);
> break;
> +case ZIPL_BOOT_MODE_SECURE_AUDIT:
> +rc = zipl_run_secure(&entry, tmp_sec, &comp_list, &cert_list,
> &tmp_cert_buf);
> +break;
> default:
> panic("Unknown boot mode");
> }
> @@ -751,6 +759,13 @@ static int zipl_run(ScsiBlockPtr *pte)
>
> /* should not return */
>
