Re: [PATCH v12 23/35] pc-bios/s390-ccw: Add signature verification for secure IPL in audit mode

[Jared Rossi <jrossi@linux.ibm.com>]

On 7/2/26 11:26 AM, Zhuoying Cai wrote:
> 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 <zycai@linux.ibm.com>
>>> ---
>>>    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.

That check is good.  I think it could be a compile time check though 
since everything is constant.

I guess the reason this feels fragile is because the size of the 
iplb_chain array determines the size of the ipl_data area, and the chain 
array size itself is determined by a magic number 7 in hw/ipl.c.  We 
need to change that line anyway to initialize the chain to zero as Eric 
pointed out, so lets also use MAX_BOOT_DEVS - 1 instead of just magic 
7.  This way we can also add another compile time check:

(sizeof(IplParameterBlock) * (MAX_BOOT_DEVS - 1)) == CERT_BUF_SIZE

which will ensure that any potential changes to the either the cert 
buffer size and/or number of allowed boot devices will be kept in sync 
for the shared buffer area.

>
>> 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

Thanks,
Jared Rossi