diff mbox series

[v2,2/2] hmat acpi: Fix out of bounds access due to missing use of indirection

Message ID 20240307160326.31570-3-Jonathan.Cameron@huawei.com (mailing list archive)
State New, archived
Headers show
Series hw/acpi/hmat: Misc fixes | expand

Commit Message

Jonathan Cameron March 7, 2024, 4:03 p.m. UTC
With a numa set up such as

-numa nodeid=0,cpus=0 \
-numa nodeid=1,memdev=mem \
-numa nodeid=2,cpus=1

and appropriate hmat_lb entries the initiator list is correctly
computed and writen to HMAT as 0,2 but then the LB data is accessed
using the node id (here 2), landing outside the entry_list array.

Stash the reverse lookup when writing the initiator list and use
it to get the correct array index index.

Fixes: 4586a2cb83 ("hmat acpi: Build System Locality Latency and Bandwidth Information Structure(s)")
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 hw/acpi/hmat.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Michael Tokarev March 13, 2024, 6:24 p.m. UTC | #1
07.03.2024 19:03, Jonathan Cameron via wrote:
> With a numa set up such as
> 
> -numa nodeid=0,cpus=0 \
> -numa nodeid=1,memdev=mem \
> -numa nodeid=2,cpus=1
> 
> and appropriate hmat_lb entries the initiator list is correctly
> computed and writen to HMAT as 0,2 but then the LB data is accessed
> using the node id (here 2), landing outside the entry_list array.
> 
> Stash the reverse lookup when writing the initiator list and use
> it to get the correct array index index.
> 
> Fixes: 4586a2cb83 ("hmat acpi: Build System Locality Latency and Bandwidth Information Structure(s)")
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

This seems like a -stable material, is it not?

Thanks,

/mjt

