Re: [PATCH qemu] pc-bios/s390-ccw: Fix off-by-one errors with loadparm and boot entries

On 8/17/26 5:58 AM, Michael Tokarev wrote:
> On 7/29/26 01:30, [email protected] wrote:
>> From: Jared Rossi <[email protected]>
>>
>> The loadparm may optionally be used to select a boot entry, with the
>> intended range being 0 through 31 inclusive, for a total of 32 entries.
>> Previously, MAX_BOOT_ENTRIES was defined as 31, indicating that it was
>> intended to correspond to the index of the boot entry rather than the
>> count; however, some guards also used MAX_BOOT_ENTRIES as a count of the
>> maximum allowed entries, which resulted in a mismatch between the 
>> intended
>> and actual range such that index 31 could never be used in practice.
>>
>> Move the definition of MAX_BOOT_ENTRIES to qipl.h so it is shared and
>> change the value to 32, representing a count of the maximum number of
>> allowed boot entries and allowing the loadparm to accept values 0 
>> through
>> 31 as intended.  Update some instances in the netboot code where
>> MAX_BOOT_ENTRIES was used as the max index so that all guards treat
>> MAX_BOOT_ENTRIES as a count across all boot methods.
>>
>> Cc: [email protected]
>> Fixes: 806315279d5c ("pc-bios/s390-ccw: Remove panics from ECKD IPL 
>> path")
>> Signed-off-by: Jared Rossi <[email protected]>
>
> While this commit suggests that the original issue were introduced in
> 806315279d5c (which is qemu v9.2.0), it looks like actual bug has
> become possible only after a different commit, a4adf071dc7
> ("pc-bios/s390-ccw: Allow to select a different pxelinux.cfg entry via 
> loadparm"),
> which is only qemu v11.0.0.
>
> So I'm not picking this change to 10.0.x stable series (at least not 
> until
> a4adf071dc7 is there too, which I don't think is necessary).
>
> Please let me know if I should, instead, pick both of the commits to
> 10.0.x, or should rework this change to work on 10.0.x instead (which -
> it seems - is not necessary).
>
> Thanks,
>
> /mjt

Hi Michael,

I see what you mean.  I think an argument can be made for any case, but
I’m not sure what is most desirable for QEMU stable.  I’ll try to clarify
the problem.

The off-by-one error itself was introduced by

806315279d5c ("pc-bios/s390-ccw: Remove panics from ECKD IPL path")

which changed the loop conditions to treat MAX_BOOT_ENTRIES as a count
instead of an index, but didn’t update the value to match.  The result is
simply that the maximum loadparm value accepted by QEMU is silently reduced
from 31 to 30, which is a mismatch between the stated max in documentation
versus the real implementation, but otherwise doesn't cause any problems in
practice because the logic is consistent.

The commit you reference

a4adf071dc7 ("pc-bios/s390-ccw: Allow to select a different pxelinux.cfg 
entry via loadparm")

introduces an actual trigger for the off-by-one error by creating an array
access that uses MAX_BOOT_ENTRIES as an index, which would be out of
bounds.  There is now a logic mismatch where MAX_BOOT_ENTRIES is used as
the number of elements in the array, but then it is also used as the index
of the last element in the array.

So, as I see it, there are three possible approaches:

1) Do nothing for now, which means the maximum loadparm value is
effectively 30 instead of 31 until both a4adf071dc7 and the proposed fix
are applied together later, but it is otherwise fine

2) Apply both a4adf071dc7 and the fix to 10.0.x stable, which is the
correct long-term solution but requires extra changes to add the
prerequisite PXE menu feature now

3) Apply a modified fix to 10.0.x stable, which resolves the off-by-one
error at the current level, but is incompatible if/when a4adf071dc7 is
applied later

Option 3 would simply mean changing MAX_BOOT_ENTRIES to 32 for the moment,
with the other changes from the proposed fix to be applied together with
a4adf071dc7 later.

Let me know if anything is unclear, or if you would like any further action
from my side.

