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 <[email protected]>
No need to change where you placed the chunk of code above. The rest of
my comments are nits.
--
Regards,
Collin
