[v1,2/2] bonding: fix PCI address comparison on non-pci ports

  The bonding PMD will iterate over all available ETH ports and for each,
compare a chunk of bytes at an offset that would correspond to the PCI
address in an rte_pci_device.

This is incorrect and unsafe. Also, the rte_device using this PCI
address is already found, no need to compare again the PCI address of
all eth devices.

Refactoring the code to fix this, the initial check to find the PCI bus
is out of scope.

Fixes: c848b518bbc7 ("net/bonding: support bifurcated driver in eal")
Cc: stable@dpdk.org

Cc: Chas Williams <chas3@att.com>
Cc: Declan Doherty <declan.doherty@intel.com>
Cc: Gowrishankar Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>

Signed-off-by: Gaetan Rivet <grive@u256.net>
---
 drivers/net/bonding/rte_eth_bond_args.c | 58 +++++++++++--------------
 1 file changed, 25 insertions(+), 33 deletions(-)
  

what scenarios may cause bugs in old ways.
Could you give an example, thanks.
  

On 26/11/20 10:24 +0800, Min Hu (Connor) wrote:
> what scenarios may cause bugs in old ways.
> Could you give an example, thanks.

Hello,

For example in the following code:

-       RTE_ETH_FOREACH_DEV(i) {
-               pci_dev = RTE_ETH_DEV_TO_PCI(&rte_eth_devices[i]);
-               eth_pci_addr = &pci_dev->addr;

All ethdev are iterated, before reading their supposed PCI addresses, and
comparing those to the one passed in arguments.

But not all ethdev will be PCI devices, so the cast is incorrect. It will do
a containerof() on a structure that it supposes contains an rte_device at the
offset 16 (two pointers accounting for the TAILQ_ENTRY()), but nothing prevents any
other bus from implementing their device with any other layout.

So the cast is wrong, but generally it will give out readable memory at least.
Then the field ((eth_dev)->device)->addr will be read and compared against the input,
with arbitrary data here.

A scenario that could cause bug would be when another bus implements a device
in such a way that it will write plausible binary rep of PCI addresses at the
same offset, and match a request from a user. The bonding PMD would take the
device over without properly checking that it is actually a PCI device.


There have been APIs introduced in the EAL to simplify the iteration of
devices, especially restricted to buses. Those APIs should be used instead.
The current implementation is inefficient and wrong. It will work in most cases
but can still trigger weird issues for users, especially in cases that bonding
PMD devs won't generally encounter (with setups where multiple buses are used
with a variety of devices).

Regards,
  

Hi,
     sorry for late reply.
     I know what you mean. But "find_port_id_by_pci_addr"
is one static type funtion. it is only used in the function
"parse_port_id". what you modified in "find_port_id_by_pci_addr"
is totally done before "find_port_id_by_pci_addr" in the function
"parse_port_id". That is, it first find pci_bus, then find
rte_pci_device, at last get port id.
So the bug you described will not happen in current version,
unless others directly use the function "find_port_id_by_pci_addr"
without considering that rte_eth_devices[] may contain non-pci devices.

So, I think the patch is OK to me.

Acked-by: Min Hu (Connor) <humin29@huawei.com>


在 2020/12/7 22:07, Gaëtan Rivet 写道:
> On 26/11/20 10:24 +0800, Min Hu (Connor) wrote:
>> what scenarios may cause bugs in old ways.
>> Could you give an example, thanks.
> 
> Hello,
> 
> For example in the following code:
> 
> -       RTE_ETH_FOREACH_DEV(i) {
> -               pci_dev = RTE_ETH_DEV_TO_PCI(&rte_eth_devices[i]);
> -               eth_pci_addr = &pci_dev->addr;
> 
> All ethdev are iterated, before reading their supposed PCI addresses, and
> comparing those to the one passed in arguments.
> 
> But not all ethdev will be PCI devices, so the cast is incorrect. It will do
> a containerof() on a structure that it supposes contains an rte_device at the
> offset 16 (two pointers accounting for the TAILQ_ENTRY()), but nothing prevents any
> other bus from implementing their device with any other layout.
> 
> So the cast is wrong, but generally it will give out readable memory at least.
> Then the field ((eth_dev)->device)->addr will be read and compared against the input,
> with arbitrary data here.
> 
> A scenario that could cause bug would be when another bus implements a device
> in such a way that it will write plausible binary rep of PCI addresses at the
> same offset, and match a request from a user. The bonding PMD would take the
> device over without properly checking that it is actually a PCI device.
> 
> 
> There have been APIs introduced in the EAL to simplify the iteration of
> devices, especially restricted to buses. Those APIs should be used instead.
> The current implementation is inefficient and wrong. It will work in most cases
> but can still trigger weird issues for users, especially in cases that bonding
> PMD devs won't generally encounter (with setups where multiple buses are used
> with a variety of devices).
> 
> Regards,
>
  

On 12/16/2020 12:14 PM, Min Hu (Connor) wrote:
> Hi,
>      sorry for late reply.
>      I know what you mean. But "find_port_id_by_pci_addr"
> is one static type funtion. it is only used in the function
> "parse_port_id". what you modified in "find_port_id_by_pci_addr"
> is totally done before "find_port_id_by_pci_addr" in the function
> "parse_port_id". That is, it first find pci_bus, then find
> rte_pci_device, at last get port id.
> So the bug you described will not happen in current version,
> unless others directly use the function "find_port_id_by_pci_addr"
> without considering that rte_eth_devices[] may contain non-pci devices.
> 
> So, I think the patch is OK to me.
> 
> Acked-by: Min Hu (Connor) <humin29@huawei.com>
> 

Applied to dpdk-next-net/main, thanks.

> 
> 在 2020/12/7 22:07, Gaëtan Rivet 写道:
>> On 26/11/20 10:24 +0800, Min Hu (Connor) wrote:
>>> what scenarios may cause bugs in old ways.
>>> Could you give an example, thanks.
>>
>> Hello,
>>
>> For example in the following code:
>>
>> -       RTE_ETH_FOREACH_DEV(i) {
>> -               pci_dev = RTE_ETH_DEV_TO_PCI(&rte_eth_devices[i]);
>> -               eth_pci_addr = &pci_dev->addr;
>>
>> All ethdev are iterated, before reading their supposed PCI addresses, and
>> comparing those to the one passed in arguments.
>>
>> But not all ethdev will be PCI devices, so the cast is incorrect. It will do
>> a containerof() on a structure that it supposes contains an rte_device at the
>> offset 16 (two pointers accounting for the TAILQ_ENTRY()), but nothing 
>> prevents any
>> other bus from implementing their device with any other layout.
>>
>> So the cast is wrong, but generally it will give out readable memory at least.
>> Then the field ((eth_dev)->device)->addr will be read and compared against the 
>> input,
>> with arbitrary data here.
>>
>> A scenario that could cause bug would be when another bus implements a device
>> in such a way that it will write plausible binary rep of PCI addresses at the
>> same offset, and match a request from a user. The bonding PMD would take the
>> device over without properly checking that it is actually a PCI device.
>>
>>
>> There have been APIs introduced in the EAL to simplify the iteration of
>> devices, especially restricted to buses. Those APIs should be used instead.
>> The current implementation is inefficient and wrong. It will work in most cases
>> but can still trigger weird issues for users, especially in cases that bonding
>> PMD devs won't generally encounter (with setups where multiple buses are used
>> with a variety of devices).
>>
>> Regards,
>>