On Thu, 2026-07-02 at 16:47 -0400, Zhuoying Cai wrote:
> 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 <[email protected]>
> > > ---
> > > 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!
Agreed. The flag checks apply regardless of format, which I overlooked
previously.
>
> > > +
> > > + 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.
Works for me. Thanks!
>
> > >
> > > *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;
