Re: [PATCH v12 18/35] hw/s390x/ipl: Rework s390_ipl_map_iplb_chain for certificate storage

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

On 7/2/26 10:56 AM, Eric Farman wrote:
> On Wed, 2026-07-01 at 16:49 -0400, Zhuoying Cai wrote:
>> Rework s390_ipl_map_iplb_chain to always allocate maximum memory for
>> the IPLB chain, regardless of the number of boot devices. This space
>> is also used to store certificates during secure boot, providing a
>> safe location for certificates until the kernel reads them during boot.
>>
>> Rename next_iplb to ipl_data to better reflect its multiple purposes:
>> storing both IPLB chains and certificate data.
>>
>> Signed-off-by: Zhuoying Cai <zycai@linux.ibm.com>
>> ---
>>   hw/s390x/ipl.c              | 13 ++++++++++---
>>   include/hw/s390x/ipl/qipl.h |  2 +-
>>   pc-bios/s390-ccw/iplb.h     |  4 ++--
>>   3 files changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index 9cb41511e8..7734b6edc7 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -425,10 +425,9 @@ static S390PCIBusDevice *s390_get_pci_device(DeviceState *dev_st, int *devtype)
>>       return pbdev;
>>   }
>>   
>> -static uint64_t s390_ipl_map_iplb_chain(IplParameterBlock *iplb_chain)
>> +static uint64_t s390_ipl_map_iplb_chain(IplParameterBlock *iplb_chain, uint16_t count)
>>   {
>>       S390IPLState *ipl = get_ipl_device();
>> -    uint16_t count = be16_to_cpu(ipl->qipl.chain_len);
>>       uint64_t len = sizeof(IplParameterBlock) * count;
>>       uint64_t chain_addr = find_iplb_chain_addr(ipl->bios_start_addr, count);
>>   
>> @@ -629,8 +628,16 @@ static bool s390_init_all_iplbs(S390IPLState *ipl)
>>               dev_st = get_boot_device(i);
>>               s390_build_iplb(dev_st, &iplb_chain[i - 1]);
>>           }
>> +    }
>>   
>> -        ipl->qipl.next_iplb = cpu_to_be64(s390_ipl_map_iplb_chain(iplb_chain));
>> +    /*
>> +     * Allocate maximum space for IPLB chain and/or certificate storage.
>> +     * Once a valid boot device is found, this space will be used to store
>> +     * certificates if secure boot is enabled.
>> +     */
>> +    if (iplb_num > 1 || s390_has_certificate()) {
>> +        ipl->qipl.ipl_data = cpu_to_be64(s390_ipl_map_iplb_chain(iplb_chain,
>> +                                                                 MAX_BOOT_DEVS - 1));
> I think iplb_chain needs to be explicitly initialized to zero now that you're allocating a larger
> space. load_next_iplb() is still honoring the length of the count when walking the IPLBs, but the
> above does a memcpy of the whole array.
>

+1.  Eric is correct.  I missed this in my review.  We copy all 7 IPLB 
chain indices, but build_iplb() only populates the indices it actually 
needs and the array isn't automatically cleared when it is first 
declared, so it could potentially contain garbage at the end.

Thanks,
Jared Rossi