Thanks for the feedback! On 7/2/26 10:13 AM, Jared Rossi wrote: > > > On 7/1/26 4:49 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. >> >> 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 | 15 ++ >> 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 | 18 +- >> 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 | 362 +++++++++++++++++++++++++++++++ >> pc-bios/s390-ccw/secure-ipl.h | 116 ++++++++++ >> 11 files changed, 607 insertions(+), 2 deletions(-) >> create mode 100644 pc-bios/s390-ccw/secure-ipl.c >> create mode 100644 pc-bios/s390-ccw/secure-ipl.h >> > [...] >> + >> +void update_cert_list(IplSignatureCertificateList *cert_list) >> +{ >> + IplSignatureCertificateEntry *cert_entry; >> + uint8_t *cert_buf; >> + >> + cert_buf = (uint8_t *)qipl.ipl_data; >> + >> + for_each_rb_entry(cert_entry, cert_list) { >> + memcpy(cert_buf, (uint8_t *)cert_entry->addr, cert_entry->len); >> + cert_entry->addr = (uint64_t)cert_buf; >> + cert_buf += cert_entry->len; >> + } >> +} > > This looks like the correct idea but I think it needs additional > validation. Firstly, as Eric mentioned, there is no check that the size > of our cert entries don't exceed the qipl.ipl_data space. I know there > are implicit factors such as maximum certificate size and such that > limit it in a way that it shouldn't be possible, but we should still check. >
At the beginning of zipl_run_secure(), I added a check to ensure that there is enough space to store the certificates required for the current IPL. We store the certificates directly in the qipl.ipl_data area rather than storing cert entries (IplSignatureCertificateEntry) in qipl.ipl_data. As discussed previously and documented here (<https://www.ibm.com/docs/en/linux-on-systems?topic=keys-required-signatures>), an IPL requires at most two, and possibly three, signed components, meaning that at most three certificates are needed. Based on that, I check that MAX_SIGNED_COMP * CERT_BUF_MAX_LEN does not exceed CERT_BUF_SIZE, ensuring there is sufficient space for the certificates used by the current IPL. Please let me know if you think any additional validation is needed. > Secondly and more critically, you need to reset the qipl.ipl_data > address to it's initial value before reusing it. I don't think I saw > that done anywhere. The address is incremented after each IPLB is > loaded from the chain (see the load_next_iplb() function in iplb.h), so > if we in theory had started with 7 IPLBs in the chain and loaded all of > them, and if we then copied the maximum allowed size of certificate > entry data, we would exceed the qipl.ipl_data upper limit and probably > clobber the kernel. > > I think the easiest way to restore the qipl.ipl_data address to its > initial value is to track how many IPLBs we've used, multiply that by > the size of the IPLB, and then move the address back by that amount. > Thanks for pointing this out. I missed resetting the qipl.ipl_data address before reusing it. I'll restore it to its initial value before copying the certificate data. Thanks! > Regards, > Jared Rossi
