Re: [PATCH v9 20/30] pc-bios/s390-ccw: Add signed component address overlap checks

[Collin Walling <walling@linux.ibm.com>]

On 3/5/26 17:41, Zhuoying Cai wrote:
> Add address range tracking and overlap checks to ensure that no
> component overlaps with a signed component during secure IPL.
> 
> Signed-off-by: Zhuoying Cai <zycai@linux.ibm.com>
> ---
>  pc-bios/s390-ccw/secure-ipl.c | 52 +++++++++++++++++++++++++++++++++++
>  pc-bios/s390-ccw/secure-ipl.h |  6 ++++
>  2 files changed, 58 insertions(+)
> 
> diff --git a/pc-bios/s390-ccw/secure-ipl.c b/pc-bios/s390-ccw/secure-ipl.c
> index 8d281c1cea..68596491c5 100644
> --- a/pc-bios/s390-ccw/secure-ipl.c
> +++ b/pc-bios/s390-ccw/secure-ipl.c
> @@ -211,6 +211,53 @@ static void init_lists(IplDeviceComponentList *comp_list,
>      cert_list->ipl_info_header.len = sizeof(cert_list->ipl_info_header);
>  }
>  
> +static bool is_comp_overlap(SecureIplCompAddrRange *comp_addr_range,
> +                            int addr_range_index,
> +                            uint64_t start_addr, uint64_t end_addr)
> +{
> +    /* neither a signed nor an unsigned component can overlap with a signed component */

How about: "Check component's address range does not overlap with any
signed component's address range."

> +    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) {
> +        zipl_secure_handle("Component address range update failed due to out-of-range"
> +                           " index; Overlapping validation cannot be guaranteed");
> +    }
> +
> +    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 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) {
> +        zipl_secure_handle("Component addresses overlap");
> +    }
> +
> +    comp_addr_range_add(comp_addr_range, *addr_range_index, is_signed,
> +                        start_addr, end_addr);
> +    *addr_range_index += 1;
> +}
> +
>  static int zipl_load_signature(ComponentEntry *entry, uint64_t sig_sec)
>  {
>      if (zipl_load_segment(entry, sig_sec) < 0) {
> @@ -254,6 +301,8 @@ int zipl_run_secure(ComponentEntry **entry_ptr, uint8_t *tmp_sec)
>       * exists for the certificate).
>       */
>      int cert_list_table[MAX_CERTIFICATES] = { [0 ... MAX_CERTIFICATES - 1] = -1 };
> +    SecureIplCompAddrRange comp_addr_range[MAX_CERTIFICATES];
> +    int addr_range_index = 0;
>      int signed_count = 0;
>  
>      if (!secure_ipl_supported()) {
> @@ -283,6 +332,9 @@ int zipl_run_secure(ComponentEntry **entry_ptr, uint8_t *tmp_sec)
>                  goto out;
>              }
>  
> +            addr_overlap_check(comp_addr_range, &addr_range_index,
> +                               comp_addr, comp_addr + comp_len, sig_len > 0);

super nit: I'd prefer !!sig_len versus sig_len > 0.

> +
>              if (!sig_len) {
>                  break;
>              }
> diff --git a/pc-bios/s390-ccw/secure-ipl.h b/pc-bios/s390-ccw/secure-ipl.h
> index eb5ba0ed47..69edfce241 100644
> --- a/pc-bios/s390-ccw/secure-ipl.h
> +++ b/pc-bios/s390-ccw/secure-ipl.h
> @@ -16,6 +16,12 @@
>  VCStorageSizeBlock *zipl_secure_get_vcssb(void);
>  int zipl_run_secure(ComponentEntry **entry_ptr, uint8_t *tmp_sec);
>  
> +typedef struct SecureIplCompAddrRange {
> +    bool is_signed;
> +    uint64_t start_addr;
> +    uint64_t end_addr;
> +} SecureIplCompAddrRange;
> +

Since this is a custom construct made for keeping track of address
ranges, why not define your own list data structure that also keeps
track of the index?  Could do:

typedef struct SecureIplCompAddrRangeList {
	SecureIplCompAddrRange comp_addr_range[MAX_CERTIFICATES];
	int index;
}

Then you could greatly simplify the function signatures by passing the
list and single entry.  Depending on how reduced the code is after the
changes, it might look better to just do something like this in
zipl_run_secure and get rid of addr_overlap_check:

```
if (is_comp_overlap(list, comp)) {
	zipl_secure_handle("message");
}
comp_addr_range_list_add(list, comp);
```

Would make things look a lot cleaner imho :)

Note: it may make sense to carry this list structure in other areas too
-- it would help keep track of the respective list indexes and result in
less variables passed around.

>  static inline void zipl_secure_handle(const char *message)
>  {
>      switch (boot_mode) {


-- 
Regards,
  Collin