[v3] net/bonding: fix potential out of bounds read
Checks
Commit Message
Add validation to pointer constructed from the IPv4 header length
in order to prevent malformed packets from generating a potential
out of bounds memory read.
Fixes: 09150784a776 ("net/bonding: burst mode hash calculation")
Cc: stable@dpdk.org
Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
---
v2: add fixes lines
v3: fix buffer end calculation
drivers/net/bonding/rte_eth_bond_pmd.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
Comments
On 4/17/2019 3:36 PM, Radu Nicolau wrote:
> Add validation to pointer constructed from the IPv4 header length
> in order to prevent malformed packets from generating a potential
> out of bounds memory read.
>
> Fixes: 09150784a776 ("net/bonding: burst mode hash calculation")
> Cc: stable@dpdk.org
>
> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
Hi Chas,
Do you have any objection on the patch?
Functionally looks correct to me, but additional checks in datapath perhaps can
be a concern, if not I am for getting the patch.
> ---
> v2: add fixes lines
> v3: fix buffer end calculation
>
> drivers/net/bonding/rte_eth_bond_pmd.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index b0d191d..2b7f2b3 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -842,6 +842,7 @@ burst_xmit_l34_hash(struct rte_mbuf **buf, uint16_t nb_pkts,
>
> for (i = 0; i < nb_pkts; i++) {
> eth_hdr = rte_pktmbuf_mtod(buf[i], struct ether_hdr *);
> + size_t pkt_end = (size_t)eth_hdr + rte_pktmbuf_data_len(buf[i]);
> proto = eth_hdr->ether_type;
> vlan_offset = get_vlan_offset(eth_hdr, &proto);
> l3hash = 0;
> @@ -865,13 +866,17 @@ burst_xmit_l34_hash(struct rte_mbuf **buf, uint16_t nb_pkts,
> tcp_hdr = (struct tcp_hdr *)
> ((char *)ipv4_hdr +
> ip_hdr_offset);
> - l4hash = HASH_L4_PORTS(tcp_hdr);
> + if ((size_t)tcp_hdr + sizeof(*tcp_hdr)
> + < pkt_end)
> + l4hash = HASH_L4_PORTS(tcp_hdr);
> } else if (ipv4_hdr->next_proto_id ==
> IPPROTO_UDP) {
> udp_hdr = (struct udp_hdr *)
> ((char *)ipv4_hdr +
> ip_hdr_offset);
> - l4hash = HASH_L4_PORTS(udp_hdr);
> + if ((size_t)udp_hdr + sizeof(*udp_hdr)
> + < pkt_end)
> + l4hash = HASH_L4_PORTS(udp_hdr);
> }
> }
> } else if (rte_cpu_to_be_16(ETHER_TYPE_IPv6) == proto) {
>
On 4/18/19 2:41 PM, Ferruh Yigit wrote:
> On 4/17/2019 3:36 PM, Radu Nicolau wrote:
>> Add validation to pointer constructed from the IPv4 header length
>> in order to prevent malformed packets from generating a potential
>> out of bounds memory read.
>>
>> Fixes: 09150784a776 ("net/bonding: burst mode hash calculation")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
>
> Hi Chas,
>
> Do you have any objection on the patch?
> Functionally looks correct to me, but additional checks in datapath perhaps can
> be a concern, if not I am for getting the patch.
I don't think the calculation is a huge concern.
Acked-by: Chas Williams <chas3@att.com>
>
>> ---
>> v2: add fixes lines
>> v3: fix buffer end calculation
>>
>> drivers/net/bonding/rte_eth_bond_pmd.c | 9 +++++++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
>> index b0d191d..2b7f2b3 100644
>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>> @@ -842,6 +842,7 @@ burst_xmit_l34_hash(struct rte_mbuf **buf, uint16_t nb_pkts,
>>
>> for (i = 0; i < nb_pkts; i++) {
>> eth_hdr = rte_pktmbuf_mtod(buf[i], struct ether_hdr *);
>> + size_t pkt_end = (size_t)eth_hdr + rte_pktmbuf_data_len(buf[i]);
>> proto = eth_hdr->ether_type;
>> vlan_offset = get_vlan_offset(eth_hdr, &proto);
>> l3hash = 0;
>> @@ -865,13 +866,17 @@ burst_xmit_l34_hash(struct rte_mbuf **buf, uint16_t nb_pkts,
>> tcp_hdr = (struct tcp_hdr *)
>> ((char *)ipv4_hdr +
>> ip_hdr_offset);
>> - l4hash = HASH_L4_PORTS(tcp_hdr);
>> + if ((size_t)tcp_hdr + sizeof(*tcp_hdr)
>> + < pkt_end)
>> + l4hash = HASH_L4_PORTS(tcp_hdr);
>> } else if (ipv4_hdr->next_proto_id ==
>> IPPROTO_UDP) {
>> udp_hdr = (struct udp_hdr *)
>> ((char *)ipv4_hdr +
>> ip_hdr_offset);
>> - l4hash = HASH_L4_PORTS(udp_hdr);
>> + if ((size_t)udp_hdr + sizeof(*udp_hdr)
>> + < pkt_end)
>> + l4hash = HASH_L4_PORTS(udp_hdr);
>> }
>> }
>> } else if (rte_cpu_to_be_16(ETHER_TYPE_IPv6) == proto) {
>>
>
On 4/18/2019 11:57 PM, Chas Williams wrote:
>
>
> On 4/18/19 2:41 PM, Ferruh Yigit wrote:
>> On 4/17/2019 3:36 PM, Radu Nicolau wrote:
>>> Add validation to pointer constructed from the IPv4 header length
>>> in order to prevent malformed packets from generating a potential
>>> out of bounds memory read.
>>>
>>> Fixes: 09150784a776 ("net/bonding: burst mode hash calculation")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
>>
>> Hi Chas,
>>
>> Do you have any objection on the patch?
>> Functionally looks correct to me, but additional checks in datapath perhaps can
>> be a concern, if not I am for getting the patch.
>
> I don't think the calculation is a huge concern.
>
> Acked-by: Chas Williams <chas3@att.com>
Applied to dpdk-next-net/master, thanks.
@@ -842,6 +842,7 @@ burst_xmit_l34_hash(struct rte_mbuf **buf, uint16_t nb_pkts,
for (i = 0; i < nb_pkts; i++) {
eth_hdr = rte_pktmbuf_mtod(buf[i], struct ether_hdr *);
+ size_t pkt_end = (size_t)eth_hdr + rte_pktmbuf_data_len(buf[i]);
proto = eth_hdr->ether_type;
vlan_offset = get_vlan_offset(eth_hdr, &proto);
l3hash = 0;
@@ -865,13 +866,17 @@ burst_xmit_l34_hash(struct rte_mbuf **buf, uint16_t nb_pkts,
tcp_hdr = (struct tcp_hdr *)
((char *)ipv4_hdr +
ip_hdr_offset);
- l4hash = HASH_L4_PORTS(tcp_hdr);
+ if ((size_t)tcp_hdr + sizeof(*tcp_hdr)
+ < pkt_end)
+ l4hash = HASH_L4_PORTS(tcp_hdr);
} else if (ipv4_hdr->next_proto_id ==
IPPROTO_UDP) {
udp_hdr = (struct udp_hdr *)
((char *)ipv4_hdr +
ip_hdr_offset);
- l4hash = HASH_L4_PORTS(udp_hdr);
+ if ((size_t)udp_hdr + sizeof(*udp_hdr)
+ < pkt_end)
+ l4hash = HASH_L4_PORTS(udp_hdr);
}
}
} else if (rte_cpu_to_be_16(ETHER_TYPE_IPv6) == proto) {