| Message ID | eeccc1f38f905a39687e8b4afd8655faa18fffba.1719849427.git.petrm@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | mlxsw: Improvements | expand |
On 7/1/24 18:41, Petr Machata wrote: > A forgotten or buggy variable initialization can cause out-of-bounds access > to a register or other item array field. For an overflow, such access would > mangle adjacent parts of the register payload. For an underflow, due to all > variables being unsigned, the access would likely trample unrelated memory. > Since neither is correct, replace these accesses with accesses at the index > of 0, and warn about the issue. That is not correct either, but indeed better. > > Suggested-by: Ido Schimmel <idosch@nvidia.com> > Signed-off-by: Petr Machata <petrm@nvidia.com> > Reviewed-by: Ido Schimmel <idosch@nvidia.com> > --- > drivers/net/ethernet/mellanox/mlxsw/item.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/net/ethernet/mellanox/mlxsw/item.h b/drivers/net/ethernet/mellanox/mlxsw/item.h > index cfafbeb42586..9f7133735760 100644 > --- a/drivers/net/ethernet/mellanox/mlxsw/item.h > +++ b/drivers/net/ethernet/mellanox/mlxsw/item.h > @@ -218,6 +218,8 @@ __mlxsw_item_bit_array_offset(const struct mlxsw_item *item, > } > > max_index = (item->size.bytes << 3) / item->element_size - 1; > + if (WARN_ON(index > max_index)) > + index = 0; you have BUG*() calls just above those lines :( anyway, WARN_ON_ONCE(), and perhaps you need to print some additional data to finally fix this? > be_index = max_index - index; > offset = be_index * item->element_size >> 3; > in_byte_index = index % (BITS_PER_BYTE / item->element_size);
On Tue, Jul 02, 2024 at 09:08:17AM +0200, Przemek Kitszel wrote: > On 7/1/24 18:41, Petr Machata wrote: > > A forgotten or buggy variable initialization can cause out-of-bounds access > > to a register or other item array field. For an overflow, such access would > > mangle adjacent parts of the register payload. For an underflow, due to all > > variables being unsigned, the access would likely trample unrelated memory. > > Since neither is correct, replace these accesses with accesses at the index > > of 0, and warn about the issue. > > That is not correct either, but indeed better. > > > > > Suggested-by: Ido Schimmel <idosch@nvidia.com> > > Signed-off-by: Petr Machata <petrm@nvidia.com> > > Reviewed-by: Ido Schimmel <idosch@nvidia.com> > > --- > > drivers/net/ethernet/mellanox/mlxsw/item.h | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/net/ethernet/mellanox/mlxsw/item.h b/drivers/net/ethernet/mellanox/mlxsw/item.h > > index cfafbeb42586..9f7133735760 100644 > > --- a/drivers/net/ethernet/mellanox/mlxsw/item.h > > +++ b/drivers/net/ethernet/mellanox/mlxsw/item.h > > @@ -218,6 +218,8 @@ __mlxsw_item_bit_array_offset(const struct mlxsw_item *item, > > } > > max_index = (item->size.bytes << 3) / item->element_size - 1; > > + if (WARN_ON(index > max_index)) > > + index = 0; > > you have BUG*() calls just above those lines :( > anyway, WARN_ON_ONCE(), and perhaps you need to print some additional > data to finally fix this? The trace should be enough, but more info can be added: diff --git a/drivers/net/ethernet/mellanox/mlxsw/item.h b/drivers/net/ethernet/mellanox/mlxsw/item.h index 9f7133735760..a619a0736bd1 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/item.h +++ b/drivers/net/ethernet/mellanox/mlxsw/item.h @@ -218,7 +218,9 @@ __mlxsw_item_bit_array_offset(const struct mlxsw_item *item, } max_index = (item->size.bytes << 3) / item->element_size - 1; - if (WARN_ON(index > max_index)) + if (WARN_ONCE(index > max_index, + "name=%s,index=%u,max_index=%u\n", item->name, index, + max_index)) index = 0; be_index = max_index - index; offset = be_index * item->element_size >> 3; Will leave it to Petr to decide what he wants to include there. > > > be_index = max_index - index; > > offset = be_index * item->element_size >> 3; > > in_byte_index = index % (BITS_PER_BYTE / item->element_size); >
diff --git a/drivers/net/ethernet/mellanox/mlxsw/item.h b/drivers/net/ethernet/mellanox/mlxsw/item.h index cfafbeb42586..9f7133735760 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/item.h +++ b/drivers/net/ethernet/mellanox/mlxsw/item.h @@ -218,6 +218,8 @@ __mlxsw_item_bit_array_offset(const struct mlxsw_item *item, } max_index = (item->size.bytes << 3) / item->element_size - 1; + if (WARN_ON(index > max_index)) + index = 0; be_index = max_index - index; offset = be_index * item->element_size >> 3; in_byte_index = index % (BITS_PER_BYTE / item->element_size);