[PATCH 0/5] more hardening fixes for s390

[PATCH 0/5] more hardening fixes for s390

[Christian Borntraeger]

Cornelia, Eric, Matt,

here are 5 more fixes for hardening QEMU against invalid input. Nothing
is strictly a security issues as defined in the policy. For example
https://qemu-project.gitlab.io/qemu/system/security.html says assert /
abort: If triggering the code path requires kernel privileges (or root
account access) in the guest, asserts/aborts in QEMU are a self
inflicted denial of service. These will not be treated as security
flaws, at most hardening bugs.
And if anyone can provide invalid boot loader content it can provide
anything to boot if secure boot is not available.
We should fix those anyway, probably even for 11.1 

Conny, I did not rebuild the s390-ccw, I assume you will do that?

Christian Borntraeger (1):
  hw/char/sclpconsole-lm: avoid guest triggerable assert

Joshua Daley (4):
  s390x/ipl: validate num_comp against iplb length before iterating
  pc-bios/s390-ccw: fix out-of-bounds read in iso_get_file_size()
  pc-bios/s390-ccw: bounds-check zipl menu entry index before array
    write
  pc-bios/s390-ccw: bound zipl menu strlen and replace VLA in
    zipl_print_entry

 hw/char/sclpconsole-lm.c    |  3 ++-
 hw/s390x/ipl.h              |  6 ++++++
 pc-bios/s390-ccw/bootmap.c  |  6 ++++--
 pc-bios/s390-ccw/helper.h   | 10 ++++++++++
 pc-bios/s390-ccw/menu.c     | 36 +++++++++++++++++++++++++++++-------
 pc-bios/s390-ccw/s390-ccw.h |  2 +-
 6 files changed, 52 insertions(+), 11 deletions(-)

-- 
2.53.0

[PATCH 1/5] hw/char/sclpconsole-lm: avoid guest triggerable assert

[Christian Borntraeger]

If a guest uses incorrect message length it can trigger an assert in
process_mbd which kills the guest instead of reporting an error.  Fix
this by adding the correct length check.

Fixes: 6a444f8507 ("s390/sclplmconsole: Add support for SCLP line-mode console")
Signed-off-by: Christian Borntraeger <borntraeger@linux.ibm.com>
---
 hw/char/sclpconsole-lm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/char/sclpconsole-lm.c b/hw/char/sclpconsole-lm.c
index 9a16896d22..f6ed282f1b 100644
--- a/hw/char/sclpconsole-lm.c
+++ b/hw/char/sclpconsole-lm.c
@@ -243,7 +243,8 @@ static int write_event_data(SCLPEvent *event, EventBufferHeader *ebh)
     SCLPConsoleLM *scon = SCLPLM_CONSOLE(event);
 
     len = be16_to_cpu(data->mdb.header.length);
