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.
