On 5/5/26 16:18, 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]>
You've done an awesome job at cleaning up the large list of parameters
passed to some functions, and I think it's bringing to light some
cleaner approaches to the SCLAF code.
Here's what I'm thinking:
- process each SCLAB at the same time a component is processed (already
done here)
- Use a SclaBlock global_sclab variable to hold on to the global SCLAB
(instead of using an info struct)
- Once all components are processed, then check_global_sclab (already
done here)
- While checking the global SCLAB, iterate through the component list
to tally the signed / unsigned counts (by checking the component flags)
and set errors depending on the SCLAB flags.
This will get rid of the SecureIplSclabInfo struct and the signed /
unsigned counter fields can be contained within the scope of the
check_global_sclab function.
> ---
> include/hw/s390x/ipl/qipl.h | 29 ++++-
> pc-bios/s390-ccw/sclp.h | 1 +
> pc-bios/s390-ccw/secure-ipl.c | 210 +++++++++++++++++++++++++++++++++-
> pc-bios/s390-ccw/secure-ipl.h | 62 ++++++++++
> 4 files changed, 296 insertions(+), 6 deletions(-)
>
> diff --git a/include/hw/s390x/ipl/qipl.h b/include/hw/s390x/ipl/qipl.h
> index a2180719b1..2a3ae6b9f5 100644
> --- a/include/hw/s390x/ipl/qipl.h
> +++ b/include/hw/s390x/ipl/qipl.h
> @@ -167,10 +167,20 @@ struct IplInfoReportBlockHeader {
> };
> typedef struct IplInfoReportBlockHeader IplInfoReportBlockHeader;
>
> +/* IPL Info Error Indicators */
> +#define S390_IIEI_NO_SIGNED_COMP 0x8000 /* bit 0 */
> +#define S390_IIEI_NO_SCLAB 0x4000 /* bit 1 */
> +#define S390_IIEI_NO_GLOBAL_SCLAB 0x2000 /* bit 2 */
> +#define S390_IIEI_MORE_GLOBAL_SCLAB 0x1000 /* bit 3 */
> +#define S390_IIEI_FOUND_UNSIGNED_COMP 0x800 /* bit 4 */
> +#define S390_IIEI_MORE_SIGNED_COMP 0x400 /* bit 5 */
> +
> struct IplInfoBlockHeader {
> uint32_t len;
> uint8_t type;
> - uint8_t reserved1[11];
> + uint8_t reserved1[3];
> + uint16_t iiei;
> + uint8_t reserved2[6];
> };
> typedef struct IplInfoBlockHeader IplInfoBlockHeader;
>
> @@ -194,13 +204,28 @@ typedef struct IplSignatureCertificateList
> IplSignatureCertificateList;
> #define S390_IPL_DEV_COMP_FLAG_SC 0x80
> #define S390_IPL_DEV_COMP_FLAG_CSV 0x40
>
> +/* IPL Device Component Error Indicators */
> +#define S390_CEI_INVALID_SCLAB 0x80000000 /* bit 0 */
> +#define S390_CEI_INVALID_SCLAB_LEN 0x40000000 /* bit 1 */
> +#define S390_CEI_INVALID_SCLAB_FORMAT 0x20000000 /* bit 2 */
> +#define S390_CEI_UNMATCHED_SCLAB_LOAD_ADDR 0x10000000 /* bit 3 */
> +#define S390_CEI_UNMATCHED_SCLAB_LOAD_PSW 0x8000000 /* bit 4 */
> +#define S390_CEI_INVALID_LOAD_PSW 0x4000000 /* bit 5 */
> +#define S390_CEI_NUC_NOT_IN_GLOBAL_SCLAB 0x2000000 /* bit 6 */
> +#define S390_CEI_SCLAB_OLA_NOT_ONE 0x1000000 /* bit 7 */
> +#define S390_CEI_SC_NOT_IN_GLOBAL_SCLAB 0x800000 /* bit 8 */
> +#define S390_CEI_SCLAB_LOAD_ADDR_NOT_ZERO 0x400000 /* bit 9 */
> +#define S390_CEI_SCLAB_LOAD_PSW_NOT_ZERO 0x200000 /* bit 10 */
> +#define S390_CEI_INVALID_UNSIGNED_ADDR 0x100000 /* bit 11 */
> +
> struct IplDeviceComponentEntry {
> uint64_t addr;
> uint64_t len;
> uint8_t flags;
> uint8_t reserved1[5];
> uint16_t cert_index;
> - uint8_t reserved2[8];
> + uint32_t cei;
> + uint8_t reserved2[4];
> };
> typedef struct IplDeviceComponentEntry IplDeviceComponentEntry;
>
> diff --git a/pc-bios/s390-ccw/sclp.h b/pc-bios/s390-ccw/sclp.h
> index a8a41cd004..cae65b29b5 100644
> --- a/pc-bios/s390-ccw/sclp.h
> +++ b/pc-bios/s390-ccw/sclp.h
> @@ -52,6 +52,7 @@ typedef struct SCCBHeader {
> #define SCCB_DATA_LEN (SCCB_SIZE - sizeof(SCCBHeader))
> #define SCCB_FAC134_DIAG320_BIT 0x4
> #define SCCB_FAC_IPL_SIPL_BIT 0x4000
> +#define SCCB_FAC_IPL_SCLAF_BIT 0x1000
>
> typedef struct ReadInfo {
> SCCBHeader h;
> diff --git a/pc-bios/s390-ccw/secure-ipl.c b/pc-bios/s390-ccw/secure-ipl.c
> index fdac74aa97..9c992149ee 100644
> --- a/pc-bios/s390-ccw/secure-ipl.c
> +++ b/pc-bios/s390-ccw/secure-ipl.c
> @@ -140,6 +140,7 @@ static void comp_list_add(IplDeviceComponentList
> *comp_list,
>
> comp_list->device_entries[comp_entry_idx].addr = comp_entry_info.addr;
> comp_list->device_entries[comp_entry_idx].len = comp_entry_info.len;
> + comp_list->device_entries[comp_entry_idx].cei = comp_entry_info.cei;
> comp_list->device_entries[comp_entry_idx].flags = comp_entry_info.flags;
> /* cert index field is meaningful only when S390_IPL_DEV_COMP_FLAG_SC is
> set */
> if (comp_entry_info.flags & S390_IPL_DEV_COMP_FLAG_SC) {
> @@ -207,6 +208,12 @@ bool secure_ipl_supported(void)
> return false;
> }
>
> + if (!sclp_is_fac_ipl_flag_on(SCCB_FAC_IPL_SCLAF_BIT)) {
> + puts("Secure IPL Code Loading Attributes Facility is not supported
> by"
> + " the hypervisor!");
> + return false;
> + }
> +
> return true;
> }
>
> @@ -265,6 +272,179 @@ static void
> comp_addr_range_add(SecureIplCompAddrRangeList *range_list,
> range_list->num += 1;
> }
>
> +static void check_sclab_opsw(SclaBlock *sclab, SecureIplSclabInfo
> *sclab_info,
> + uint32_t *cei_flags)
> +{
> + if (!(sclab->flags & S390_SCLAB_OPSW)) {
> + /* OPSW = 0 - Load PSW field in SCLAB must contain zeros */
> + zipl_secure_validate(sclab->load_psw == 0, cei_flags,
> + S390_CEI_SCLAB_LOAD_PSW_NOT_ZERO,
> + "Load PSW is not zero when Override PSW bit is
> zero");
> + } else {
> + /* OPSW = 1 indicating global SCLAB */
> + sclab_info->global_count += 1;
> + if (sclab_info->global_count == 1) {
> + sclab_info->global_load_psw = sclab->load_psw;
> + sclab_info->global_flags = sclab->flags;
> + }
> +
> + /* override load address flag must set to one */
> + zipl_secure_validate(sclab->flags & S390_SCLAB_OLA, cei_flags,
> + S390_CEI_SCLAB_LOAD_PSW_NOT_ZERO,
> + "OLA flag is not set to one in the global
> SCLAB");
> + }
> +}
> +
> +static void check_sclab_ola(SclaBlock *sclab, uint64_t load_addr, uint32_t
> *cei_flags)
> +{
> + if (!(sclab->flags & S390_SCLAB_OLA)) {
> + /* OLA = 0 - Load address field in SCLAB must contain zeros */
> + zipl_secure_validate(sclab->load_addr == 0, cei_flags,
> + 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 == load_addr, cei_flags,
> + S390_CEI_UNMATCHED_SCLAB_LOAD_ADDR,
> + "Load Address does not match with component
> load address");
> + }
> +}
> +
> +static bool is_psw_valid(uint64_t psw, SecureIplCompAddrRangeList
> *range_list)
> +{
> + uint32_t addr = psw & 0x7fffffff;
> +
> + /* PSW points within a signed binary code component */
> + for (int i = 0; i < range_list->num; i++) {
> + if (range_list->comp_addr_range[i].is_signed &&
> + addr >= range_list->comp_addr_range[i].start_addr &&
> + addr <= range_list->comp_addr_range[i].end_addr - 2) {
> + return true;
> + }
> + }
> + return false;
> +}
This function is very similar to check_comp_overlap. I suggest making a
single "check_overlap" or "intersect" function (the latter mirrors the
naming scheme in the kernel) to combine their functionality and then
have their callers handle the outcome.
e.g. the if function will return true if there is an overlap/intersect.
For components, caller will true it as an error. For the PSW check, the
caller will treat true as success.
> +
> +static void check_load_psw(SecureIplCompAddrRangeList *range_list,
> + uint64_t sclab_load_psw,
> + SecureIplCompEntryInfo *comp_entry_info)
> +{
> + uint64_t load_psw;
> +
> + load_psw = comp_entry_info->addr;
> + zipl_secure_validate(is_psw_valid(sclab_load_psw, range_list) &&
> + is_psw_valid(load_psw, range_list),
> + &comp_entry_info->cei, S390_CEI_INVALID_LOAD_PSW,
> "Invalid PSW");
> +
> + /* compare load PSW with the PSW specified in component */
> + zipl_secure_validate(sclab_load_psw == load_psw, &comp_entry_info->cei,
> + S390_CEI_UNMATCHED_SCLAB_LOAD_PSW,
> + "Load PSW does not match with PSW in component");
> +}
> +
> +static void check_no_unsigned_comp(SecureIplSclabInfo sclab_info,
> + IplDeviceComponentList *comp_list)
> +{
> + bool is_nuc_set;
> +
> + is_nuc_set = sclab_info.global_flags & S390_SCLAB_NUC;
> + if (is_nuc_set && sclab_info.unsigned_count > 0) {
> + comp_list->ipl_info_header.iiei |= S390_IIEI_FOUND_UNSIGNED_COMP;
> + zipl_secure_error("Unsigned components are not allowed");
> + }
> +}
> +
> +static void check_single_comp(SecureIplSclabInfo sclab_info,
> + IplDeviceComponentList *comp_list)
> +{
> + bool is_sc_set;
> +
> + is_sc_set = sclab_info.global_flags & S390_SCLAB_SC;
> + if (is_sc_set &&
> + sclab_info.signed_count != 1 &&
> + sclab_info.unsigned_count >= 0) {
Does the unsigned count matter here?
> + comp_list->ipl_info_header.iiei |= S390_IIEI_MORE_SIGNED_COMP;
> + zipl_secure_error("Only one signed component is allowed");
> + }
> +}
For both functions check_no_unsigned_comp and check_single_comp, could a
zipl_secure_validate be used instead?
> +
> +void check_global_sclab(SecureIplSclabInfo sclab_info,
> + IplDeviceComponentList *comp_list)
Pass this a SclaBlock global_sclab (which should've been assigned by a
subsequent call to check_sclab -- see below), and the
IplDeviceComponentList *comp_list. Iterate through the list and check
each comp.flags for signed and unsigned components, and check PSW validity.
> +{
> + if (sclab_info.count == 0) {
> + return;
> + }
Is this check needed here? zipl_secure_validate(sclab_info.count > 0,
...) is called before this function anyway.
> +
> + if (sclab_info.global_count == 0) {
Would be: if (!global_sclab)
> + comp_list->ipl_info_header.iiei |= S390_IIEI_NO_GLOBAL_SCLAB;
> + zipl_secure_error("Global SCLAB does not exists");
> + return;
> + }
> +
> + if (sclab_info.global_count > 1) {
This check should happen earlier in check_sclab() (see below).
> + comp_list->ipl_info_header.iiei |= S390_IIEI_MORE_GLOBAL_SCLAB;
> + zipl_secure_error("More than one global SCLAB");
> + return;
> + }
> +
> + if (sclab_info.global_flags) {
> + /* Unsigned components are not allowed if NUC flag is set in the
> global SCLAB */
> + check_no_unsigned_comp(sclab_info, comp_list);
> +
> + /* Only one signed component is allowed is SC flag is set in the
> global SCLAB */
> + check_single_comp(sclab_info, comp_list);
Move both function bodies to here, they're small enough and it'll be
easier to read (the comments are helpful)
> + }
> +}
> +
> +static void check_sclab(SecureIplCompEntryInfo *comp_entry_info,
> + SecureIplSclabInfo *sclab_info)
> +{
Pass this a SclaBlock global_sclab. When the OPSW is set, check if
!global_sclab, set global_sclab = sclab; else, set iiei.
The global_sclab is later passed to check_global_sclab().
You might need to pass this function the comp entry (to locate
sclab and set cei) and the header (to set iiei)... this assumes
SecureIplCompEntryInfo goes away as suggested in a previous patch.
> + SclabOriginLocator *sclab_locator;
> + SclaBlock *sclab;
> +
> + /* sclab locator is located at the last 8 bytes of the signed comp */
> + sclab_locator = (SclabOriginLocator *)(comp_entry_info->addr +
> + comp_entry_info->len - 8);
> +
> + /* return early if sclab does not exist */
> + zipl_secure_validate(magic_match(sclab_locator->magic, ZIPL_MAGIC),
> + &comp_entry_info->cei, S390_CEI_INVALID_SCLAB,
> + "Magic does not match. SCLAB does not exist");
> +
> + if (comp_entry_info->cei & S390_CEI_INVALID_SCLAB) {
> + return;
> + }
> +
> + zipl_secure_validate(sclab_locator->len >= S390_SCLAB_MIN_LEN,
> &comp_entry_info->cei,
> + S390_CEI_INVALID_SCLAB_LEN | S390_CEI_INVALID_SCLAB,
> + "Invalid SCLAB length");
> +
> + /* return early if sclab is invalid */
> + if (comp_entry_info->cei & S390_CEI_INVALID_SCLAB) {
> + return;
> + }
> +
> + sclab_info->count += 1;
> + sclab = (SclaBlock *)(comp_entry_info->addr + comp_entry_info->len -
> + sclab_locator->len);
> +
> + zipl_secure_validate(sclab->format == 0, &comp_entry_info->cei,
> + S390_CEI_INVALID_SCLAB_FORMAT,
> + "Format-0 SCLAB is not being used");
> +
> + check_sclab_opsw(sclab, sclab_info, &comp_entry_info->cei);
> + check_sclab_ola(sclab, comp_entry_info->addr, &comp_entry_info->cei);
Both check_sclab_opsw() and check_sclab_ola() function bodies can just
be put in here. They're small enough and visually similar (check flag,
then zipl_secure_validate()). Would be more readable to see all sclab
checking code in one view.
> +
> + zipl_secure_validate(~sclab->flags & S390_SCLAB_NUC || sclab->flags &
> S390_SCLAB_OPSW,
> + &comp_entry_info->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_info->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) {
> @@ -307,7 +487,7 @@ int zipl_run_secure(ComponentEntry **entry_ptr, uint8_t
> *tmp_sec)
> */
> int cert_list_table[MAX_CERTIFICATES] = { [0 ... MAX_CERTIFICATES - 1] =
> -1 };
> SecureIplCompAddrRangeList range_list = { 0 };
> - int signed_count = 0;
> + SecureIplSclabInfo sclab_info = { 0 };
>
> init_lists(&comp_list, &cert_list);
> cert_buf = malloc(get_total_certs_length());
> @@ -343,6 +523,13 @@ int zipl_run_secure(ComponentEntry **entry_ptr, uint8_t
> *tmp_sec)
>
> /* no signature present (unsigned component) */
> if (!sig_entry_info.len) {
> + zipl_secure_validate(comp_entry_info.addr >=
> S390_UNSIGNED_MIN_ADDR,
> + &comp_entry_info.cei,
> S390_CEI_INVALID_UNSIGNED_ADDR,
> + "Load address is less than 0x2000");
"Load address for unsigned component is less than 0x2000"
> +
> + comp_list_add(&comp_list, comp_entry_info);
Thinking about it... back in patch 21, shouldn't unsigned components be
added to the list?
> +
> + sclab_info.unsigned_count += 1;
> break;
> }
>
> @@ -352,6 +539,7 @@ int zipl_run_secure(ComponentEntry **entry_ptr, uint8_t
> *tmp_sec)
> */
> comp_entry_info.flags = S390_IPL_DEV_COMP_FLAG_SC;
>
> + check_sclab(&comp_entry_info, &sclab_info);
> verified = verify_signature(comp_entry_info, sig_entry_info,
> &cert_len, &cert_table_idx);
>
> @@ -378,7 +566,7 @@ int zipl_run_secure(ComponentEntry **entry_ptr, uint8_t
> *tmp_sec)
>
> comp_list_add(&comp_list, comp_entry_info);
>
> - signed_count += 1;
> + sclab_info.signed_count += 1;
> /* After a signature is used another new one can be accepted */
> sig_entry_info.len = 0;
> break;
> @@ -395,10 +583,24 @@ int zipl_run_secure(ComponentEntry **entry_ptr, uint8_t
> *tmp_sec)
> }
> }
>
> - if (signed_count == 0) {
> - zipl_secure_error("Secure boot is on, but components are not
> signed");
> + /* validate load PSW with PSW specified in the final entry */
> + if (sclab_info.global_load_psw) {
> + comp_entry_info = (SecureIplCompEntryInfo){ 0 };
> + comp_entry_info.addr = entry->compdat.load_psw;
> +
> + check_load_psw(&range_list, sclab_info.global_load_psw,
> &comp_entry_info);
> + comp_list_add(&comp_list, comp_entry_info);
> }
This check could be done inside check_global_sclab() with the changes
mentioned above. While iterating through the comp_list checking for
flags, you can also check PSW validity.
>
> + zipl_secure_validate(sclab_info.signed_count > 0,
> + &comp_list.ipl_info_header.iiei,
> S390_IIEI_NO_SIGNED_COMP,
> + "Secure boot is on, but components are not signed");
> +
> + zipl_secure_validate(sclab_info.count > 0,
> &comp_list.ipl_info_header.iiei,
> + S390_IIEI_NO_SCLAB, "No recognizable SCLAB");
Both of these iiei flags could be set near the start of
zipl_run_secure() (which is technically true since no SCLABs or signed
comps have been found at that point in time). Then, when the first
valid SCLAB or signed comp is found, clear the respective flag.
At the end of this function, check if those flags are still set
and call zipl_secure_error().
That will help justify getting rid of the counter fields.
> +
> + check_global_sclab(sclab_info, &comp_list);
> +
> update_iirb(&comp_list, &cert_list);
>
> *entry_ptr = entry;
> diff --git a/pc-bios/s390-ccw/secure-ipl.h b/pc-bios/s390-ccw/secure-ipl.h
> index 29bbf65c6c..950cd45b3c 100644
> --- a/pc-bios/s390-ccw/secure-ipl.h
> +++ b/pc-bios/s390-ccw/secure-ipl.h
> @@ -16,10 +16,48 @@
> VCStorageSizeBlock *zipl_secure_get_vcssb(void);
> int zipl_run_secure(ComponentEntry **entry_ptr, uint8_t *tmp_sec);
>
> +#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;
> +
> +/* Custom struct used to consolidate SCLAB overhead */
> +typedef struct SecureIplSclabInfo {
> + int count;
> + int global_count;
> + int signed_count;
> + int unsigned_count;
> + uint64_t global_load_psw;
> + uint16_t global_flags;
> +} SecureIplSclabInfo;
I think with the suggestions above, this struct should no longer be
needed. The global fields can be handled by tracking a special
SclaBlock global_sclab. The signed/unsigned counts can be tallied by
iterating through the comp list. Global count can be checked by
global_sclab != NULL.
Count might be able to go away too (see above).
> +
> /* Custom struct for secure IPL component entry information */
> typedef struct SecureIplCompEntryInfo {
> uint64_t addr;
> uint64_t len;
> + uint32_t cei;
> uint16_t cert_index;
> uint8_t flags;
> } SecureIplCompEntryInfo;
> @@ -50,6 +88,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)
> +
This is MUCH better! :)
> static inline uint64_t _diag320(void *data, unsigned long subcode)
> {
> register unsigned long addr asm("0") = (unsigned long)data;
With the suggestions above, most (if not all) SCLAF-related code should
be contained to the check_sclab and check_global_sclab functions and
less overhead managed in zipl_secure_run, which will make this a LOT
easier to visually parse.
Hopefully I'm not missing a major design flaw in this thinking. Please
let me know if the suggestions are not feasible.
--
Regards,
Collin