-    if (len < sizeof(data->mdb.header)) {
+    if (len < sizeof(data->mdb.header) ||
+        len > be16_to_cpu(data->header.length) - sizeof(EventBufferHeader)) {
         return SCLP_RC_INCONSISTENT_LENGTHS;
     }
     len -= sizeof(data->mdb.header);
-- 
2.53.0

Re: [PATCH 1/5] hw/char/sclpconsole-lm: avoid guest triggerable assert

[Matthew Rosato]

On 7/27/26 7:50 AM, Christian Borntraeger wrote:
> If a guest uses incorrect message length it can trigger an assert in
> process_mbd which kills the guest instead of reporting an error.  Fix

Typo, s/process_mbd/process_mdb/

Maybe can just be fixed when applying?

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

> this by adding the correct length check.
> 
> Fixes: 6a444f8507 ("s390/sclplmconsole: Add support for SCLP line-mode console")
> Signed-off-by: Christian Borntraeger <borntraeger@linux.ibm.com>
> ---
>  hw/char/sclpconsole-lm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/char/sclpconsole-lm.c b/hw/char/sclpconsole-lm.c
> index 9a16896d22..f6ed282f1b 100644
> --- a/hw/char/sclpconsole-lm.c
> +++ b/hw/char/sclpconsole-lm.c
> @@ -243,7 +243,8 @@ static int write_event_data(SCLPEvent *event, EventBufferHeader *ebh)
>      SCLPConsoleLM *scon = SCLPLM_CONSOLE(event);
>  
>      len = be16_to_cpu(data->mdb.header.length);
> -    if (len < sizeof(data->mdb.header)) {
> +    if (len < sizeof(data->mdb.header) ||
> +        len > be16_to_cpu(data->header.length) - sizeof(EventBufferHeader)) {
>          return SCLP_RC_INCONSISTENT_LENGTHS;
>      }
>      len -= sizeof(data->mdb.header);

[PATCH 2/5] s390x/ipl: validate num_comp against iplb length before iterating

[Christian Borntraeger]

From: Joshua Daley <jdaley@linux.ibm.com>

In ipl_valid_pv_components(), the upper bound of the for loop,
ipib_pv->num_comp, is read from guest memory. Before iterating, verify
that its value will not cause a read beyond the end of the
IplParameterBlock.

Fixes: c3347ed0d2ee42a7 ("s390x: protvirt: Support unpack facility")
Signed-off-by: Joshua Daley <jdaley@linux.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>
---
 hw/s390x/ipl.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
index fac30763df..ef9c063d90 100644
--- a/hw/s390x/ipl.h
+++ b/hw/s390x/ipl.h
@@ -124,6 +124,12 @@ static inline bool ipl_valid_pv_components(IplParameterBlock *iplb)
         return false;
     }
 
+    if (offsetof(IplParameterBlock, pv.components) +
+        ipib_pv->num_comp * sizeof(IPLBlockPVComp) >
+        be32_to_cpu(iplb->len)) {
+        return false;
+    }
+
     for (i = 0; i < ipib_pv->num_comp; i++) {
         /* Addr must be 4k aligned */
         if (ipib_pv->components[i].addr & ~TARGET_PAGE_MASK) {
-- 
2.53.0

Re: [PATCH 2/5] s390x/ipl: validate num_comp against iplb length before iterating

[Matthew Rosato]

On 7/27/26 7:50 AM, Christian Borntraeger wrote:
> From: Joshua Daley <jdaley@linux.ibm.com>
> 
> In ipl_valid_pv_components(), the upper bound of the for loop,
> ipib_pv->num_comp, is read from guest memory. Before iterating, verify
> that its value will not cause a read beyond the end of the
> IplParameterBlock.
> 
> Fixes: c3347ed0d2ee42a7 ("s390x: protvirt: Support unpack facility")
> Signed-off-by: Joshua Daley <jdaley@linux.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

> ---
>  hw/s390x/ipl.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> index fac30763df..ef9c063d90 100644
> --- a/hw/s390x/ipl.h
> +++ b/hw/s390x/ipl.h
> @@ -124,6 +124,12 @@ static inline bool ipl_valid_pv_components(IplParameterBlock *iplb)
>          return false;
>      }
>  
> +    if (offsetof(IplParameterBlock, pv.components) +
> +        ipib_pv->num_comp * sizeof(IPLBlockPVComp) >
> +        be32_to_cpu(iplb->len)) {
> +        return false;
> +    }
> +
>      for (i = 0; i < ipib_pv->num_comp; i++) {
>          /* Addr must be 4k aligned */
>          if (ipib_pv->components[i].addr & ~TARGET_PAGE_MASK) {

[PATCH 3/5] pc-bios/s390-ccw: fix out-of-bounds read in iso_get_file_size()

[Christian Borntraeger]

From: Joshua Daley <jdaley@linux.ibm.com>

In the dir_rem[level] == 0 case, level is decremented, then
a virtio_read() is issued on sec_loc[level]. If level is -1, then the
4 bytes before the static sec_loc array are read, and the virtio_read()
is issued on that garbage block number.

Guard the call to virtio_read() against the value of level to prevent
this.

Fixes: 869648e87eeb ("pc-bios/s390-ccw: El Torito 16-bit boot image size field workaround")
Signed-off-by: Joshua Daley <jdaley@linux.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@inux.ibm.com>
---
 pc-bios/s390-ccw/bootmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 420ee32eff..ed6e8cbbc7 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -902,7 +902,7 @@ static inline long iso_get_file_size(uint32_t load_rba)
         if (dir_rem[level] == 0) {
             /* Nothing remaining */
             level--;
-            if (virtio_read(sec_loc[level], temp)) {
+            if (level >= 0 && virtio_read(sec_loc[level], temp)) {
                 puts("Failed to read ISO directory");
                 return -EIO;
             }
-- 
2.53.0

Re: [PATCH 3/5] pc-bios/s390-ccw: fix out-of-bounds read in iso_get_file_size()

[Matthew Rosato]

On 7/27/26 7:50 AM, Christian Borntraeger wrote:
> From: Joshua Daley <jdaley@linux.ibm.com>
> 
> In the dir_rem[level] == 0 case, level is decremented, then
> a virtio_read() is issued on sec_loc[level]. If level is -1, then the
> 4 bytes before the static sec_loc array are read, and the virtio_read()
> is issued on that garbage block number.
> 
> Guard the call to virtio_read() against the value of level to prevent
> this.
> 
> Fixes: 869648e87eeb ("pc-bios/s390-ccw: El Torito 16-bit boot image size field workaround")
> Signed-off-by: Joshua Daley <jdaley@linux.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@inux.ibm.com>

Typo in the RB tag, please fix when applying or if there is a v2!

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

> ---
>  pc-bios/s390-ccw/bootmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> index 420ee32eff..ed6e8cbbc7 100644
> --- a/pc-bios/s390-ccw/bootmap.c
> +++ b/pc-bios/s390-ccw/bootmap.c
> @@ -902,7 +902,7 @@ static inline long iso_get_file_size(uint32_t load_rba)
>          if (dir_rem[level] == 0) {
>              /* Nothing remaining */
>              level--;
> -            if (virtio_read(sec_loc[level], temp)) {
> +            if (level >= 0 && virtio_read(sec_loc[level], temp)) {
>                  puts("Failed to read ISO directory");
>                  return -EIO;
>              }

[PATCH 4/5] pc-bios/s390-ccw: bounds-check zipl menu entry index before array write

[Christian Borntraeger]

From: Joshua Daley <jdaley@linux.ibm.com>

menu_get_zipl_boot_index() iterates NUL-separated strings from the
zipl stage-2 boot-menu block, passes each to zipl_print_entry() which
converts EBCDIC to ASCII and returns atoi(), then writes true into
valid_entries[entry]. valid_entries is a MAX_BOOT_ENTRIES element
stack array, but entry was never bounds-checked, so a crafted on-disk
value could index arbitrarily beyond the array.

Fix this in two places:

- zipl_print_entry() now validates that the first significant character
  (after an optional leading space) is a digit. Entries that fail this
  check return -1 without printing.

- menu_get_zipl_boot_index() skips any entry whose index is outside
  [0, MAX_BOOT_ENTRIES) before writing to valid_entries[].

Fixes: 7385e947fc65 ("pc-bios/s390-ccw: fix non-sequential boot entries (eckd)")
Signed-off-by: Joshua Daley <jdaley@linux.ibm.com>
---
 pc-bios/s390-ccw/menu.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
index eeaff78f87..b6a9a56d46 100644
--- a/pc-bios/s390-ccw/menu.c
+++ b/pc-bios/s390-ccw/menu.c
@@ -176,18 +176,24 @@ int menu_get_boot_index(bool *valid_entries)
     return boot_index;
 }
 
-/* Returns the entry number that was printed */
+/* Returns the entry number that was printed, or -1 on invalid entry */
 static int zipl_print_entry(const char *data, size_t len)
 {
     char buf[len + 2];
+    const char *p;
 
     ebcdic_to_ascii(data, buf, len);
     buf[len] = '\n';
     buf[len + 1] = '\0';
 
+    p = (buf[0] == ' ') ? buf + 1 : buf;
+    if (!isdigit((unsigned char)*p)) {
+        return -1;
+    }
+
     printf("%s", buf);
 
-    return buf[0] == ' ' ? atoi(buf + 1) : atoi(buf);
+    return atoi(p);
 }
 
 int menu_get_zipl_boot_index(const char *menu_data)
@@ -216,6 +222,9 @@ int menu_get_zipl_boot_index(const char *menu_data)
         entry = zipl_print_entry(menu_data, len);
         menu_data += len + 1;
 
+        if (entry < 0 || entry >= MAX_BOOT_ENTRIES) {
+            continue;
+        }
         valid_entries[entry] = true;
 
         if (entry == 0) {
-- 
2.53.0

Re: [PATCH 4/5] pc-bios/s390-ccw: bounds-check zipl menu entry index before array write

[Matthew Rosato]

On 7/27/26 7:50 AM, Christian Borntraeger wrote:
> From: Joshua Daley <jdaley@linux.ibm.com>
> 
> menu_get_zipl_boot_index() iterates NUL-separated strings from the
> zipl stage-2 boot-menu block, passes each to zipl_print_entry() which
> converts EBCDIC to ASCII and returns atoi(), then writes true into
> valid_entries[entry]. valid_entries is a MAX_BOOT_ENTRIES element
> stack array, but entry was never bounds-checked, so a crafted on-disk
> value could index arbitrarily beyond the array.
> 
> Fix this in two places:
> 
> - zipl_print_entry() now validates that the first significant character
>   (after an optional leading space) is a digit. Entries that fail this
>   check return -1 without printing.
> 
> - menu_get_zipl_boot_index() skips any entry whose index is outside
>   [0, MAX_BOOT_ENTRIES) before writing to valid_entries[].
> 
> Fixes: 7385e947fc65 ("pc-bios/s390-ccw: fix non-sequential boot entries (eckd)")
> Signed-off-by: Joshua Daley <jdaley@linux.ibm.com>

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

> ---
>  pc-bios/s390-ccw/menu.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
> index eeaff78f87..b6a9a56d46 100644
> --- a/pc-bios/s390-ccw/menu.c
> +++ b/pc-bios/s390-ccw/menu.c
> @@ -176,18 +176,24 @@ int menu_get_boot_index(bool *valid_entries)
>      return boot_index;
>  }
>  
> -/* Returns the entry number that was printed */
> +/* Returns the entry number that was printed, or -1 on invalid entry */
>  static int zipl_print_entry(const char *data, size_t len)
>  {
>      char buf[len + 2];
> +    const char *p;
>  
>      ebcdic_to_ascii(data, buf, len);
>      buf[len] = '\n';
>      buf[len + 1] = '\0';
>  
> +    p = (buf[0] == ' ') ? buf + 1 : buf;
> +    if (!isdigit((unsigned char)*p)) {
> +        return -1;
> +    }
> +
>      printf("%s", buf);
>  
> -    return buf[0] == ' ' ? atoi(buf + 1) : atoi(buf);
> +    return atoi(p);
>  }
>  
>  int menu_get_zipl_boot_index(const char *menu_data)
> @@ -216,6 +222,9 @@ int menu_get_zipl_boot_index(const char *menu_data)
>          entry = zipl_print_entry(menu_data, len);
>          menu_data += len + 1;
>  
> +        if (entry < 0 || entry >= MAX_BOOT_ENTRIES) {
> +            continue;
> +        }
>          valid_entries[entry] = true;
>  
>          if (entry == 0) {

[PATCH 5/5] pc-bios/s390-ccw: bound zipl menu strlen and replace VLA in zipl_print_entry

[Christian Borntraeger]

From: Joshua Daley <jdaley@linux.ibm.com>

menu_get_zipl_boot_index() calls strlen() on a pointer into the middle
of _s2 with no upper bound, so a stage-2 image whose blocks contain no
NUL bytes causes strlen() to walk beyond _s2. The resulting length
is then used to size a stack VLA in zipl_print_entry(), risking a stack
overflow.

Fix by:

- Implementing strnlen(), a bounded version of strlen().

- Adding a menu_data_end parameter to menu_get_zipl_boot_index() and
  replacing both strlen() calls with strnlen() bounded by the remaining
  buffer space. The loop guard also checks that the pointer has not
  reached menu_data_end. For robustness, the function returns 0
  (boot default) if somehow menu_data is initially >= menu_data_end.

- Replacing the VLA char buf[len + 2] in zipl_print_entry() with a fixed
  ZIPL_ENTRY_MAX + 2 (82-byte) buffer and truncating len before use.

- Passing s2_end (_s2 + sizeof(_s2)) as menu_data_end at the one call
  site in eckd_get_boot_menu_index(), so the bound is exactly the end of
  the buffer.

Fixes: f7178910845a ("s390-ccw: print zipl boot menu")
Signed-off-by: Joshua Daley <jdaley@linux.ibm.com>
---
 pc-bios/s390-ccw/bootmap.c  |  4 +++-
 pc-bios/s390-ccw/helper.h   | 10 ++++++++++
 pc-bios/s390-ccw/menu.c     | 23 ++++++++++++++++++-----
 pc-bios/s390-ccw/s390-ccw.h |  2 +-
 4 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index ed6e8cbbc7..81512265ce 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -61,6 +61,7 @@ static uint8_t _s2[MAX_SECTOR_SIZE * 3] __attribute__((__aligned__(PAGE_SIZE)));
 static void *s2_prev_blk = _s2;
 static void *s2_cur_blk = _s2 + MAX_SECTOR_SIZE;
 static void *s2_next_blk = _s2 + MAX_SECTOR_SIZE * 2;
+static void *s2_end = _s2 + sizeof(_s2);
 
 static inline int verify_boot_info(BootInfo *bip)
 {
@@ -308,7 +309,8 @@ static int eckd_get_boot_menu_index(block_number_t s1b_block_nr)
                 }
             }
 
-            return menu_get_zipl_boot_index(s2_cur_blk + banner_offset);
+            return menu_get_zipl_boot_index(s2_cur_blk + banner_offset,
+                                            s2_end);
         }
 
         prev_block_nr = cur_block_nr;
diff --git a/pc-bios/s390-ccw/helper.h b/pc-bios/s390-ccw/helper.h
index 8e3dfcb6d6..d9b7da444a 100644
--- a/pc-bios/s390-ccw/helper.h
+++ b/pc-bios/s390-ccw/helper.h
@@ -45,4 +45,14 @@ static inline void sleep(unsigned int seconds)
     }
 }
 
+static inline size_t strnlen(const char *s, size_t maxlen)
+{
+    size_t len = 0;
+
+    while (len < maxlen && s[len]) {
+        len++;
+    }
+    return len;
+}
+
 #endif
diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
index b6a9a56d46..af96a83de9 100644
--- a/pc-bios/s390-ccw/menu.c
+++ b/pc-bios/s390-ccw/menu.c
@@ -16,6 +16,7 @@
 #include "s390-ccw.h"
 #include "sclp.h"
 #include "s390-time.h"
+#include "helper.h"
 
 #define KEYCODE_NO_INP '\0'
 #define KEYCODE_ESCAPE '\033'
@@ -26,6 +27,9 @@
 #define ZIPL_TIMEOUT_OFFSET 138
 #define ZIPL_FLAG_OFFSET    140
 
+/* Max printable chars for a zipl boot menu entry */
+#define ZIPL_ENTRY_MAX 80
+
 #define TOD_CLOCK_MILLISECOND   0x3e8000
 
 #define LOW_CORE_EXTERNAL_INT_ADDR   0x86
@@ -179,9 +183,13 @@ int menu_get_boot_index(bool *valid_entries)
 /* Returns the entry number that was printed, or -1 on invalid entry */
 static int zipl_print_entry(const char *data, size_t len)
 {
-    char buf[len + 2];
+    char buf[ZIPL_ENTRY_MAX + 2];
     const char *p;
 
+    if (len > ZIPL_ENTRY_MAX) {
+        len = ZIPL_ENTRY_MAX;
+    }
+
     ebcdic_to_ascii(data, buf, len);
     buf[len] = '\n';
     buf[len + 1] = '\0';
@@ -196,7 +204,7 @@ static int zipl_print_entry(const char *data, size_t len)
     return atoi(p);
 }
 
-int menu_get_zipl_boot_index(const char *menu_data)
+int menu_get_zipl_boot_index(const char *menu_data, const char *menu_data_end)
 {
     size_t len;
     int entry;
@@ -212,13 +220,18 @@ int menu_get_zipl_boot_index(const char *menu_data)
         timeout = zipl_timeout * 1000;
     }
 
+    if (menu_data >= menu_data_end) {
+        return 0;
+    }
+
     /* Print banner */
     puts("s390-ccw zIPL Boot Menu\n");
-    menu_data += strlen(menu_data) + 1;
+    len = strnlen(menu_data, menu_data_end - menu_data);
+    menu_data += len + 1;
 
     /* Print entries */
-    while (*menu_data) {
-        len = strlen(menu_data);
+    while (menu_data < menu_data_end && *menu_data) {
+        len = strnlen(menu_data, menu_data_end - menu_data);
         entry = zipl_print_entry(menu_data, len);
         menu_data += len + 1;
 
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index 1e1f71775e..f6030a6071 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -76,7 +76,7 @@ void jump_to_low_kernel(void);
 
 /* menu.c */
 void menu_set_parms(uint8_t boot_menu_flag, uint32_t boot_menu_timeout);
-int menu_get_zipl_boot_index(const char *menu_data);
+int menu_get_zipl_boot_index(const char *menu_data, const char *menu_data_end);
 bool menu_is_enabled_zipl(void);
 int menu_get_enum_boot_index(bool *valid_entries);
 bool menu_is_enabled_enum(void);
-- 
2.53.0

Re: [PATCH 5/5] pc-bios/s390-ccw: bound zipl menu strlen and replace VLA in zipl_print_entry

[Matthew Rosato]

On 7/27/26 7:50 AM, Christian Borntraeger wrote:
> From: Joshua Daley <jdaley@linux.ibm.com>
> 
> menu_get_zipl_boot_index() calls strlen() on a pointer into the middle
> of _s2 with no upper bound, so a stage-2 image whose blocks contain no
> NUL bytes causes strlen() to walk beyond _s2. The resulting length
> is then used to size a stack VLA in zipl_print_entry(), risking a stack
> overflow.
> 
> Fix by:
> 
> - Implementing strnlen(), a bounded version of strlen().
> 
> - Adding a menu_data_end parameter to menu_get_zipl_boot_index() and
>   replacing both strlen() calls with strnlen() bounded by the remaining
>   buffer space. The loop guard also checks that the pointer has not
>   reached menu_data_end. For robustness, the function returns 0
>   (boot default) if somehow menu_data is initially >= menu_data_end.
> 
> - Replacing the VLA char buf[len + 2] in zipl_print_entry() with a fixed
>   ZIPL_ENTRY_MAX + 2 (82-byte) buffer and truncating len before use.
> 
> - Passing s2_end (_s2 + sizeof(_s2)) as menu_data_end at the one call
>   site in eckd_get_boot_menu_index(), so the bound is exactly the end of
>   the buffer.
> 
> Fixes: f7178910845a ("s390-ccw: print zipl boot menu")
> Signed-off-by: Joshua Daley <jdaley@linux.ibm.com>
> ---
>  pc-bios/s390-ccw/bootmap.c  |  4 +++-
>  pc-bios/s390-ccw/helper.h   | 10 ++++++++++
>  pc-bios/s390-ccw/menu.c     | 23 ++++++++++++++++++-----
>  pc-bios/s390-ccw/s390-ccw.h |  2 +-
>  4 files changed, 32 insertions(+), 7 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> index ed6e8cbbc7..81512265ce 100644
> --- a/pc-bios/s390-ccw/bootmap.c
> +++ b/pc-bios/s390-ccw/bootmap.c
> @@ -61,6 +61,7 @@ static uint8_t _s2[MAX_SECTOR_SIZE * 3] __attribute__((__aligned__(PAGE_SIZE)));
>  static void *s2_prev_blk = _s2;
>  static void *s2_cur_blk = _s2 + MAX_SECTOR_SIZE;
>  static void *s2_next_blk = _s2 + MAX_SECTOR_SIZE * 2;
> +static void *s2_end = _s2 + sizeof(_s2);
>  
>  static inline int verify_boot_info(BootInfo *bip)
>  {
> @@ -308,7 +309,8 @@ static int eckd_get_boot_menu_index(block_number_t s1b_block_nr)
>                  }
>              }
>  
> -            return menu_get_zipl_boot_index(s2_cur_blk + banner_offset);
> +            return menu_get_zipl_boot_index(s2_cur_blk + banner_offset,
> +                                            s2_end);
>          }
>  
>          prev_block_nr = cur_block_nr;
> diff --git a/pc-bios/s390-ccw/helper.h b/pc-bios/s390-ccw/helper.h
> index 8e3dfcb6d6..d9b7da444a 100644
> --- a/pc-bios/s390-ccw/helper.h
> +++ b/pc-bios/s390-ccw/helper.h
> @@ -45,4 +45,14 @@ static inline void sleep(unsigned int seconds)
>      }
>  }
>  
> +static inline size_t strnlen(const char *s, size_t maxlen)
> +{
> +    size_t len = 0;
> +
> +    while (len < maxlen && s[len]) {
> +        len++;
> +    }
> +    return len;
> +}
> +
>  #endif
> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
> index b6a9a56d46..af96a83de9 100644
> --- a/pc-bios/s390-ccw/menu.c
> +++ b/pc-bios/s390-ccw/menu.c
> @@ -16,6 +16,7 @@
>  #include "s390-ccw.h"
>  #include "sclp.h"
>  #include "s390-time.h"
> +#include "helper.h"
>  
>  #define KEYCODE_NO_INP '\0'
>  #define KEYCODE_ESCAPE '\033'
> @@ -26,6 +27,9 @@
>  #define ZIPL_TIMEOUT_OFFSET 138
>  #define ZIPL_FLAG_OFFSET    140
>  
> +/* Max printable chars for a zipl boot menu entry */
> +#define ZIPL_ENTRY_MAX 80
> +
>  #define TOD_CLOCK_MILLISECOND   0x3e8000
>  
>  #define LOW_CORE_EXTERNAL_INT_ADDR   0x86
> @@ -179,9 +183,13 @@ int menu_get_boot_index(bool *valid_entries)
>  /* Returns the entry number that was printed, or -1 on invalid entry */
>  static int zipl_print_entry(const char *data, size_t len)
>  {
> -    char buf[len + 2];
> +    char buf[ZIPL_ENTRY_MAX + 2];
>      const char *p;
>  
> +    if (len > ZIPL_ENTRY_MAX) {
> +        len = ZIPL_ENTRY_MAX;
> +    }
> +
>      ebcdic_to_ascii(data, buf, len);
>      buf[len] = '\n';
>      buf[len + 1] = '\0';
> @@ -196,7 +204,7 @@ static int zipl_print_entry(const char *data, size_t len)
>      return atoi(p);
>  }
>  
> -int menu_get_zipl_boot_index(const char *menu_data)
> +int menu_get_zipl_boot_index(const char *menu_data, const char *menu_data_end)
>  {
>      size_t len;
>      int entry;
> @@ -212,13 +220,18 @@ int menu_get_zipl_boot_index(const char *menu_data)
>          timeout = zipl_timeout * 1000;
>      }
>  
> +    if (menu_data >= menu_data_end) {
> +        return 0;
> +    }
> +
>      /* Print banner */
>      puts("s390-ccw zIPL Boot Menu\n");
> -    menu_data += strlen(menu_data) + 1;
> +    len = strnlen(menu_data, menu_data_end - menu_data);
> +    menu_data += len + 1;

If we've already hit menu_data_end at this point should we bail rather
than skipping the while() loop and immediately proceeding into
menu_get_boot_index(valid_entries); with an all-false array?

AFAICT the current implementation would print no options and wait for
user input.

e.g. something like

    if (menu_data >= menu_data_end)
        return 0;

>  
>      /* Print entries */
> -    while (*menu_data) {
> -        len = strlen(menu_data);
> +    while (menu_data < menu_data_end && *menu_data) {
> +        len = strnlen(menu_data, menu_data_end - menu_data);
>          entry = zipl_print_entry(menu_data, len);
>          menu_data += len + 1;
>  
> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
> index 1e1f71775e..f6030a6071 100644
> --- a/pc-bios/s390-ccw/s390-ccw.h
> +++ b/pc-bios/s390-ccw/s390-ccw.h
> @@ -76,7 +76,7 @@ void jump_to_low_kernel(void);
>  
>  /* menu.c */
>  void menu_set_parms(uint8_t boot_menu_flag, uint32_t boot_menu_timeout);
> -int menu_get_zipl_boot_index(const char *menu_data);
> +int menu_get_zipl_boot_index(const char *menu_data, const char *menu_data_end);
>  bool menu_is_enabled_zipl(void);
>  int menu_get_enum_boot_index(bool *valid_entries);
>  bool menu_is_enabled_enum(void);

Re: [PATCH 5/5] pc-bios/s390-ccw: bound zipl menu strlen and replace VLA in zipl_print_entry

[Matthew Rosato]

On 7/27/26 11:51 AM, Matthew Rosato wrote:
> On 7/27/26 7:50 AM, Christian Borntraeger wrote:
>> From: Joshua Daley <jdaley@linux.ibm.com>
>>
>> menu_get_zipl_boot_index() calls strlen() on a pointer into the middle
>> of _s2 with no upper bound, so a stage-2 image whose blocks contain no
>> NUL bytes causes strlen() to walk beyond _s2. The resulting length
>> is then used to size a stack VLA in zipl_print_entry(), risking a stack
>> overflow.
>>
>> Fix by:
>>
>> - Implementing strnlen(), a bounded version of strlen().
>>
>> - Adding a menu_data_end parameter to menu_get_zipl_boot_index() and
>>   replacing both strlen() calls with strnlen() bounded by the remaining
>>   buffer space. The loop guard also checks that the pointer has not
>>   reached menu_data_end. For robustness, the function returns 0
>>   (boot default) if somehow menu_data is initially >= menu_data_end.
>>
>> - Replacing the VLA char buf[len + 2] in zipl_print_entry() with a fixed
>>   ZIPL_ENTRY_MAX + 2 (82-byte) buffer and truncating len before use.
>>
>> - Passing s2_end (_s2 + sizeof(_s2)) as menu_data_end at the one call
>>   site in eckd_get_boot_menu_index(), so the bound is exactly the end of
>>   the buffer.
>>
>> Fixes: f7178910845a ("s390-ccw: print zipl boot menu")
>> Signed-off-by: Joshua Daley <jdaley@linux.ibm.com>
>> ---
>>  pc-bios/s390-ccw/bootmap.c  |  4 +++-
>>  pc-bios/s390-ccw/helper.h   | 10 ++++++++++
>>  pc-bios/s390-ccw/menu.c     | 23 ++++++++++++++++++-----
>>  pc-bios/s390-ccw/s390-ccw.h |  2 +-
>>  4 files changed, 32 insertions(+), 7 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
>> index ed6e8cbbc7..81512265ce 100644
>> --- a/pc-bios/s390-ccw/bootmap.c
>> +++ b/pc-bios/s390-ccw/bootmap.c
>> @@ -61,6 +61,7 @@ static uint8_t _s2[MAX_SECTOR_SIZE * 3] __attribute__((__aligned__(PAGE_SIZE)));
>>  static void *s2_prev_blk = _s2;
>>  static void *s2_cur_blk = _s2 + MAX_SECTOR_SIZE;
>>  static void *s2_next_blk = _s2 + MAX_SECTOR_SIZE * 2;
>> +static void *s2_end = _s2 + sizeof(_s2);
>>  
>>  static inline int verify_boot_info(BootInfo *bip)
>>  {
>> @@ -308,7 +309,8 @@ static int eckd_get_boot_menu_index(block_number_t s1b_block_nr)
>>                  }
>>              }
>>  
>> -            return menu_get_zipl_boot_index(s2_cur_blk + banner_offset);
>> +            return menu_get_zipl_boot_index(s2_cur_blk + banner_offset,
>> +                                            s2_end);
>>          }
>>  
>>          prev_block_nr = cur_block_nr;
>> diff --git a/pc-bios/s390-ccw/helper.h b/pc-bios/s390-ccw/helper.h
>> index 8e3dfcb6d6..d9b7da444a 100644
>> --- a/pc-bios/s390-ccw/helper.h
>> +++ b/pc-bios/s390-ccw/helper.h
>> @@ -45,4 +45,14 @@ static inline void sleep(unsigned int seconds)
>>      }
>>  }
>>  
>> +static inline size_t strnlen(const char *s, size_t maxlen)
>> +{
>> +    size_t len = 0;
>> +
>> +    while (len < maxlen && s[len]) {
>> +        len++;
>> +    }
>> +    return len;
>> +}
>> +
>>  #endif
>> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
>> index b6a9a56d46..af96a83de9 100644
>> --- a/pc-bios/s390-ccw/menu.c
>> +++ b/pc-bios/s390-ccw/menu.c
>> @@ -16,6 +16,7 @@
>>  #include "s390-ccw.h"
>>  #include "sclp.h"
>>  #include "s390-time.h"
>> +#include "helper.h"
>>  
>>  #define KEYCODE_NO_INP '\0'
>>  #define KEYCODE_ESCAPE '\033'
>> @@ -26,6 +27,9 @@
>>  #define ZIPL_TIMEOUT_OFFSET 138
>>  #define ZIPL_FLAG_OFFSET    140
>>  
>> +/* Max printable chars for a zipl boot menu entry */
>> +#define ZIPL_ENTRY_MAX 80
>> +
>>  #define TOD_CLOCK_MILLISECOND   0x3e8000
>>  
>>  #define LOW_CORE_EXTERNAL_INT_ADDR   0x86
>> @@ -179,9 +183,13 @@ int menu_get_boot_index(bool *valid_entries)
>>  /* Returns the entry number that was printed, or -1 on invalid entry */
>>  static int zipl_print_entry(const char *data, size_t len)
>>  {
>> -    char buf[len + 2];
>> +    char buf[ZIPL_ENTRY_MAX + 2];
>>      const char *p;
>>  
>> +    if (len > ZIPL_ENTRY_MAX) {
>> +        len = ZIPL_ENTRY_MAX;
>> +    }
>> +
>>      ebcdic_to_ascii(data, buf, len);
>>      buf[len] = '\n';
>>      buf[len + 1] = '\0';
>> @@ -196,7 +204,7 @@ static int zipl_print_entry(const char *data, size_t len)
>>      return atoi(p);
>>  }
>>  
>> -int menu_get_zipl_boot_index(const char *menu_data)
>> +int menu_get_zipl_boot_index(const char *menu_data, const char *menu_data_end)
>>  {
>>      size_t len;
>>      int entry;
>> @@ -212,13 +220,18 @@ int menu_get_zipl_boot_index(const char *menu_data)
>>          timeout = zipl_timeout * 1000;
>>      }
>>  
>> +    if (menu_data >= menu_data_end) {
>> +        return 0;
>> +    }
>> +
>>      /* Print banner */
>>      puts("s390-ccw zIPL Boot Menu\n");
>> -    menu_data += strlen(menu_data) + 1;
>> +    len = strnlen(menu_data, menu_data_end - menu_data);
>> +    menu_data += len + 1;
> 
> If we've already hit menu_data_end at this point should we bail rather
> than skipping the while() loop and immediately proceeding into
> menu_get_boot_index(valid_entries); with an all-false array?
> 
> AFAICT the current implementation would print no options and wait for
> user input.
> 
> e.g. something like
> 
>     if (menu_data >= menu_data_end)
>         return 0;