Thanks,
Jared Rossi

>
>>   include/hw/s390x/ipl/qipl.h |  2 ++
>>   pc-bios/s390-ccw/netmain.c  | 11 +++++++----
>>   pc-bios/s390-ccw/s390-ccw.h |  2 --
>>   3 files changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/hw/s390x/ipl/qipl.h b/include/hw/s390x/ipl/qipl.h
>> index 8d3c83a80b..b390f2f112 100644
>> --- a/include/hw/s390x/ipl/qipl.h
>> +++ b/include/hw/s390x/ipl/qipl.h
>> @@ -20,6 +20,8 @@
>>   #define LOADPARM_LEN    8
>>   #define NO_LOADPARM "\0\0\0\0\0\0\0\0"
>>   +#define MAX_BOOT_ENTRIES  32
>> +
>>   enum S390IplType {
>>       S390_IPL_TYPE_FCP = 0x00,
>>       S390_IPL_TYPE_CCW = 0x02,
>> diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
>> index 651cedf6ef..791854fce0 100644
>> --- a/pc-bios/s390-ccw/netmain.c
>> +++ b/pc-bios/s390-ccw/netmain.c
>> @@ -40,6 +40,9 @@
>>   #define DEFAULT_BOOT_RETRIES 10
>>   #define DEFAULT_TFTP_RETRIES 20
>>   +/* Index 0 is reserved for default alias, start PXE cfg indices at 
>> 1 */
>> +#define PXECFG_MAX              (MAX_BOOT_ENTRIES - 1)
>> +
>>   extern char _start[];
>>     #define KERNEL_ADDR             ((void *)0L)
>> @@ -381,13 +384,13 @@ static int 
>> net_select_and_load_kernel(filename_ip_t *fn_ip,
>>     static int net_try_pxelinux_cfg(filename_ip_t *fn_ip)
>>   {
>> -    struct pl_cfg_entry entries[MAX_BOOT_ENTRIES];
>> +    struct pl_cfg_entry entries[PXECFG_MAX];
>>       int num_ent, def_ent = 0;
>>         num_ent = pxelinux_load_parse_cfg(fn_ip, mac, get_uuid(),
>>                                         DEFAULT_TFTP_RETRIES,
>>                                         cfgbuf, sizeof(cfgbuf),
>> -                                      entries, MAX_BOOT_ENTRIES, 
>> &def_ent);
>> +                                      entries, PXECFG_MAX, &def_ent);
>>         return net_select_and_load_kernel(fn_ip, num_ent, def_ent, 
>> entries);
>>   }
>> @@ -470,11 +473,11 @@ static int 
>> net_try_direct_tftp_load(filename_ip_t *fn_ip)
>>            * a magic comment string.
>>            */
>>           if (!strncasecmp("# pxelinux", cfgbuf, 10)) {
>> -            struct pl_cfg_entry entries[MAX_BOOT_ENTRIES];
>> +            struct pl_cfg_entry entries[PXECFG_MAX];
>>               int num_ent, def_ent = 0;
>>                 num_ent = pxelinux_parse_cfg(cfgbuf, sizeof(cfgbuf), 
>> entries,
>> -                                         MAX_BOOT_ENTRIES, &def_ent);
>> +                                         PXECFG_MAX, &def_ent);
>>               return net_select_and_load_kernel(fn_ip, num_ent, def_ent,
>>                                                 entries);
>>           }
>> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
>> index 1e1f71775e..d0498e2944 100644
>> --- a/pc-bios/s390-ccw/s390-ccw.h
>> +++ b/pc-bios/s390-ccw/s390-ccw.h
>> @@ -82,8 +82,6 @@ int menu_get_enum_boot_index(bool *valid_entries);
>>   bool menu_is_enabled_enum(void);
>>   int menu_get_boot_index(bool *valid_entries);
>>   -#define MAX_BOOT_ENTRIES  31
>> -
>>   __attribute__ ((__noreturn__))
>>   static inline void panic(const char *string)
>>   {
>