> ---
>   hw/acpi/hmat.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/acpi/hmat.c b/hw/acpi/hmat.c
> index 723ae28d32..b933ae3c06 100644
> --- a/hw/acpi/hmat.c
> +++ b/hw/acpi/hmat.c
> @@ -78,6 +78,7 @@ static void build_hmat_lb(GArray *table_data, HMAT_LB_Info *hmat_lb,
>                             uint32_t *initiator_list)
>   {
>       int i, index;
> +    uint32_t initiator_to_index[MAX_NODES] = {};
>       HMAT_LB_Data *lb_data;
>       uint16_t *entry_list;
>       uint32_t base;
> @@ -121,6 +122,8 @@ static void build_hmat_lb(GArray *table_data, HMAT_LB_Info *hmat_lb,
>       /* Initiator Proximity Domain List */
>       for (i = 0; i < num_initiator; i++) {
>           build_append_int_noprefix(table_data, initiator_list[i], 4);
> +        /* Reverse mapping for array possitions */
> +        initiator_to_index[initiator_list[i]] = i;
>       }
>   
>       /* Target Proximity Domain List */
> @@ -132,7 +135,8 @@ static void build_hmat_lb(GArray *table_data, HMAT_LB_Info *hmat_lb,
>       entry_list = g_new0(uint16_t, num_initiator * num_target);
>       for (i = 0; i < hmat_lb->list->len; i++) {
>           lb_data = &g_array_index(hmat_lb->list, HMAT_LB_Data, i);
> -        index = lb_data->initiator * num_target + lb_data->target;
> +        index = initiator_to_index[lb_data->initiator] * num_target +
> +            lb_data->target;
>   
>           entry_list[index] = (uint16_t)(lb_data->data / hmat_lb->base);
>       }
Jonathan Cameron March 15, 2024, 10:25 a.m. UTC | #2
On Wed, 13 Mar 2024 21:24:06 +0300
Michael Tokarev <mjt@tls.msk.ru> wrote:

> 07.03.2024 19:03, Jonathan Cameron via wrote:
> > With a numa set up such as
> > 
> > -numa nodeid=0,cpus=0 \
> > -numa nodeid=1,memdev=mem \
> > -numa nodeid=2,cpus=1
> > 
> > and appropriate hmat_lb entries the initiator list is correctly
> > computed and writen to HMAT as 0,2 but then the LB data is accessed
> > using the node id (here 2), landing outside the entry_list array.
> > 
> > Stash the reverse lookup when writing the initiator list and use
> > it to get the correct array index index.
> > 
> > Fixes: 4586a2cb83 ("hmat acpi: Build System Locality Latency and Bandwidth Information Structure(s)")
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>  
> 
> This seems like a -stable material, is it not?

Yes. Use case is obscure, but indeed seems suitable for stable.
Thanks.

Jonathan

> 
> Thanks,
> 
> /mjt
> 
> > ---
> >   hw/acpi/hmat.c | 6 +++++-
> >   1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/acpi/hmat.c b/hw/acpi/hmat.c
> > index 723ae28d32..b933ae3c06 100644
> > --- a/hw/acpi/hmat.c
> > +++ b/hw/acpi/hmat.c
> > @@ -78,6 +78,7 @@ static void build_hmat_lb(GArray *table_data, HMAT_LB_Info *hmat_lb,
> >                             uint32_t *initiator_list)
> >   {
> >       int i, index;
> > +    uint32_t initiator_to_index[MAX_NODES] = {};
> >       HMAT_LB_Data *lb_data;
> >       uint16_t *entry_list;
> >       uint32_t base;
> > @@ -121,6 +122,8 @@ static void build_hmat_lb(GArray *table_data, HMAT_LB_Info *hmat_lb,
> >       /* Initiator Proximity Domain List */
> >       for (i = 0; i < num_initiator; i++) {
> >           build_append_int_noprefix(table_data, initiator_list[i], 4);
> > +        /* Reverse mapping for array possitions */
> > +        initiator_to_index[initiator_list[i]] = i;
> >       }
> >   
> >       /* Target Proximity Domain List */
> > @@ -132,7 +135,8 @@ static void build_hmat_lb(GArray *table_data, HMAT_LB_Info *hmat_lb,
> >       entry_list = g_new0(uint16_t, num_initiator * num_target);
> >       for (i = 0; i < hmat_lb->list->len; i++) {
> >           lb_data = &g_array_index(hmat_lb->list, HMAT_LB_Data, i);
> > -        index = lb_data->initiator * num_target + lb_data->target;
> > +        index = initiator_to_index[lb_data->initiator] * num_target +
> > +            lb_data->target;
> >   
> >           entry_list[index] = (uint16_t)(lb_data->data / hmat_lb->base);
> >       }  
>
diff mbox series

Patch

diff --git a/hw/acpi/hmat.c b/hw/acpi/hmat.c
index 723ae28d32..b933ae3c06 100644
--- a/hw/acpi/hmat.c
+++ b/hw/acpi/hmat.c
@@ -78,6 +78,7 @@  static void build_hmat_lb(GArray *table_data, HMAT_LB_Info *hmat_lb,
                           uint32_t *initiator_list)
 {
     int i, index;
+    uint32_t initiator_to_index[MAX_NODES] = {};
     HMAT_LB_Data *lb_data;
     uint16_t *entry_list;
     uint32_t base;
@@ -121,6 +122,8 @@  static void build_hmat_lb(GArray *table_data, HMAT_LB_Info *hmat_lb,
     /* Initiator Proximity Domain List */
     for (i = 0; i < num_initiator; i++) {
         build_append_int_noprefix(table_data, initiator_list[i], 4);
+        /* Reverse mapping for array possitions */
+        initiator_to_index[initiator_list[i]] = i;
     }
 
     /* Target Proximity Domain List */
@@ -132,7 +135,8 @@  static void build_hmat_lb(GArray *table_data, HMAT_LB_Info *hmat_lb,
     entry_list = g_new0(uint16_t, num_initiator * num_target);
     for (i = 0; i < hmat_lb->list->len; i++) {
         lb_data = &g_array_index(hmat_lb->list, HMAT_LB_Data, i);
-        index = lb_data->initiator * num_target + lb_data->target;
+        index = initiator_to_index[lb_data->initiator] * num_target +
+            lb_data->target;
 
         entry_list[index] = (uint16_t)(lb_data->data / hmat_lb->base);
     }