Hm, well, my proposal would still print the s390-ccw zIPL Boot Menu\n
message but then quietly use the default boot option.

Maybe it would be better to move the
+    len = strnlen(menu_data, menu_data_end - menu_data);
+    menu_data += len + 1;

up a few lines before the puts and combine it with the menu_data >=
menu_data_end check you already added?  That way in both cases we
silently take the default option without printing anything?


> 
>>  
>>      /* Print entries */
>> -    while (*menu_data) {
>> -        len = strlen(menu_data);
>> +    while (menu_data < menu_data_end && *menu_data) {
>> +        len = strnlen(menu_data, menu_data_end - menu_data);
>>          entry = zipl_print_entry(menu_data, len);
>>          menu_data += len + 1;
>>  
>> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
>> index 1e1f71775e..f6030a6071 100644
>> --- a/pc-bios/s390-ccw/s390-ccw.h
>> +++ b/pc-bios/s390-ccw/s390-ccw.h
>> @@ -76,7 +76,7 @@ void jump_to_low_kernel(void);
>>  
>>  /* menu.c */
>>  void menu_set_parms(uint8_t boot_menu_flag, uint32_t boot_menu_timeout);
>> -int menu_get_zipl_boot_index(const char *menu_data);
>> +int menu_get_zipl_boot_index(const char *menu_data, const char *menu_data_end);
>>  bool menu_is_enabled_zipl(void);
>>  int menu_get_enum_boot_index(bool *valid_entries);
>>  bool menu_is_enabled_enum(void);
> 

Re: [PATCH 5/5] pc-bios/s390-ccw: bound zipl menu strlen and replace VLA in zipl_print_entry

[Joshua Daley]

On 7/27/2026 12:02 PM, Matthew Rosato wrote:
> On 7/27/26 11:51 AM, Matthew Rosato wrote:
>> On 7/27/26 7:50 AM, Christian Borntraeger wrote:
>>> From: Joshua Daley <jdaley@linux.ibm.com>
>>>
>>> menu_get_zipl_boot_index() calls strlen() on a pointer into the middle
>>> of _s2 with no upper bound, so a stage-2 image whose blocks contain no
>>> NUL bytes causes strlen() to walk beyond _s2. The resulting length
>>> is then used to size a stack VLA in zipl_print_entry(), risking a stack
>>> overflow.
>>>
>>> Fix by:
>>>
>>> - Implementing strnlen(), a bounded version of strlen().
>>>
>>> - Adding a menu_data_end parameter to menu_get_zipl_boot_index() and
>>>    replacing both strlen() calls with strnlen() bounded by the remaining
>>>    buffer space. The loop guard also checks that the pointer has not
>>>    reached menu_data_end. For robustness, the function returns 0
>>>    (boot default) if somehow menu_data is initially >= menu_data_end.
>>>
>>> - Replacing the VLA char buf[len + 2] in zipl_print_entry() with a fixed
>>>    ZIPL_ENTRY_MAX + 2 (82-byte) buffer and truncating len before use.
>>>
>>> - Passing s2_end (_s2 + sizeof(_s2)) as menu_data_end at the one call
>>>    site in eckd_get_boot_menu_index(), so the bound is exactly the end of
>>>    the buffer.
>>>
>>> Fixes: f7178910845a ("s390-ccw: print zipl boot menu")
>>> Signed-off-by: Joshua Daley <jdaley@linux.ibm.com>
>>> ---
>>>   pc-bios/s390-ccw/bootmap.c  |  4 +++-
>>>   pc-bios/s390-ccw/helper.h   | 10 ++++++++++
>>>   pc-bios/s390-ccw/menu.c     | 23 ++++++++++++++++++-----
>>>   pc-bios/s390-ccw/s390-ccw.h |  2 +-
>>>   4 files changed, 32 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
>>> index ed6e8cbbc7..81512265ce 100644
>>> --- a/pc-bios/s390-ccw/bootmap.c
>>> +++ b/pc-bios/s390-ccw/bootmap.c
>>> @@ -61,6 +61,7 @@ static uint8_t _s2[MAX_SECTOR_SIZE * 3] __attribute__((__aligned__(PAGE_SIZE)));
>>>   static void *s2_prev_blk = _s2;
>>>   static void *s2_cur_blk = _s2 + MAX_SECTOR_SIZE;
>>>   static void *s2_next_blk = _s2 + MAX_SECTOR_SIZE * 2;
>>> +static void *s2_end = _s2 + sizeof(_s2);
>>>   
>>>   static inline int verify_boot_info(BootInfo *bip)
>>>   {
>>> @@ -308,7 +309,8 @@ static int eckd_get_boot_menu_index(block_number_t s1b_block_nr)
>>>                   }
>>>               }
>>>   
>>> -            return menu_get_zipl_boot_index(s2_cur_blk + banner_offset);
>>> +            return menu_get_zipl_boot_index(s2_cur_blk + banner_offset,
>>> +                                            s2_end);
>>>           }
>>>   
>>>           prev_block_nr = cur_block_nr;
>>> diff --git a/pc-bios/s390-ccw/helper.h b/pc-bios/s390-ccw/helper.h
>>> index 8e3dfcb6d6..d9b7da444a 100644
>>> --- a/pc-bios/s390-ccw/helper.h
>>> +++ b/pc-bios/s390-ccw/helper.h
>>> @@ -45,4 +45,14 @@ static inline void sleep(unsigned int seconds)
>>>       }
>>>   }
>>>   
>>> +static inline size_t strnlen(const char *s, size_t maxlen)
>>> +{
>>> +    size_t len = 0;
>>> +
>>> +    while (len < maxlen && s[len]) {
>>> +        len++;
>>> +    }
>>> +    return len;
>>> +}
>>> +
>>>   #endif
>>> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
>>> index b6a9a56d46..af96a83de9 100644
>>> --- a/pc-bios/s390-ccw/menu.c
>>> +++ b/pc-bios/s390-ccw/menu.c
>>> @@ -16,6 +16,7 @@
>>>   #include "s390-ccw.h"
>>>   #include "sclp.h"
>>>   #include "s390-time.h"
>>> +#include "helper.h"
>>>   
>>>   #define KEYCODE_NO_INP '\0'
>>>   #define KEYCODE_ESCAPE '\033'
>>> @@ -26,6 +27,9 @@
>>>   #define ZIPL_TIMEOUT_OFFSET 138
>>>   #define ZIPL_FLAG_OFFSET    140
>>>   
>>> +/* Max printable chars for a zipl boot menu entry */
>>> +#define ZIPL_ENTRY_MAX 80
>>> +
>>>   #define TOD_CLOCK_MILLISECOND   0x3e8000
>>>   
>>>   #define LOW_CORE_EXTERNAL_INT_ADDR   0x86
>>> @@ -179,9 +183,13 @@ int menu_get_boot_index(bool *valid_entries)
>>>   /* Returns the entry number that was printed, or -1 on invalid entry */
>>>   static int zipl_print_entry(const char *data, size_t len)
>>>   {
>>> -    char buf[len + 2];
>>> +    char buf[ZIPL_ENTRY_MAX + 2];
>>>       const char *p;
>>>   
>>> +    if (len > ZIPL_ENTRY_MAX) {
>>> +        len = ZIPL_ENTRY_MAX;
>>> +    }
>>> +
>>>       ebcdic_to_ascii(data, buf, len);
>>>       buf[len] = '\n';
>>>       buf[len + 1] = '\0';
>>> @@ -196,7 +204,7 @@ static int zipl_print_entry(const char *data, size_t len)
>>>       return atoi(p);
>>>   }
>>>   
>>> -int menu_get_zipl_boot_index(const char *menu_data)
>>> +int menu_get_zipl_boot_index(const char *menu_data, const char *menu_data_end)
>>>   {
>>>       size_t len;
>>>       int entry;
>>> @@ -212,13 +220,18 @@ int menu_get_zipl_boot_index(const char *menu_data)
>>>           timeout = zipl_timeout * 1000;
>>>       }
>>>   
>>> +    if (menu_data >= menu_data_end) {
>>> +        return 0;
>>> +    }
>>> +
>>>       /* Print banner */
>>>       puts("s390-ccw zIPL Boot Menu\n");
>>> -    menu_data += strlen(menu_data) + 1;
>>> +    len = strnlen(menu_data, menu_data_end - menu_data);
>>> +    menu_data += len + 1;
>>
>> If we've already hit menu_data_end at this point should we bail rather
>> than skipping the while() loop and immediately proceeding into
>> menu_get_boot_index(valid_entries); with an all-false array?
>>
>> AFAICT the current implementation would print no options and wait for
>> user input.
Right.

