Re: [PATCH v2 1/3] platform/x86: hp-bioscfg: pass validated element count to package parsers

[Muhammad Bilal]

Hi Ilpo,

I've posted v3 of the series, which splits the prerequisite changes into
separate patches and updates the commit messages based on your review.

https://lore.kernel.org/all/20260707202111.35414-1-meatuni001@xxxxxxxxx/

Thanks for your review.

On Mon, Jul 6, 2026 at 10:57 PM Ilpo Järvinen
<ilpo.jarvinen@xxxxxxxxxxxxxxx> wrote:
>
> On Sat, 4 Jul 2026, Muhammad Bilal wrote:
>
> > hp_init_bios_package_attribute() validates obj->package.count against
> > min_elements and then hands off elements = obj->package.elements to
>
> quoting elements = obj->package.elements statement in this context is
> confusing. I don't think it's usually necessary to copy code like this to
> changelog, here you can just state "elements" (which even happens to match
> the struct member's name even).
>
> > one of the five per-type hp_populate_*_package_data() wrappers
> > (string, integer, enumeration, ordered list, password). None of these
> > wrappers receive that validated count. Instead each one re-derives it
> > locally:
> >
> >   hp_populate_integer_elements_from_package(integer_obj,
> >                                             integer_obj->package.count,
> >                                             instance_id);
> >
> > integer_obj here is elements, i.e. a pointer to elements[0] (the NAME
> > field, always ACPI_TYPE_STRING). Reading ->package.count off a string
> > object aliases ->string.length in the underlying union acpi_object, so
> > the "count" passed down is not the real package size at all.
>
> Only at this point you're actually telling what the problem is.
>
> My suggestion is to rewrite this so that you start by telling that wrong
> size (count) is read and passed on by these functions (sort of summary of
> the problem). Then explain what caused that.
>
> > For string, integer, enumeration and password attributes,
> > hp_populate_*_elements_from_package() bounds its iteration using the
> > corresponding per-type ELEM_CNT constant (STR_ELEM_CNT,
> > INT_ELEM_CNT, ENUM_ELEM_CNT and PSWD_ELEM_CNT). This relies on
> > hp_init_bios_package_attribute() rejecting packages with fewer than
> > ELEM_CNT elements before invoking the parsers.
> >
> > Relaxing that check would allow shorter packages to reach these
> > functions, making the fixed loop bounds unsafe.
>
> ??
>
> You might be doing this in some later patch but then you should say
> that an upcoming patch is going to relax this check, otherwise it comes
> out of nowhere.
>
> So I think you're trying to solve two cases here:
>
> 1) Passing wrong count.
> 2) Allowing count less than what those consts define (but this is only
> needed for some later patch?).
>
> But it's so that fixing 1 ends up also solving 2? I think the changelog
> should mostly focus on 1 and then only state in the end that it also
> prepares for an upcoming change that requires supporting 2.
>
> > hp_populate_ordered_list_elements_from_package() doesn't even use the
> > count for its main loop bound - it iterates unconditionally up to
> > ORD_ELEM_CNT:
> >
> >   for (elem = 1, eloc = 1; eloc < ORD_ELEM_CNT; elem++, eloc++)
> >
> > which relies entirely on the same coincidence.
> >
> > This is only safe as long as every caller is guaranteed to hand these
> > functions a package with at least ELEM_CNT real elements. A following
> > change relaxes that guarantee to allow shorter packages through, which
> > would turn this into a real out-of-bounds heap read of the
> > elements[] array once the real count drops below the fixed ELEM_CNT
> > loop bound.
> >
> > Fix this at the source: thread the real, already-validated
> > obj->package.count down through each *_package_data() wrapper instead
>
> "thread down" sounds odd to my (non-native) ear in this context.
>
> each *_package_data() wrapper -> *_package_data() wrappers
>
> > of letting the per-type code guess at it, and use it to also bound
>
> "guess at it" sounds odd to my ear and is now even correct given your
> explanation of the problem.
>
> > hp_populate_ordered_list_elements_from_package()'s main loop.
>
> > This is a
> > no-op for any package that already meets the existing ELEM_CNT
> > minimums, and is a prerequisite for safely accepting shorter packages.
> >
> > Fixes: a34fc329b189 ("platform/x86: hp-bioscfg: bioscfg")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Muhammad Bilal <meatuni001@xxxxxxxxx>
> > ---
> >  drivers/platform/x86/hp/hp-bioscfg/bioscfg.c               | 5 +++++
> >  drivers/platform/x86/hp/hp-bioscfg/bioscfg.h               | 5 +++++
> >  drivers/platform/x86/hp/hp-bioscfg/enum-attributes.c       | 3 ++-
> >  drivers/platform/x86/hp/hp-bioscfg/int-attributes.c        | 3 ++-
> >  drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c | 7 ++++---
> >  drivers/platform/x86/hp/hp-bioscfg/passwdobj-attributes.c  | 5 +++--
> >  drivers/platform/x86/hp/hp-bioscfg/string-attributes.c     | 3 ++-
> >  7 files changed, 23 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> > index 27fd6cd215290..768330d291da8 100644
> > --- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> > +++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> > @@ -731,26 +731,31 @@ static int hp_init_bios_package_attribute(enum hp_wmi_data_type attr_type,
> >       switch (attr_type) {
> >       case HPWMI_STRING_TYPE:
> >               ret = hp_populate_string_package_data(elements,
> > +                                                   obj->package.count,
> >                                                     instance_id,
> >                                                     attr_name_kobj);
> >               break;
> >       case HPWMI_INTEGER_TYPE:
> >               ret = hp_populate_integer_package_data(elements,
> > +                                                    obj->package.count,
> >                                                      instance_id,
> >                                                      attr_name_kobj);
> >               break;
> >       case HPWMI_ENUMERATION_TYPE:
> >               ret = hp_populate_enumeration_package_data(elements,
> > +                                                        obj->package.count,
> >                                                          instance_id,
> >                                                          attr_name_kobj);
> >               break;
> >       case HPWMI_ORDERED_LIST_TYPE:
> >               ret = hp_populate_ordered_list_package_data(elements,
> > +                                                         obj->package.count,
> >                                                           instance_id,
> >                                                           attr_name_kobj);
> >               break;
> >       case HPWMI_PASSWORD_TYPE:
> >               ret = hp_populate_password_package_data(elements,
> > +                                                     obj->package.count,
> >                                                       instance_id,
> >                                                       attr_name_kobj);
> >               break;
> > diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.h b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.h
> > index f1eec0e4ba075..416d7e7aaaae3 100644
> > --- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.h
> > +++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.h
> > @@ -401,6 +401,7 @@ int hp_populate_string_buffer_data(u8 *buffer_ptr, u32 *buffer_size,
> >  int hp_alloc_string_data(void);
> >  void hp_exit_string_attributes(void);
> >  int hp_populate_string_package_data(union acpi_object *str_obj,
> > +                                 int str_obj_count,
> >                                   int instance_id,
> >                                   struct kobject *attr_name_kobj);
> >
> > @@ -411,6 +412,7 @@ int hp_populate_integer_buffer_data(u8 *buffer_ptr, u32 *buffer_size,
> >  int hp_alloc_integer_data(void);
> >  void hp_exit_integer_attributes(void);
> >  int hp_populate_integer_package_data(union acpi_object *integer_obj,
> > +                                  int integer_obj_count,
> >                                    int instance_id,
> >                                    struct kobject *attr_name_kobj);
> >
> > @@ -421,6 +423,7 @@ int hp_populate_enumeration_buffer_data(u8 *buffer_ptr, u32 *buffer_size,
> >  int hp_alloc_enumeration_data(void);
> >  void hp_exit_enumeration_attributes(void);
> >  int hp_populate_enumeration_package_data(union acpi_object *enum_obj,
> > +                                      int enum_obj_count,
> >                                        int instance_id,
> >                                        struct kobject *attr_name_kobj);
> >
> > @@ -432,6 +435,7 @@ int hp_populate_ordered_list_buffer_data(u8 *buffer_ptr,
> >  int hp_alloc_ordered_list_data(void);
> >  void hp_exit_ordered_list_attributes(void);
> >  int hp_populate_ordered_list_package_data(union acpi_object *order_obj,
> > +                                       int order_obj_count,
> >                                         int instance_id,
> >                                         struct kobject *attr_name_kobj);
> >
> > @@ -440,6 +444,7 @@ int hp_populate_password_buffer_data(u8 *buffer_ptr, u32 *buffer_size,
> >                                    int instance_id,
> >                                    struct kobject *attr_name_kobj);
> >  int hp_populate_password_package_data(union acpi_object *password_obj,
> > +                                   int password_obj_count,
> >                                     int instance_id,
> >                                     struct kobject *attr_name_kobj);
> >  int hp_alloc_password_data(void);
> > diff --git a/drivers/platform/x86/hp/hp-bioscfg/enum-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/enum-attributes.c
> > index af4d1920d4880..3aa2c440e0528 100644
> > --- a/drivers/platform/x86/hp/hp-bioscfg/enum-attributes.c
> > +++ b/drivers/platform/x86/hp/hp-bioscfg/enum-attributes.c
> > @@ -304,6 +304,7 @@ static int hp_populate_enumeration_elements_from_package(union acpi_object *enum
> >   * @attr_name_kobj: The parent kernel object
> >   */
> >  int hp_populate_enumeration_package_data(union acpi_object *enum_obj,
> > +                                      int enum_obj_count,
> >                                        int instance_id,
> >                                        struct kobject *attr_name_kobj)
> >  {
> > @@ -312,7 +313,7 @@ int hp_populate_enumeration_package_data(union acpi_object *enum_obj,
> >       enum_data->attr_name_kobj = attr_name_kobj;
> >
> >       hp_populate_enumeration_elements_from_package(enum_obj,
> > -                                                   enum_obj->package.count,
> > +                                                   enum_obj_count,
> >                                                     instance_id);
> >       hp_update_attribute_permissions(enum_data->common.is_readonly,
> >                                       &enumeration_current_val);
> > diff --git a/drivers/platform/x86/hp/hp-bioscfg/int-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/int-attributes.c
> > index d96e160953e39..107e4cf1efb8a 100644
> > --- a/drivers/platform/x86/hp/hp-bioscfg/int-attributes.c
> > +++ b/drivers/platform/x86/hp/hp-bioscfg/int-attributes.c
> > @@ -279,6 +279,7 @@ static int hp_populate_integer_elements_from_package(union acpi_object *integer_
> >   * @attr_name_kobj: The parent kernel object
> >   */
> >  int hp_populate_integer_package_data(union acpi_object *integer_obj,
> > +                                  int integer_obj_count,
> >                                    int instance_id,
> >                                    struct kobject *attr_name_kobj)
> >  {
> > @@ -286,7 +287,7 @@ int hp_populate_integer_package_data(union acpi_object *integer_obj,
> >
> >       integer_data->attr_name_kobj = attr_name_kobj;
> >       hp_populate_integer_elements_from_package(integer_obj,
> > -                                               integer_obj->package.count,
> > +                                               integer_obj_count,
> >                                                 instance_id);
> >       hp_update_attribute_permissions(integer_data->common.is_readonly,
> >                                       &integer_current_val);
> > diff --git a/drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c
> > index f09489a085c86..a50d074125268 100644
> > --- a/drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c
> > +++ b/drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c
> > @@ -145,7 +145,7 @@ static int hp_populate_ordered_list_elements_from_package(union acpi_object *ord
> >       if (!order_obj)
> >               return -EINVAL;
> >
> > -     for (elem = 1, eloc = 1; eloc < ORD_ELEM_CNT; elem++, eloc++) {
> > +     for (elem = 1, eloc = 1; eloc < ORD_ELEM_CNT && elem < order_obj_count; elem++, eloc++) {
>
> This looks like a separate fix belonging to own patch. It allows you also
> to write more focused changelog text for each patch.
>
> >
> >               switch (order_obj[elem].type) {
> >               case ACPI_TYPE_STRING:
> > @@ -301,7 +301,8 @@ static int hp_populate_ordered_list_elements_from_package(union acpi_object *ord
> >   * @instance_id: The instance to enumerate
> >   * @attr_name_kobj: The parent kernel object
> >   */
> > -int hp_populate_ordered_list_package_data(union acpi_object *order_obj, int instance_id,
> > +int hp_populate_ordered_list_package_data(union acpi_object *order_obj, int order_obj_count,
> > +                                       int instance_id,
> >                                         struct kobject *attr_name_kobj)
> >  {
> >       struct ordered_list_data *ordered_list_data = &bioscfg_drv.ordered_list_data[instance_id];
> > @@ -309,7 +310,7 @@ int hp_populate_ordered_list_package_data(union acpi_object *order_obj, int inst
> >       ordered_list_data->attr_name_kobj = attr_name_kobj;
> >
> >       hp_populate_ordered_list_elements_from_package(order_obj,
> > -                                                    order_obj->package.count,
> > +                                                    order_obj_count,
> >                                                      instance_id);
> >       hp_update_attribute_permissions(ordered_list_data->common.is_readonly,
> >                                       &ordered_list_current_val);
> > diff --git a/drivers/platform/x86/hp/hp-bioscfg/passwdobj-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/passwdobj-attributes.c
> > index 4d79eb8056a5d..89316d90454d2 100644
> > --- a/drivers/platform/x86/hp/hp-bioscfg/passwdobj-attributes.c
> > +++ b/drivers/platform/x86/hp/hp-bioscfg/passwdobj-attributes.c
> > @@ -388,7 +388,8 @@ static int hp_populate_password_elements_from_package(union acpi_object *passwor
> >   * @instance_id: The instance to enumerate
> >   * @attr_name_kobj: The parent kernel object
> >   */
> > -int hp_populate_password_package_data(union acpi_object *password_obj, int instance_id,
> > +int hp_populate_password_package_data(union acpi_object *password_obj, int password_obj_count,
> > +                                   int instance_id,
> >                                     struct kobject *attr_name_kobj)
> >  {
> >       struct password_data *password_data = &bioscfg_drv.password_data[instance_id];
> > @@ -396,7 +397,7 @@ int hp_populate_password_package_data(union acpi_object *password_obj, int insta
> >       password_data->attr_name_kobj = attr_name_kobj;
> >
> >       hp_populate_password_elements_from_package(password_obj,
> > -                                                password_obj->package.count,
> > +                                                password_obj_count,
> >                                                  instance_id);
> >
> >       hp_friendly_user_name_update(password_data->common.path,
> > diff --git a/drivers/platform/x86/hp/hp-bioscfg/string-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/string-attributes.c
> > index fe5a9a3a4ef17..da5e81f1d188f 100644
> > --- a/drivers/platform/x86/hp/hp-bioscfg/string-attributes.c
> > +++ b/drivers/platform/x86/hp/hp-bioscfg/string-attributes.c
> > @@ -267,6 +267,7 @@ static int hp_populate_string_elements_from_package(union acpi_object *string_ob
> >   * @attr_name_kobj: The parent kernel object
> >   */
> >  int hp_populate_string_package_data(union acpi_object *string_obj,
> > +                                 int string_obj_count,
> >                                   int instance_id,
> >                                   struct kobject *attr_name_kobj)
> >  {
> > @@ -275,7 +276,7 @@ int hp_populate_string_package_data(union acpi_object *string_obj,
> >       string_data->attr_name_kobj = attr_name_kobj;
> >
> >       hp_populate_string_elements_from_package(string_obj,
> > -                                              string_obj->package.count,
> > +                                              string_obj_count,
> >                                                instance_id);
> >
> >       hp_update_attribute_permissions(string_data->common.is_readonly,
> >
>
> --
>  i.
>