Re: [PATCH v6 21/28] pc-bios/s390-ccw: Add additional security checks for secure boot

[Zhuoying Cai <zycai@linux.ibm.com>]

Thank you for all the feedback! I'll address the comments in the next
version.

On 9/29/25 9:30 AM, Thomas Huth wrote:
> On 18/09/2025 01.21, Zhuoying Cai wrote:
>> Add additional checks to ensure that components do not overlap with
>> signed components when loaded into memory.
>>
>> Add additional checks to ensure the load addresses of unsigned components
>> are greater than or equal to 0x2000.
>>
>> When the secure IPL code loading attributes facility (SCLAF) is installed,
>> all signed components must contain a secure code loading attributes block
>> (SCLAB).
>>
>> The SCLAB provides further validation of information on where to load the
>> signed binary code from the load device, and where to start the execution
>> of the loaded OS code.
>>
>> When SCLAF is installed, its content must be evaluated during secure IPL.
>> However, a missing SCLAB will not be reported in audit mode. The SCALB
>> checking will be skipped in this case.
>>
>> Add IPL Information Error Indicators (IIEI) and Component Error
>> Indicators (CEI) for IPL Information Report Block (IIRB).
>>
>> When SCLAF is installed, additional secure boot checks are performed
>> during zipl and store results of verification into IIRB.
>>
>> Signed-off-by: Zhuoying Cai <zycai@linux.ibm.com>
>> ---
> ...
>> diff --git a/pc-bios/s390-ccw/secure-ipl.c b/pc-bios/s390-ccw/secure-ipl.c
>> index 8eab19cb09..cd798c1198 100644
>> --- a/pc-bios/s390-ccw/secure-ipl.c
>> +++ b/pc-bios/s390-ccw/secure-ipl.c

[ ... ]

>> +
>> +static bool is_psw_valid(uint64_t psw, SecureIplCompAddrRange *comp_addr_range,
>> +                         int range_index)
>> +{
>> +    uint32_t addr = psw & 0x3FFFFFFF;
> 
> Shouldn't that be 0x7fffffff instead?
> 

Thanks for pointing it out. It should be 0x7fffffff.

>> +    /* PSW points within a signed binary code component */
>> +    for (int i = 0; i < range_index; i++) {
>> +        if (comp_addr_range[i].is_signed &&
>> +            addr >= comp_addr_range[i].start_addr &&
>> +            addr <= comp_addr_range[i].end_addr) {
> 
> is it still OK if the address points to the end_addr? Or should that be 
> end_addr - 2 instead (since an opcode has at least two bytes)?
> 

Using end_addr - 2 seems correct, since it accounts for the minimum
instruction length.

Just to clarify: using end_addr - 2 should ensure that at least a 2-byte
instruction fits. Should longer instructions (e.g., 4 and 6 bytes) be a
concern in this context?

>> +            return true;
>> +       }
>> +    }
>> +
>> +    return false;
>> +}
> ...
>>   
>> +static inline bool is_sclab_flag_set(uint16_t sclab_flags, uint16_t flag)
>> +{
>> +    return (sclab_flags & flag) != 0;
>> +}
>> +
>> +static inline bool validate_unsigned_addr(uint64_t comp_load_addr)
>> +{
>> +    /* usigned load address must be greater than or equal to 0x2000 */
>> +    return comp_load_addr >= 0x2000;
>> +}
>> +
>> +static inline bool validate_sclab_magic(uint8_t *sclab_magic)
>> +{
>> +    /* identifies the presence of SCLAB */
>> +    return magic_match(sclab_magic, ZIPL_MAGIC);
>> +}
>> +
>> +static inline bool validate_sclab_length(uint16_t sclab_len)
>> +{
>> +    /* minimum SCLAB length is 32 bytes */
>> +    return sclab_len >= 32;
>> +}
>> +
>> +static inline bool validate_sclab_format(uint8_t sclab_format)
>> +{
>> +    /* SCLAB format must set to zero, indicating a format-0 SCLAB being used */
>> +    return sclab_format == 0;
>> +}
>> +
>> +static inline bool validate_sclab_ola_zero(uint64_t sclab_load_addr)
>> +{
>> +    /* Load address field in SCLAB must contain zeros */
>> +    return sclab_load_addr == 0;
>> +}
>> +
>> +static inline bool validate_sclab_ola_one(uint64_t sclab_load_addr,
>> +                                          uint64_t comp_load_addr)
>> +{
>> +   /* Load address field must match storage address of the component */
>> +   return sclab_load_addr == comp_load_addr;
>> +}
>> +
>> +static inline bool validate_sclab_opsw_zero(uint64_t sclab_load_psw)
>> +{
>> +    /* Load PSW field in SCLAB must contain zeros */
>> +    return sclab_load_psw == 0;
>> +}
>>
>> +static inline bool validate_sclab_opsw_one(uint16_t sclab_flags)
>> +{
>> +   /* OLA must set to one */
>> +   return is_sclab_flag_set(sclab_flags, S390_SECURE_IPL_SCLAB_FLAG_OLA);
>> +}
>> +
>> +static inline bool validate_lpsw(uint64_t sclab_load_psw, uint64_t comp_load_psw)
>> +{
>> +    /* compare load PSW with the PSW specified in component */
>> +    return sclab_load_psw == comp_load_psw;
>> +}
> 
> Most of these inline functions just compare something with 0 or other values 
> here, and you only use them in one spot of the code ... So you need 5 lines 
> of code for something that could be done in two lines of code at the calling 
> sites instead, i.e. this looks like unnecessary code to me. Please inline 
> the comparisons (together with the comment that you've got here) in the 
> calling sites to get rid of this code bloat.
> 
>   Thanks,
>    Thomas
>