On 7/27/26 7:50 AM, Christian Borntraeger wrote:
> From: Joshua Daley <[email protected]>
>
> 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 <[email protected]>
> ---
> 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);
