On 7/28/26 9:47 AM, Joshua Daley wrote: > 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().
Thanks Josh! One other minor comment... Nit: it might be worth clarifying why we must implement our own strnlen() where we were previously using strlen() without any custom implementation. Something like: 's390-ccw uses libc from SLOF, which includes strlen() but does not have an implementation of strnlen(), so we must implement our own.' Either way: Reviewed-by: Matthew Rosato <[email protected]> > > - 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. The function returns 0 (boot default) if > somehow menu_data reaches menu_data_end before printing any entries. > > - 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 <[email protected]> > --- > pc-bios/s390-ccw/bootmap.c | 4 +++- > pc-bios/s390-ccw/helper.h | 10 ++++++++++ > pc-bios/s390-ccw/menu.c | 29 +++++++++++++++++++++++------ > pc-bios/s390-ccw/s390-ccw.h | 2 +- > 4 files changed, 37 insertions(+), 8 deletions(-) > > diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c > index 420ee32eff..662265171b 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..9b81154b0e 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,22 @@ int menu_get_zipl_boot_index(const char *menu_data) > timeout = zipl_timeout * 1000; > } > > - /* Print banner */ > + 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 */ > + } > + > puts("s390-ccw zIPL Boot Menu\n"); > - menu_data += strlen(menu_data) + 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);
