diff mbox series

[4/4] net/netvsc: check for overflow on packet info from host

Message ID 1597113194-90208-4-git-send-email-longli@linuxonhyperv.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series [1/4] net/netvsc: move rxbuf_info from per-device to per-queue |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues
ci/travis-robot success Travis build: passed

Commit Message

Long Li Aug. 11, 2020, 2:33 a.m. UTC 
  From: Stephen Hemminger <stephen@networkplumber.org>

The data from the host is trusted but checked by the driver.
One check that is missing is that the packet offset and length
might cause wraparound.

Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Long Li <longli@microsoft.com>
---
 drivers/net/netvsc/hn_rxtx.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Stephen Hemminger Aug. 11, 2020, 3:42 p.m. UTC | #1 
On Mon, 10 Aug 2020 19:33:14 -0700
longli@linuxonhyperv.com wrote:

> From: Stephen Hemminger <stephen@networkplumber.org>
> 
> The data from the host is trusted but checked by the driver.
> One check that is missing is that the packet offset and length
> might cause wraparound.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Long Li <longli@microsoft.com>

Reported-by: Nan Chen <whutchennan@gmail.com>
Luca Boccassi Oct. 27, 2020, 5:10 p.m. UTC | #2 
On Mon, 2020-08-10 at 19:33 -0700, longli@linuxonhyperv.com wrote:
> From: Stephen Hemminger <stephen@networkplumber.org>
> 
> The data from the host is trusted but checked by the driver.
> One check that is missing is that the packet offset and length
> might cause wraparound.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Long Li <longli@microsoft.com>
> ---
>  drivers/net/netvsc/hn_rxtx.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/netvsc/hn_rxtx.c b/drivers/net/netvsc/hn_rxtx.c
> index a388ff258..d8d3f07f5 100644
> --- a/drivers/net/netvsc/hn_rxtx.c
> +++ b/drivers/net/netvsc/hn_rxtx.c
> @@ -666,7 +666,8 @@ static void hn_rndis_rx_data(struct hn_rx_queue *rxq,
>  			     struct hn_rx_bufinfo *rxb,
>  			     void *data, uint32_t dlen)
>  {
> -	unsigned int data_off, data_len, pktinfo_off, pktinfo_len;
> +	unsigned int data_off, data_len, total_len;
> +	unsigned int pktinfo_off, pktinfo_len;
>  	const struct rndis_packet_msg *pkt = data;
>  	struct hn_rxinfo info = {
>  		.vlan_info = HN_NDIS_VLAN_INFO_INVALID,
> @@ -711,7 +712,8 @@ static void hn_rndis_rx_data(struct hn_rx_queue *rxq,
>  			goto error;
>  	}
>  
> -	if (unlikely(data_off + data_len > pkt->len))
> +	if (__builtin_add_overflow(data_off, data_len, &total_len) ||
> +	    total_len > pkt->len)
>  		goto error;
>  
>  	if (unlikely(data_len < RTE_ETHER_HDR_LEN))

This patch breaks the build with GCC < 5 (CentOS 7, RHEL 7, SLE 12) as
__builtin_add_overflow is not available. Could you please send a follow
up to fix it?
Ferruh Yigit Oct. 27, 2020, 11:07 p.m. UTC | #3 
On 10/27/2020 5:10 PM, Luca Boccassi wrote:
> On Mon, 2020-08-10 at 19:33 -0700, longli@linuxonhyperv.com wrote:
>> From: Stephen Hemminger <stephen@networkplumber.org>
>>
>> The data from the host is trusted but checked by the driver.
>> One check that is missing is that the packet offset and length
>> might cause wraparound.
>>
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>> Signed-off-by: Long Li <longli@microsoft.com>
>> ---
>>   drivers/net/netvsc/hn_rxtx.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/netvsc/hn_rxtx.c b/drivers/net/netvsc/hn_rxtx.c
>> index a388ff258..d8d3f07f5 100644
>> --- a/drivers/net/netvsc/hn_rxtx.c
>> +++ b/drivers/net/netvsc/hn_rxtx.c
>> @@ -666,7 +666,8 @@ static void hn_rndis_rx_data(struct hn_rx_queue *rxq,
>>   			     struct hn_rx_bufinfo *rxb,
>>   			     void *data, uint32_t dlen)
>>   {
>> -	unsigned int data_off, data_len, pktinfo_off, pktinfo_len;
>> +	unsigned int data_off, data_len, total_len;
>> +	unsigned int pktinfo_off, pktinfo_len;
>>   	const struct rndis_packet_msg *pkt = data;
>>   	struct hn_rxinfo info = {
>>   		.vlan_info = HN_NDIS_VLAN_INFO_INVALID,
>> @@ -711,7 +712,8 @@ static void hn_rndis_rx_data(struct hn_rx_queue *rxq,
>>   			goto error;
>>   	}
>>   
>> -	if (unlikely(data_off + data_len > pkt->len))
>> +	if (__builtin_add_overflow(data_off, data_len, &total_len) ||
>> +	    total_len > pkt->len)
>>   		goto error;
>>   
>>   	if (unlikely(data_len < RTE_ETHER_HDR_LEN))
> 
> This patch breaks the build with GCC < 5 (CentOS 7, RHEL 7, SLE 12) as
> __builtin_add_overflow is not available. Could you please send a follow
> up to fix it?
> 

It should be already fixed in the repo:
https://git.dpdk.org/dpdk/commit/?id=d73543b5f46d

Are you getting the build error with 20.11-rc1?
Luca Boccassi Oct. 28, 2020, 11:08 a.m. UTC | #4 
On Tue, 2020-10-27 at 23:07 +0000, Ferruh Yigit wrote:
> On 10/27/2020 5:10 PM, Luca Boccassi wrote:
> > On Mon, 2020-08-10 at 19:33 -0700, longli@linuxonhyperv.com wrote:
> > > From: Stephen Hemminger <stephen@networkplumber.org>
> > > 
> > > The data from the host is trusted but checked by the driver.
> > > One check that is missing is that the packet offset and length
> > > might cause wraparound.
> > > 
> > > Cc: stable@dpdk.org
> > > 
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > > Signed-off-by: Long Li <longli@microsoft.com>
> > > ---
> > >   drivers/net/netvsc/hn_rxtx.c | 6 ++++--
> > >   1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/net/netvsc/hn_rxtx.c b/drivers/net/netvsc/hn_rxtx.c
> > > index a388ff258..d8d3f07f5 100644
> > > --- a/drivers/net/netvsc/hn_rxtx.c
> > > +++ b/drivers/net/netvsc/hn_rxtx.c
> > > @@ -666,7 +666,8 @@ static void hn_rndis_rx_data(struct hn_rx_queue *rxq,
> > >   			     struct hn_rx_bufinfo *rxb,
> > >   			     void *data, uint32_t dlen)
> > >   {
> > > -	unsigned int data_off, data_len, pktinfo_off, pktinfo_len;
> > > +	unsigned int data_off, data_len, total_len;
> > > +	unsigned int pktinfo_off, pktinfo_len;
> > >   	const struct rndis_packet_msg *pkt = data;
> > >   	struct hn_rxinfo info = {
> > >   		.vlan_info = HN_NDIS_VLAN_INFO_INVALID,
> > > @@ -711,7 +712,8 @@ static void hn_rndis_rx_data(struct hn_rx_queue *rxq,
> > >   			goto error;
> > >   	}
> > >   
> > > -	if (unlikely(data_off + data_len > pkt->len))
> > > +	if (__builtin_add_overflow(data_off, data_len, &total_len) ||
> > > +	    total_len > pkt->len)
> > >   		goto error;
> > >   
> > >   	if (unlikely(data_len < RTE_ETHER_HDR_LEN))
> > 
> > This patch breaks the build with GCC < 5 (CentOS 7, RHEL 7, SLE 12) as
> > __builtin_add_overflow is not available. Could you please send a follow
> > up to fix it?
> > 
> 
> It should be already fixed in the repo:
> https://git.dpdk.org/dpdk/commit/?id=d73543b5f46d
> 
> Are you getting the build error with 20.11-rc1?

No, with the backport. The original patch was marked for stable, but
the fixup was not. I'll pick it up.
Ferruh Yigit Oct. 29, 2020, 8:43 a.m. UTC | #5 
On 10/28/2020 11:08 AM, Luca Boccassi wrote:
> On Tue, 2020-10-27 at 23:07 +0000, Ferruh Yigit wrote:
>> On 10/27/2020 5:10 PM, Luca Boccassi wrote:
>>> On Mon, 2020-08-10 at 19:33 -0700, longli@linuxonhyperv.com wrote:
>>>> From: Stephen Hemminger <stephen@networkplumber.org>
>>>>
>>>> The data from the host is trusted but checked by the driver.
>>>> One check that is missing is that the packet offset and length
>>>> might cause wraparound.
>>>>
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>>> Signed-off-by: Long Li <longli@microsoft.com>
>>>> ---
>>>>    drivers/net/netvsc/hn_rxtx.c | 6 ++++--
>>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/netvsc/hn_rxtx.c b/drivers/net/netvsc/hn_rxtx.c
>>>> index a388ff258..d8d3f07f5 100644
>>>> --- a/drivers/net/netvsc/hn_rxtx.c
>>>> +++ b/drivers/net/netvsc/hn_rxtx.c
>>>> @@ -666,7 +666,8 @@ static void hn_rndis_rx_data(struct hn_rx_queue *rxq,
>>>>    			     struct hn_rx_bufinfo *rxb,
>>>>    			     void *data, uint32_t dlen)
>>>>    {
>>>> -	unsigned int data_off, data_len, pktinfo_off, pktinfo_len;
>>>> +	unsigned int data_off, data_len, total_len;
>>>> +	unsigned int pktinfo_off, pktinfo_len;
>>>>    	const struct rndis_packet_msg *pkt = data;
>>>>    	struct hn_rxinfo info = {
>>>>    		.vlan_info = HN_NDIS_VLAN_INFO_INVALID,
>>>> @@ -711,7 +712,8 @@ static void hn_rndis_rx_data(struct hn_rx_queue *rxq,
>>>>    			goto error;
>>>>    	}
>>>>    
>>>> -	if (unlikely(data_off + data_len > pkt->len))
>>>> +	if (__builtin_add_overflow(data_off, data_len, &total_len) ||
>>>> +	    total_len > pkt->len)
>>>>    		goto error;
>>>>    
>>>>    	if (unlikely(data_len < RTE_ETHER_HDR_LEN))
>>>
>>> This patch breaks the build with GCC < 5 (CentOS 7, RHEL 7, SLE 12) as
>>> __builtin_add_overflow is not available. Could you please send a follow
>>> up to fix it?
>>>
>>
>> It should be already fixed in the repo:
>> https://git.dpdk.org/dpdk/commit/?id=d73543b5f46d
>>
>> Are you getting the build error with 20.11-rc1?
> 
> No, with the backport. The original patch was marked for stable, but
> the fixup was not.

Yes, it should be also marked for stable, seems missed.

> I'll pick it up.
> 

Thanks.
diff mbox series

Patch

diff --git a/drivers/net/netvsc/hn_rxtx.c b/drivers/net/netvsc/hn_rxtx.c
index a388ff258..d8d3f07f5 100644
--- a/drivers/net/netvsc/hn_rxtx.c
+++ b/drivers/net/netvsc/hn_rxtx.c
@@ -666,7 +666,8 @@  static void hn_rndis_rx_data(struct hn_rx_queue *rxq,
 			     struct hn_rx_bufinfo *rxb,
 			     void *data, uint32_t dlen)
 {
-	unsigned int data_off, data_len, pktinfo_off, pktinfo_len;
+	unsigned int data_off, data_len, total_len;
+	unsigned int pktinfo_off, pktinfo_len;
 	const struct rndis_packet_msg *pkt = data;
 	struct hn_rxinfo info = {
 		.vlan_info = HN_NDIS_VLAN_INFO_INVALID,
@@ -711,7 +712,8 @@  static void hn_rndis_rx_data(struct hn_rx_queue *rxq,
 			goto error;
 	}
 
-	if (unlikely(data_off + data_len > pkt->len))
+	if (__builtin_add_overflow(data_off, data_len, &total_len) ||
+	    total_len > pkt->len)
 		goto error;
 
 	if (unlikely(data_len < RTE_ETHER_HDR_LEN))