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

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

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