>>
>> e.g. something like
>>
>>      if (menu_data >= menu_data_end)
>>          return 0;
> 
> Hm, well, my proposal would still print the s390-ccw zIPL Boot Menu\n
> message but then quietly use the default boot option.
> 
> Maybe it would be better to move the
> +    len = strnlen(menu_data, menu_data_end - menu_data);
> +    menu_data += len + 1;
> 
> up a few lines before the puts and combine it with the menu_data >=
> menu_data_end check you already added?  That way in both cases we
> silently take the default option without printing anything?
> 

I agree with your assessment. Need another check after the "banner skip" and
before the puts().


if (menu_data >= menu_data_end) {
     return 0; /* Boot default */
}

/* Skip banner */
len = strnlen(menu_data, menu_data_end - menu_data);
menu_data += len + 1;

if (menu_data >= menu_data_end || !(*menu_data)) {
     return 0; /* No entries, boot default */
}

/* Print banner */
puts("s390-ccw zIPL Boot Menu\n");

/* Print entries */
while (menu_data < menu_data_end && *menu_data) {
...

> 
>>
>>>   
>>>       /* Print entries */
>>> -    while (*menu_data) {
>>> -        len = strlen(menu_data);
>>> +    while (menu_data < menu_data_end && *menu_data) {
>>> +        len = strnlen(menu_data, menu_data_end - menu_data);
>>>           entry = zipl_print_entry(menu_data, len);
>>>           menu_data += len + 1;
>>>   
>>> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
>>> index 1e1f71775e..f6030a6071 100644
>>> --- a/pc-bios/s390-ccw/s390-ccw.h
>>> +++ b/pc-bios/s390-ccw/s390-ccw.h
>>> @@ -76,7 +76,7 @@ void jump_to_low_kernel(void);
>>>   
>>>   /* menu.c */
>>>   void menu_set_parms(uint8_t boot_menu_flag, uint32_t boot_menu_timeout);
>>> -int menu_get_zipl_boot_index(const char *menu_data);
>>> +int menu_get_zipl_boot_index(const char *menu_data, const char *menu_data_end);
>>>   bool menu_is_enabled_zipl(void);
>>>   int menu_get_enum_boot_index(bool *valid_entries);
>>>   bool menu_is_enabled_enum(void);
>>
> 

Re: [PATCH 5/5] pc-bios/s390-ccw: bound zipl menu strlen and replace VLA in zipl_print_entry

[Eric Farman]

On 7/27/26 1:32 PM, Joshua Daley wrote:
> On 7/27/2026 12:02 PM, Matthew Rosato wrote:
>> On 7/27/26 11:51 AM, Matthew Rosato wrote:
>>> On 7/27/26 7:50 AM, Christian Borntraeger wrote:
>>>> From: Joshua Daley <jdaley@linux.ibm.com>
>>>>
>>>> menu_get_zipl_boot_index() calls strlen() on a pointer into the middle
>>>> of _s2 with no upper bound, so a stage-2 image whose blocks contain no
>>>> NUL bytes causes strlen() to walk beyond _s2. The resulting length
>>>> is then used to size a stack VLA in zipl_print_entry(), risking a stack
>>>> overflow.
>>>>
>>>> Fix by:
>>>>
>>>> - Implementing strnlen(), a bounded version of strlen().
>>>>
>>>> - Adding a menu_data_end parameter to menu_get_zipl_boot_index() and
>>>>    replacing both strlen() calls with strnlen() bounded by the 
>>>> remaining
>>>>    buffer space. The loop guard also checks that the pointer has not
>>>>    reached menu_data_end. For robustness, the function returns 0
>>>>    (boot default) if somehow menu_data is initially >= menu_data_end.
>>>>
>>>> - Replacing the VLA char buf[len + 2] in zipl_print_entry() with a 
>>>> fixed
>>>>    ZIPL_ENTRY_MAX + 2 (82-byte) buffer and truncating len before use.
>>>>
>>>> - Passing s2_end (_s2 + sizeof(_s2)) as menu_data_end at the one call
>>>>    site in eckd_get_boot_menu_index(), so the bound is exactly the 
>>>> end of
>>>>    the buffer.
>>>>
>>>> Fixes: f7178910845a ("s390-ccw: print zipl boot menu")
>>>> Signed-off-by: Joshua Daley <jdaley@linux.ibm.com>
>>>> ---
>>>>   pc-bios/s390-ccw/bootmap.c  |  4 +++-
>>>>   pc-bios/s390-ccw/helper.h   | 10 ++++++++++
>>>>   pc-bios/s390-ccw/menu.c     | 23 ++++++++++++++++++-----
>>>>   pc-bios/s390-ccw/s390-ccw.h |  2 +-
>>>>   4 files changed, 32 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
>>>> index ed6e8cbbc7..81512265ce 100644
>>>> --- a/pc-bios/s390-ccw/bootmap.c
>>>> +++ b/pc-bios/s390-ccw/bootmap.c
>>>> @@ -61,6 +61,7 @@ static uint8_t _s2[MAX_SECTOR_SIZE * 3] 
>>>> __attribute__((__aligned__(PAGE_SIZE)));
>>>>   static void *s2_prev_blk = _s2;
>>>>   static void *s2_cur_blk = _s2 + MAX_SECTOR_SIZE;
>>>>   static void *s2_next_blk = _s2 + MAX_SECTOR_SIZE * 2;
>>>> +static void *s2_end = _s2 + sizeof(_s2);
>>>>   static inline int verify_boot_info(BootInfo *bip)
>>>>   {
>>>> @@ -308,7 +309,8 @@ static int 
>>>> eckd_get_boot_menu_index(block_number_t s1b_block_nr)
>>>>                   }
>>>>               }
>>>> -            return menu_get_zipl_boot_index(s2_cur_blk + 
>>>> banner_offset);
>>>> +            return menu_get_zipl_boot_index(s2_cur_blk + 
>>>> banner_offset,
>>>> +                                            s2_end);
>>>>           }
>>>>           prev_block_nr = cur_block_nr;
>>>> diff --git a/pc-bios/s390-ccw/helper.h b/pc-bios/s390-ccw/helper.h
>>>> index 8e3dfcb6d6..d9b7da444a 100644
>>>> --- a/pc-bios/s390-ccw/helper.h
>>>> +++ b/pc-bios/s390-ccw/helper.h
>>>> @@ -45,4 +45,14 @@ static inline void sleep(unsigned int seconds)
>>>>       }
>>>>   }
>>>> +static inline size_t strnlen(const char *s, size_t maxlen)
>>>> +{
>>>> +    size_t len = 0;
>>>> +
>>>> +    while (len < maxlen && s[len]) {
>>>> +        len++;
>>>> +    }
>>>> +    return len;
>>>> +}
>>>> +
>>>>   #endif
>>>> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
>>>> index b6a9a56d46..af96a83de9 100644
>>>> --- a/pc-bios/s390-ccw/menu.c
>>>> +++ b/pc-bios/s390-ccw/menu.c
>>>> @@ -16,6 +16,7 @@
>>>>   #include "s390-ccw.h"
>>>>   #include "sclp.h"
>>>>   #include "s390-time.h"
>>>> +#include "helper.h"
>>>>   #define KEYCODE_NO_INP '\0'
>>>>   #define KEYCODE_ESCAPE '\033'
>>>> @@ -26,6 +27,9 @@
>>>>   #define ZIPL_TIMEOUT_OFFSET 138
>>>>   #define ZIPL_FLAG_OFFSET    140
>>>> +/* Max printable chars for a zipl boot menu entry */
>>>> +#define ZIPL_ENTRY_MAX 80
>>>> +
>>>>   #define TOD_CLOCK_MILLISECOND   0x3e8000
>>>>   #define LOW_CORE_EXTERNAL_INT_ADDR   0x86
>>>> @@ -179,9 +183,13 @@ int menu_get_boot_index(bool *valid_entries)
>>>>   /* Returns the entry number that was printed, or -1 on invalid 
>>>> entry */
>>>>   static int zipl_print_entry(const char *data, size_t len)
>>>>   {
>>>> -    char buf[len + 2];
>>>> +    char buf[ZIPL_ENTRY_MAX + 2];
>>>>       const char *p;
>>>> +    if (len > ZIPL_ENTRY_MAX) {
>>>> +        len = ZIPL_ENTRY_MAX;
>>>> +    }
>>>> +
>>>>       ebcdic_to_ascii(data, buf, len);
>>>>       buf[len] = '\n';
>>>>       buf[len + 1] = '\0';
>>>> @@ -196,7 +204,7 @@ static int zipl_print_entry(const char *data, 
>>>> size_t len)
>>>>       return atoi(p);
>>>>   }
>>>> -int menu_get_zipl_boot_index(const char *menu_data)
>>>> +int menu_get_zipl_boot_index(const char *menu_data, const char 
>>>> *menu_data_end)
>>>>   {
>>>>       size_t len;
>>>>       int entry;
>>>> @@ -212,13 +220,18 @@ int menu_get_zipl_boot_index(const char 
>>>> *menu_data)
>>>>           timeout = zipl_timeout * 1000;
>>>>       }
>>>> +    if (menu_data >= menu_data_end) {
>>>> +        return 0;
>>>> +    }
>>>> +
>>>>       /* Print banner */
>>>>       puts("s390-ccw zIPL Boot Menu\n");
>>>> -    menu_data += strlen(menu_data) + 1;
>>>> +    len = strnlen(menu_data, menu_data_end - menu_data);
>>>> +    menu_data += len + 1;
>>>
>>> If we've already hit menu_data_end at this point should we bail rather
>>> than skipping the while() loop and immediately proceeding into
>>> menu_get_boot_index(valid_entries); with an all-false array?
>>>
>>> AFAICT the current implementation would print no options and wait for
>>> user input.
> Right.
> 
>>>
>>> e.g. something like
>>>
>>>      if (menu_data >= menu_data_end)
>>>          return 0;
>>
>> Hm, well, my proposal would still print the s390-ccw zIPL Boot Menu\n
>> message but then quietly use the default boot option.
>>
>> Maybe it would be better to move the
>> +    len = strnlen(menu_data, menu_data_end - menu_data);
>> +    menu_data += len + 1;
>>
>> up a few lines before the puts and combine it with the menu_data >=
>> menu_data_end check you already added?  That way in both cases we
>> silently take the default option without printing anything?
>>
> 
> I agree with your assessment. Need another check after the "banner skip" 
> and
> before the puts().
> 
> 
> if (menu_data >= menu_data_end) {
>      return 0; /* Boot default */
> }
> 
> /* Skip banner */
> len = strnlen(menu_data, menu_data_end - menu_data);
> menu_data += len + 1;
> 
> if (menu_data >= menu_data_end || !(*menu_data)) {
>      return 0; /* No entries, boot default */
> }
> 
> /* Print banner */
> puts("s390-ccw zIPL Boot Menu\n");
> 
> /* Print entries */
> while (menu_data < menu_data_end && *menu_data) {
> ...

@Joshua, can you send a v2 of just this patch? I'll pick up the first 
four with the little edits.

> 
>>
>>>
>>>>       /* Print entries */
>>>> -    while (*menu_data) {
>>>> -        len = strlen(menu_data);
>>>> +    while (menu_data < menu_data_end && *menu_data) {
>>>> +        len = strnlen(menu_data, menu_data_end - menu_data);
>>>>           entry = zipl_print_entry(menu_data, len);
>>>>           menu_data += len + 1;
>>>> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
>>>> index 1e1f71775e..f6030a6071 100644
>>>> --- a/pc-bios/s390-ccw/s390-ccw.h
>>>> +++ b/pc-bios/s390-ccw/s390-ccw.h
>>>> @@ -76,7 +76,7 @@ void jump_to_low_kernel(void);
>>>>   /* menu.c */
>>>>   void menu_set_parms(uint8_t boot_menu_flag, uint32_t 
>>>> boot_menu_timeout);
>>>> -int menu_get_zipl_boot_index(const char *menu_data);
>>>> +int menu_get_zipl_boot_index(const char *menu_data, const char 
>>>> *menu_data_end);
>>>>   bool menu_is_enabled_zipl(void);
>>>>   int menu_get_enum_boot_index(bool *valid_entries);
>>>>   bool menu_is_enabled_enum(void);
>>>
>>
> 
> 

Re: [PATCH 0/5] more hardening fixes for s390

[Eric Farman]

On 7/27/26 7:50 AM, Christian Borntraeger wrote:
> Cornelia, Eric, Matt,
> 
> here are 5 more fixes for hardening QEMU against invalid input. Nothing
> is strictly a security issues as defined in the policy. For example
> https://qemu-project.gitlab.io/qemu/system/security.html says assert /
> abort: If triggering the code path requires kernel privileges (or root
> account access) in the guest, asserts/aborts in QEMU are a self
> inflicted denial of service. These will not be treated as security
> flaws, at most hardening bugs.
> And if anyone can provide invalid boot loader content it can provide
> anything to boot if secure boot is not available.
> We should fix those anyway, probably even for 11.1
> 
> Conny, I did not rebuild the s390-ccw, I assume you will do that?
> 
> Christian Borntraeger (1):
>    hw/char/sclpconsole-lm: avoid guest triggerable assert
> 
> Joshua Daley (4):
>    s390x/ipl: validate num_comp against iplb length before iterating
>    pc-bios/s390-ccw: fix out-of-bounds read in iso_get_file_size()
>    pc-bios/s390-ccw: bounds-check zipl menu entry index before array
>      write
>    pc-bios/s390-ccw: bound zipl menu strlen and replace VLA in
>      zipl_print_entry
> 
>   hw/char/sclpconsole-lm.c    |  3 ++-
>   hw/s390x/ipl.h              |  6 ++++++
>   pc-bios/s390-ccw/bootmap.c  |  6 ++++--
>   pc-bios/s390-ccw/helper.h   | 10 ++++++++++
>   pc-bios/s390-ccw/menu.c     | 36 +++++++++++++++++++++++++++++-------
>   pc-bios/s390-ccw/s390-ccw.h |  2 +-
>   6 files changed, 52 insertions(+), 11 deletions(-)
> 

These have been running on our systems the last few days.
For the series:

Reviewed-by: Eric Farman <farman@linux.ibm.com>

Re: [PATCH 0/5] more hardening fixes for s390

[Cornelia Huck]

On Mon, Jul 27 2026, Christian Borntraeger <borntraeger@linux.ibm.com> wrote:

> Cornelia, Eric, Matt,
>
> here are 5 more fixes for hardening QEMU against invalid input. Nothing
> is strictly a security issues as defined in the policy. For example
> https://qemu-project.gitlab.io/qemu/system/security.html says assert /
> abort: If triggering the code path requires kernel privileges (or root
> account access) in the guest, asserts/aborts in QEMU are a self
> inflicted denial of service. These will not be treated as security
> flaws, at most hardening bugs.
> And if anyone can provide invalid boot loader content it can provide
> anything to boot if secure boot is not available.
> We should fix those anyway, probably even for 11.1 

Fixing: yes. For 11.1: how easy are they to trigger? For the bios fixes,
I assume you need a broken/crafted disk image; I assume that for the
other two, you need some buggy/misbehaving guest. Did you actually
manage to trigger this via some kind of test case?

>
> Conny, I did not rebuild the s390-ccw, I assume you will do that?

Yes, the bios rebuild will be done by whoever ends up applying the
patches.

>
> Christian Borntraeger (1):
>   hw/char/sclpconsole-lm: avoid guest triggerable assert
>
> Joshua Daley (4):
>   s390x/ipl: validate num_comp against iplb length before iterating
>   pc-bios/s390-ccw: fix out-of-bounds read in iso_get_file_size()
>   pc-bios/s390-ccw: bounds-check zipl menu entry index before array
>     write
>   pc-bios/s390-ccw: bound zipl menu strlen and replace VLA in
>     zipl_print_entry
>
>  hw/char/sclpconsole-lm.c    |  3 ++-
>  hw/s390x/ipl.h              |  6 ++++++
>  pc-bios/s390-ccw/bootmap.c  |  6 ++++--
>  pc-bios/s390-ccw/helper.h   | 10 ++++++++++
>  pc-bios/s390-ccw/menu.c     | 36 +++++++++++++++++++++++++++++-------
>  pc-bios/s390-ccw/s390-ccw.h |  2 +-
>  6 files changed, 52 insertions(+), 11 deletions(-)

Re: [PATCH 0/5] more hardening fixes for s390

[Christian Borntraeger]

Am 27.07.26 um 14:14 schrieb Cornelia Huck:
> On Mon, Jul 27 2026, Christian Borntraeger <borntraeger@linux.ibm.com> wrote:
> 
>> Cornelia, Eric, Matt,
>>
>> here are 5 more fixes for hardening QEMU against invalid input. Nothing
>> is strictly a security issues as defined in the policy. For example
>> https://qemu-project.gitlab.io/qemu/system/security.html says assert /
>> abort: If triggering the code path requires kernel privileges (or root
>> account access) in the guest, asserts/aborts in QEMU are a self
>> inflicted denial of service. These will not be treated as security
>> flaws, at most hardening bugs.
>> And if anyone can provide invalid boot loader content it can provide
>> anything to boot if secure boot is not available.
>> We should fix those anyway, probably even for 11.1
> 
> Fixing: yes. For 11.1: how easy are they to trigger? For the bios fixes,
> I assume you need a broken/crafted disk image; I assume that for the
> other two, you need some buggy/misbehaving guest. Did you actually
> manage to trigger this via some kind of test case?

Easy to trigger, but only with modified guests or modified zipls. Normal
guests will not trigger those.

Re: [PATCH 0/5] more hardening fixes for s390

[Cornelia Huck]

On Mon, Jul 27 2026, Christian Borntraeger <borntraeger@linux.ibm.com> wrote:

> Am 27.07.26 um 14:14 schrieb Cornelia Huck:
>> On Mon, Jul 27 2026, Christian Borntraeger <borntraeger@linux.ibm.com> wrote:
>> 
>>> Cornelia, Eric, Matt,
>>>
>>> here are 5 more fixes for hardening QEMU against invalid input. Nothing
>>> is strictly a security issues as defined in the policy. For example
>>> https://qemu-project.gitlab.io/qemu/system/security.html says assert /
>>> abort: If triggering the code path requires kernel privileges (or root
>>> account access) in the guest, asserts/aborts in QEMU are a self
>>> inflicted denial of service. These will not be treated as security
>>> flaws, at most hardening bugs.
>>> And if anyone can provide invalid boot loader content it can provide
>>> anything to boot if secure boot is not available.
>>> We should fix those anyway, probably even for 11.1
>> 
>> Fixing: yes. For 11.1: how easy are they to trigger? For the bios fixes,
>> I assume you need a broken/crafted disk image; I assume that for the
>> other two, you need some buggy/misbehaving guest. Did you actually
>> manage to trigger this via some kind of test case?
>
> Easy to trigger, but only with modified guests or modified zipls. Normal
> guests will not trigger those.

Ok, so I think these would be suitable for the next release, but maybe
with a cc:stable.