On 7/27/26 11:51 AM, Matthew Rosato wrote:
> 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;
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);
>
