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

[Thomas Huth <thuth@redhat.com>]

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
> @@ -202,6 +202,12 @@ static bool secure_ipl_supported(void)
>           return false;
>       }
>   
> +    if (!sclp_is_sclaf_on()) {
> +        puts("Secure IPL Code Loading Attributes Facility is not supported by" \

No need for the backslash here.

> +             " the hypervisor!");
> +        return false;
> +    }
> +
>       return true;
>   }
>   
> @@ -214,6 +220,393 @@ static void init_lists(IplDeviceComponentList *comps, IplSignatureCertificateLis
>       certs->ipl_info_header.len = sizeof(certs->ipl_info_header);
>   }
>   
> +static bool is_comp_overlap(SecureIplCompAddrRange *comp_addr_range, int addr_range_index,

I'd suggest to move the second parameter to a separate line, to keep the 
line length below 80 columns.

> +                            uint64_t start_addr, uint64_t end_addr)
> +{
> +    /* neither a signed nor an unsigned component can overlap with a signed component */
> +    for (int i = 0; i < addr_range_index; i++) {
> +        if ((comp_addr_range[i].start_addr <= end_addr &&
> +            start_addr <= comp_addr_range[i].end_addr) &&
> +            comp_addr_range[i].is_signed) {
> +            return true;
> +       }
> +    }
> +
> +    return false;
> +}
> +
> +static void comp_addr_range_add(SecureIplCompAddrRange *comp_addr_range,
> +                                int addr_range_index, bool is_signed,
> +                                uint64_t start_addr, uint64_t end_addr)
> +{
> +    if (addr_range_index > MAX_CERTIFICATES - 1) {

That error goes completely unnoticed. Should there be a log / warning here?

> +        return;
> +    }
> +
> +    comp_addr_range[addr_range_index].is_signed = is_signed;
> +    comp_addr_range[addr_range_index].start_addr = start_addr;
> +    comp_addr_range[addr_range_index].end_addr = end_addr;
> +}
> +
> +static void check_unsigned_addr(uint64_t load_addr, IplDeviceComponentList *comps,
> +                                int comp_index)
> +{
> +    uint32_t flag;
> +    const char *msg;
> +    bool valid;
> +
> +    valid = validate_unsigned_addr(load_addr);
> +    if (!valid) {
> +        flag = S390_IPL_COMPONENT_CEI_INVALID_UNSIGNED_ADDR;
> +        msg = "Load address is less than 0x2000";
> +        set_cei_with_log(comps, comp_index, flag, msg);

I'd maybe rather pass the string directly as parameter, without the detour 
through the "msg" variable.

> +    }
> +}
> +
> +static void addr_overlap_check(SecureIplCompAddrRange *comp_addr_range,
> +                               int *addr_range_index,
> +                               uint64_t start_addr, uint64_t end_addr, bool is_signed)
> +{
> +    bool overlap;
> +
> +    overlap = is_comp_overlap(comp_addr_range, *addr_range_index,
> +                              start_addr, end_addr);
> +    if (!overlap) {
> +        comp_addr_range_add(comp_addr_range, *addr_range_index, is_signed,
> +                            start_addr, end_addr);
> +        *addr_range_index += 1;
> +    } else {
> +        zipl_secure_handle("Component addresses overlap");
> +    }
> +}
> +
> +static bool check_sclab_presence(uint8_t *sclab_magic,
> +                                 IplDeviceComponentList *comps, int comp_index)
> +{
> +    if (!validate_sclab_magic(sclab_magic)) {
> +        comps->device_entries[comp_index].cei |= S390_IPL_COMPONENT_CEI_INVALID_SCLAB;
> +
> +        /* a missing SCLAB will not be reported in audit mode */
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
> +static void check_sclab_length(uint16_t sclab_len,
> +                               IplDeviceComponentList *comps, int comp_index)
> +{
> +    const char *msg;
> +    uint32_t flag;
> +    bool valid;
> +
> +    valid = validate_sclab_length(sclab_len);
> +    if (!valid) {
> +        flag = S390_IPL_COMPONENT_CEI_INVALID_SCLAB_LEN |
> +               S390_IPL_COMPONENT_CEI_INVALID_SCLAB;
> +        msg = "Invalid SCLAB length";
> +        set_cei_with_log(comps, comp_index, flag, msg);

Again, pass string directly, without msg variable?

> +    }
> +}
> +
> +static void check_sclab_format(uint8_t sclab_format,
> +                               IplDeviceComponentList *comps, int comp_index)
> +{
> +    const char *msg;
> +    uint32_t flag;
> +    bool valid;
> +
> +    valid = validate_sclab_format(sclab_format);
> +    if (!valid) {
> +        flag = S390_IPL_COMPONENT_CEI_INVALID_SCLAB_FORMAT;
> +        msg = "Format-0 SCLAB is not being use";
> +        set_cei_with_log(comps, comp_index, flag, msg);

dito

> +    }
> +}
> +
> +static void check_sclab_opsw(SecureCodeLoadingAttributesBlock *sclab,
> +                             SecureIplSclabInfo *sclab_info,
> +                             IplDeviceComponentList *comps, int comp_index)
> +{
> +    const char *msg;
> +    uint32_t flag;
> +    bool is_opsw_set;
> +    bool valid;
> +
> +    is_opsw_set = is_sclab_flag_set(sclab->flags, S390_SECURE_IPL_SCLAB_FLAG_OPSW);
> +    if (!is_opsw_set) {
> +        valid = validate_sclab_opsw_zero(sclab->load_psw);
> +        if (!valid) {
> +            flag = S390_IPL_COMPONENT_CEI_SCLAB_LOAD_PSW_NOT_ZERO;
> +            msg = "Load PSW is not zero when Override PSW bit is zero";
> +            set_cei_with_log(comps, comp_index, flag, msg);
> +        }
> +    } else {
> +        /* OPSW = 1 indicating global SCLAB */
> +        valid = validate_sclab_opsw_one(sclab->flags);
> +        if (!valid) {
> +            flag = S390_IPL_COMPONENT_CEI_SCLAB_OLA_NOT_ONE;
> +            msg = "Override Load Address bit is not set to one in the global SCLAB";
> +            set_cei_with_log(comps, comp_index, flag, msg);

Is it ok here to continue with the code below, even if it was not valid? Or 
should there be a return statement here?

> +        }
> +
> +        sclab_info->global_count += 1;
> +        if (sclab_info->global_count == 1) {
> +            sclab_info->load_psw = sclab->load_psw;
> +            sclab_info->flags = sclab->flags;
> +        }
> +    }
> +}
> +
> +static void check_sclab_ola(SecureCodeLoadingAttributesBlock *sclab,
> +                            uint64_t load_addr, IplDeviceComponentList *comps,
> +                            int comp_index)
> +{
> +    const char *msg;
> +    uint32_t flag;
> +    bool is_ola_set;
> +    bool valid;
> +
> +    is_ola_set = is_sclab_flag_set(sclab->flags, S390_SECURE_IPL_SCLAB_FLAG_OLA);
> +    if (!is_ola_set) {
> +        valid = validate_sclab_ola_zero(sclab->load_addr);
> +        if (!(valid)) {

No need for the inner braces here.

> +            flag = S390_IPL_COMPONENT_CEI_SCLAB_LOAD_ADDR_NOT_ZERO;
> +            msg = "Load Address is not zero when Override Load Address bit is zero";
> +            set_cei_with_log(comps, comp_index, flag, msg);
> +        }
> +
> +    } else {
> +        valid = validate_sclab_ola_one(sclab->load_addr, load_addr);
> +        if (!valid) {
> +            flag = S390_IPL_COMPONENT_CEI_UNMATCHED_SCLAB_LOAD_ADDR;
> +            msg = "Load Address does not match with component load address";
> +            set_cei_with_log(comps, comp_index, flag, msg);
> +        }
> +    }
> +}
> +
> +static void check_sclab_nuc(uint16_t sclab_flags, IplDeviceComponentList *comps,
> +                            int comp_index)
> +{
> +    const char *msg;
> +    uint32_t flag;
> +    bool is_nuc_set;
> +    bool is_global_sclab;
> +
> +    is_nuc_set = is_sclab_flag_set(sclab_flags, S390_SECURE_IPL_SCLAB_FLAG_NUC);
> +    is_global_sclab = is_sclab_flag_set(sclab_flags, S390_SECURE_IPL_SCLAB_FLAG_OPSW);
> +    if (is_nuc_set && !is_global_sclab) {
> +        flag = S390_IPL_COMPONENT_CEI_NUC_NOT_IN_GLOBAL_SCLA;
> +        msg = "No Unsigned Components bit is set, but not in the global SCLAB";
> +        set_cei_with_log(comps, comp_index, flag, msg);
> +    }
> +}
> +
> +static void check_sclab_sc(uint16_t sclab_flags, IplDeviceComponentList *comps,
> +                           int comp_index)
> +{
> +    const char *msg;
> +    uint32_t flag;
> +    bool is_sc_set;
> +    bool is_global_sclab;
> +
> +    is_sc_set = is_sclab_flag_set(sclab_flags, S390_SECURE_IPL_SCLAB_FLAG_SC);
> +    is_global_sclab = is_sclab_flag_set(sclab_flags, S390_SECURE_IPL_SCLAB_FLAG_OPSW);
> +    if (is_sc_set && !is_global_sclab) {
> +        flag = S390_IPL_COMPONENT_CEI_SC_NOT_IN_GLOBAL_SCLAB;
> +        msg = "Single Component bit is set, but not in the global SCLAB";
> +        set_cei_with_log(comps, comp_index, flag, msg);
> +    }
> +}
> +
> +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?

> +    /* 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)?

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