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

On 7/28/2026 1:34 PM, Matthew Rosato wrote:
> 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]>
> 
Yeah, I think that's good info to include. Eric, if you are otherwise ready
to pick up this patch, can you append that sentence to this first bullet?

Thanks for your review, Matt.

>>
>> - 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);
>