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