Re: [PATCH v12 26/35] pc-bios/s390-ccw: Add additional security checks for secure boot

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

Thanks for the review!

On 7/2/26 12:08 PM, Eric Farman wrote:
> On Wed, 2026-07-01 at 16:49 -0400, 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.
>>
>> 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>
>> ---
>>  include/hw/s390x/ipl/qipl.h   |  29 +++++-
>>  pc-bios/s390-ccw/sclp.h       |   1 +
>>  pc-bios/s390-ccw/secure-ipl.c | 170 +++++++++++++++++++++++++++++++++-
>>  pc-bios/s390-ccw/secure-ipl.h |  51 ++++++++++
>>  4 files changed, 246 insertions(+), 5 deletions(-)
>>

[...]

>> +static void check_sclab(SclaBlock **global_sclab,
>> +                        IplDeviceComponentEntry *comp_entry,
>> +                        IplInfoBlockHeader *comp_list_hdr)
>> +{
>> +    SclabOriginLocator *sclab_locator;
>> +    SclaBlock *sclab;
>> +
>> +    /* sclab locator is located at the last 8 bytes of the signed comp */
>> +    sclab_locator = (SclabOriginLocator *)(comp_entry->addr +
>> +                                           comp_entry->len - 8);
> 
> Need to verify that len >=8, otherwise who knows what gets read.
> 
>> +
>> +    /* return early if sclab does not exist */
>> +    zipl_secure_validate(magic_match(sclab_locator->magic, ZIPL_MAGIC),
>> +                         &comp_entry->cei, S390_CEI_INVALID_SCLAB,
>> +                         "Magic does not match. SCLAB does not exist");
>> +
>> +    if (comp_entry->cei & S390_CEI_INVALID_SCLAB) {
>> +        return;
>> +    }
>> +
>> +    zipl_secure_validate(sclab_locator->len >= S390_SCLAB_MIN_LEN, &comp_entry->cei,
>> +                         S390_CEI_INVALID_SCLAB_LEN | S390_CEI_INVALID_SCLAB,
>> +                         "Invalid SCLAB length");
>> +
>> +    /* return early if sclab is invalid */
>> +    if (comp_entry->cei & S390_CEI_INVALID_SCLAB) {
>> +        return;
>> +    }
> 
> I get why these two conditions (missing/invalid SCLAB) cause us to stop processing when in audit
> mode...
> 
>> +
>> +    sclab = (SclaBlock *)(comp_entry->addr + comp_entry->len -
>> +                          sclab_locator->len);
>> +
>> +    zipl_secure_validate(sclab->format == 0, &comp_entry->cei,
>> +                         S390_CEI_INVALID_SCLAB_FORMAT,
>> +                         "Format-0 SCLAB is not being used");
> 
> ...but why doesn't this one (SCLAB format)? Do the subsequent flags checks all apply regardless of
> format?
> 

Continuing with the flag checks seems reasonable to me, since we're
still dealing with some form of valid SCLAB. Please let me know your
thoughts. Thanks!

>> +
>> +    if (!(sclab->flags & S390_SCLAB_OPSW)) {
>> +        /* OPSW = 0 - Load PSW field in SCLAB must contain zeros */
>> +        zipl_secure_validate(sclab->load_psw == 0, &comp_entry->cei,
>> +                             S390_CEI_SCLAB_LOAD_PSW_NOT_ZERO,
>> +                             "Load PSW is not zero when Override PSW bit is zero");
>> +    } else {
>> +        /* OPSW = 1 indicating global SCLAB */
>> +        if (*global_sclab) {
>> +            comp_list_hdr->iiei |= S390_IIEI_MORE_GLOBAL_SCLAB;
>> +            zipl_secure_error("More than one global SCLAB");
>> +        }
>> +        *global_sclab = sclab;
>> +
>> +        /* override load address flag must set to one */
>> +        zipl_secure_validate(sclab->flags & S390_SCLAB_OLA, &comp_entry->cei,
>> +                             S390_CEI_SCLAB_OLA_NOT_ONE,
>> +                             "OLA flag is not set to one in the global SCLAB");
>> +    }
>> +
>> +    if (!(sclab->flags & S390_SCLAB_OLA)) {
>> +        /* OLA = 0 - Load address field in SCLAB must contain zeros */
>> +        zipl_secure_validate(sclab->load_addr == 0, &comp_entry->cei,
>> +                             S390_CEI_SCLAB_LOAD_ADDR_NOT_ZERO,
>> +                             "Load Address is not zero when OLA flag is zero");
>> +    } else {
>> +        /* OLA = 1 - Load address field must match storage address of the component */
>> +        zipl_secure_validate(sclab->load_addr == comp_entry->addr, &comp_entry->cei,
>> +                             S390_CEI_UNMATCHED_SCLAB_LOAD_ADDR,
>> +                             "Load Address does not match with component load address");
>> +    }
>> +
>> +    zipl_secure_validate(~sclab->flags & S390_SCLAB_NUC || sclab->flags & S390_SCLAB_OPSW,
>> +                         &comp_entry->cei, S390_CEI_NUC_NOT_IN_GLOBAL_SCLAB,
>> +                         "NUC bit is set, but not in the global SCLAB");
>> +
>> +    zipl_secure_validate(~sclab->flags & S390_SCLAB_SC || sclab->flags & S390_SCLAB_OPSW,
>> +                         &comp_entry->cei, S390_CEI_SC_NOT_IN_GLOBAL_SCLAB,
>> +                         "SC bit is set, but not in the global SCLAB");
>> +}
>> +
>>  static int zipl_load_signature(ComponentEntry *entry, uint64_t sig)
>>  {
>>      if (entry->compdat.sig_info.format != DER_SIGNATURE_FORMAT) {
>> @@ -268,6 +415,8 @@ int zipl_run_secure(ComponentEntry **entry_ptr, uint8_t *tmp_sec,
>>      uint8_t *tmp_buf;
>>      bool verified;
>>      bool signed_found = false;
>> +    bool sclab_found = false;
>> +    SclaBlock *global_sclab = NULL;
>>  
>>      if ((MAX_SIGNED_COMP * CERT_BUF_MAX_LEN) > (CERT_BUF_SIZE)) {
>>          panic("Not enough memory to store certificates");
>> @@ -308,6 +457,10 @@ int zipl_run_secure(ComponentEntry **entry_ptr, uint8_t *tmp_sec,
>>  
>>              /* no signature present (unsigned component) */
>>              if (!sig_entry.len) {
>> +                zipl_secure_validate(comp_entry.addr >= S390_UNSIGNED_MIN_ADDR,
>> +                            &comp_entry.cei, S390_CEI_INVALID_UNSIGNED_ADDR,
>> +                            "Load address for unsigned component is less than 0x2000");
>> +
>>                  comp_list_add(comp_list, comp_entry);
>>                  break;
>>              }
>> @@ -319,6 +472,9 @@ int zipl_run_secure(ComponentEntry **entry_ptr, uint8_t *tmp_sec,
>>              comp_entry.flags = S390_IPL_DEV_COMP_FLAG_SC;
>>              signed_found = true;
>>  
>> +            check_sclab(&global_sclab, &comp_entry, &comp_list->ipl_info_header);
>> +            sclab_found |= !(comp_entry.cei & S390_CEI_INVALID_SCLAB);
>> +
>>              cert_entry = (IplSignatureCertificateEntry) { 0 };
>>              verified = verify_signature(comp_entry, sig_entry,
>>                                          &cert_entry.len, &cert_table_idx);
>> @@ -364,9 +520,17 @@ int zipl_run_secure(ComponentEntry **entry_ptr, uint8_t *tmp_sec,
>>          }
>>      }
>>  
>> -    if (!signed_found) {
>> -        zipl_secure_error("Secure boot is on, but components are not signed");
>> -    }
>> +    zipl_secure_validate(signed_found, &comp_list->ipl_info_header.iiei,
>> +                         S390_IIEI_NO_SIGNED_COMP,
>> +                         "Secure boot is on, but components are not signed");
>> +
>> +    zipl_secure_validate(sclab_found, &comp_list->ipl_info_header.iiei,
>> +                         S390_IIEI_NO_SCLAB, "No recognizable SCLAB");
>> +
>> +    comp_entry = (IplDeviceComponentEntry){ 0 };
>> +    comp_entry.addr = entry->compdat.load_psw;
>> +    check_global_sclab(global_sclab, &comp_entry, comp_list);
>> +    comp_list_add(comp_list, comp_entry);
> 
> Do we actually need to add an (empty) entry when global_sclab wasn't found?
> 

I think we should still add this entry even when global_sclab is not
found, since the comp_entry would still contain the PSW value from the
EXEC entry.

>>  
>>      *entry_ptr = entry;
>>      free((void *)sig_entry.addr);
>> diff --git a/pc-bios/s390-ccw/secure-ipl.h b/pc-bios/s390-ccw/secure-ipl.h
>> index 1b1287858b..1bacb4987a 100644
>> --- a/pc-bios/s390-ccw/secure-ipl.h
>> +++ b/pc-bios/s390-ccw/secure-ipl.h
>> @@ -27,6 +27,33 @@ int zipl_run_secure(ComponentEntry **entry_ptr, uint8_t *tmp_sec,
>>                      IplSignatureCertificateList *cert_list,
>>                      uint8_t **tmp_cert_buf);
>>  
>> +#define S390_SCLAB_OPSW    0x8000   /* override PSW flag */
>> +#define S390_SCLAB_OLA     0x4000   /* override load address flag */
>> +#define S390_SCLAB_NUC     0x2000   /* no unsigned components flag */
>> +#define S390_SCLAB_SC      0x1000   /* single component flag */
>> +
>> +#define S390_SCLAB_MIN_LEN      32
>> +#define S390_UNSIGNED_MIN_ADDR  0x2000
>> +
>> +/* Secure Code Loading Attributes Block */
>> +struct SclaBlock {
>> +    uint8_t  format;
>> +    uint8_t  reserved1;
>> +    uint16_t flags;
>> +    uint8_t  reserved2[4];
>> +    uint64_t load_psw;
>> +    uint64_t load_addr;
>> +    uint64_t reserved3[];
>> +} __attribute__ ((packed));
>> +typedef struct SclaBlock SclaBlock;
>> +
>> +struct SclabOriginLocator {
>> +    uint8_t reserved[2];
>> +    uint16_t len;
>> +    uint8_t magic[4];
>> +} __attribute__ ((packed));
>> +typedef struct SclabOriginLocator SclabOriginLocator;
>> +
>>  static inline void zipl_secure_error(const char *message)
>>  {
>>      switch (boot_mode) {
>> @@ -38,6 +65,30 @@ static inline void zipl_secure_error(const char *message)
>>      }
>>  }
>>  
>> +static inline void zipl_secure_validate_u16(bool condition, uint16_t *flags,
>> +                                            uint16_t flag, const char *message)
>> +{
>> +    if (!condition) {
>> +        *flags |= flag;
>> +        zipl_secure_error(message);
>> +    }
>> +}
>> +
>> +static inline void zipl_secure_validate_u32(bool condition, uint32_t *flags,
>> +                                            uint32_t flag, const char *message)
>> +{
>> +    if (!condition) {
>> +        *flags |= flag;
>> +        zipl_secure_error(message);
>> +    }
>> +}
>> +
>> +#define zipl_secure_validate(condition, flags, flag, message)   \
>> +    _Generic((flags),                                           \
>> +        uint16_t * : zipl_secure_validate_u16,                  \
>> +        uint32_t * : zipl_secure_validate_u32                   \
>> +    )(condition, flags, flag, message)
>> +
>>  static inline uint64_t _diag320(void *data, unsigned long subcode)
>>  {
>>      register unsigned long addr asm("0") = (unsigned long)data;