[PATCH] net: openvswitch: datapath: fix data type in queue_gso_packets

[PATCH] net: openvswitch: datapath: fix data type in queue_gso_packets

[Gustavo A. R. Silva]

gso_type is being used in binary AND operations together with SKB_GSO_UDP.
The issue is that variable gso_type is of type unsigned short and
SKB_GSO_UDP expands to more than 16 bits:

SKB_GSO_UDP = 1 << 16

this makes any binary AND operation between gso_type and SKB_GSO_UDP to
be always zero, hence making some code unreachable and likely causing
undesired behavior.

Fix this by changing the data type of variable gso_type to unsigned int.

Addresses-Coverity-ID: 1462223
Fixes: 0c19f846d582 ("net: accept UFO datagrams from tuntap and packet")
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
 net/openvswitch/datapath.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 99cfafc..ef38e5a 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -308,7 +308,7 @@ static int queue_gso_packets(struct datapath *dp, struct sk_buff *skb,
 			     const struct dp_upcall_info *upcall_info,
 				 uint32_t cutlen)
 {
-	unsigned short gso_type = skb_shinfo(skb)->gso_type;
+	unsigned int gso_type = skb_shinfo(skb)->gso_type;
 	struct sw_flow_key later_key;
 	struct sk_buff *segs, *nskb;
 	int err;
-- 
2.7.4

Re: [PATCH] net: openvswitch: datapath: fix data type in queue_gso_packets

[Willem de Bruijn]

On Sat, Nov 25, 2017 at 2:14 PM, Gustavo A. R. Silva
<garsilva@embeddedor.com> wrote:
> gso_type is being used in binary AND operations together with SKB_GSO_UDP.
> The issue is that variable gso_type is of type unsigned short and
> SKB_GSO_UDP expands to more than 16 bits:
>
> SKB_GSO_UDP = 1 << 16
>
> this makes any binary AND operation between gso_type and SKB_GSO_UDP to
> be always zero, hence making some code unreachable and likely causing
> undesired behavior.
>
> Fix this by changing the data type of variable gso_type to unsigned int.
>
> Addresses-Coverity-ID: 1462223
> Fixes: 0c19f846d582 ("net: accept UFO datagrams from tuntap and packet")
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>

Acked-by: Willem de Bruijn <willemb@google.com>

Good catch, thanks!

The issue here is that I brought back SKB_GSO_UDP at the end of the
list at 1 << 16 to avoid renaming of all that used to follow, while it used
to be defined as 1 << 1.

The skb_shinfo(skb)->gso_type field itself has been expanded as of v4.12
in commit 7f564528a480 ("skbuff: Extend gso_type to unsigned int.").

A quick scan shows a few nic drivers that also still cast to unsigned
short: bnxt, atl1c and qede. Since UFO hardware offload no longer
exists, this is safe wrt this patch. And it is fine for older kernels as
no driver supported the previous entry at 1 << 16, SKB_GSO_ESP.
But it is fragile wrt follow-on offloads. Probably a net-next patch.

The only other likely issue I spotted with the longer gso_type is
in trace events. net_dev_start_xmit and net_dev_rx_verbose_template
both export as u16.

Re: [PATCH] net: openvswitch: datapath: fix data type in queue_gso_packets

[Willem de Bruijn]

On Sat, Nov 25, 2017 at 2:14 PM, Gustavo A. R. Silva
<garsilva-L1vi/lXTdts+Va1GwOuvDg@public.gmane.org> wrote:
> gso_type is being used in binary AND operations together with SKB_GSO_UDP.
> The issue is that variable gso_type is of type unsigned short and
> SKB_GSO_UDP expands to more than 16 bits:
>
> SKB_GSO_UDP = 1 << 16
>
> this makes any binary AND operation between gso_type and SKB_GSO_UDP to
> be always zero, hence making some code unreachable and likely causing
> undesired behavior.
>
> Fix this by changing the data type of variable gso_type to unsigned int.
>
> Addresses-Coverity-ID: 1462223
> Fixes: 0c19f846d582 ("net: accept UFO datagrams from tuntap and packet")
> Signed-off-by: Gustavo A. R. Silva <garsilva-L1vi/lXTdts+Va1GwOuvDg@public.gmane.org>

Acked-by: Willem de Bruijn <willemb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

Good catch, thanks!

The issue here is that I brought back SKB_GSO_UDP at the end of the
list at 1 << 16 to avoid renaming of all that used to follow, while it used
to be defined as 1 << 1.

The skb_shinfo(skb)->gso_type field itself has been expanded as of v4.12
in commit 7f564528a480 ("skbuff: Extend gso_type to unsigned int.").

A quick scan shows a few nic drivers that also still cast to unsigned
short: bnxt, atl1c and qede. Since UFO hardware offload no longer
exists, this is safe wrt this patch. And it is fine for older kernels as
no driver supported the previous entry at 1 << 16, SKB_GSO_ESP.
But it is fragile wrt follow-on offloads. Probably a net-next patch.

The only other likely issue I spotted with the longer gso_type is
in trace events. net_dev_start_xmit and net_dev_rx_verbose_template
both export as u16.

Re: [PATCH] net: openvswitch: datapath: fix data type in queue_gso_packets

[David Miller]

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: Sat, 25 Nov 2017 16:15:01 -0500

> On Sat, Nov 25, 2017 at 2:14 PM, Gustavo A. R. Silva
> <garsilva@embeddedor.com> wrote:
>> gso_type is being used in binary AND operations together with SKB_GSO_UDP.
>> The issue is that variable gso_type is of type unsigned short and
>> SKB_GSO_UDP expands to more than 16 bits:
>>
>> SKB_GSO_UDP = 1 << 16
>>
>> this makes any binary AND operation between gso_type and SKB_GSO_UDP to
>> be always zero, hence making some code unreachable and likely causing
>> undesired behavior.
>>
>> Fix this by changing the data type of variable gso_type to unsigned int.
>>
>> Addresses-Coverity-ID: 1462223
>> Fixes: 0c19f846d582 ("net: accept UFO datagrams from tuntap and packet")
>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> 
> Acked-by: Willem de Bruijn <willemb@google.com>

Applied and I'll queued this up with Willem's changes for -stable.

Thanks!

Re: [PATCH] net: openvswitch: datapath: fix data type in queue_gso_packets

[David Miller]

From: Willem de Bruijn <willemdebruijn.kernel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date: Sat, 25 Nov 2017 16:15:01 -0500

> On Sat, Nov 25, 2017 at 2:14 PM, Gustavo A. R. Silva
> <garsilva-L1vi/lXTdts+Va1GwOuvDg@public.gmane.org> wrote:
>> gso_type is being used in binary AND operations together with SKB_GSO_UDP.
>> The issue is that variable gso_type is of type unsigned short and
>> SKB_GSO_UDP expands to more than 16 bits:
>>
>> SKB_GSO_UDP = 1 << 16
>>
>> this makes any binary AND operation between gso_type and SKB_GSO_UDP to
>> be always zero, hence making some code unreachable and likely causing
>> undesired behavior.
>>
>> Fix this by changing the data type of variable gso_type to unsigned int.
>>
>> Addresses-Coverity-ID: 1462223
>> Fixes: 0c19f846d582 ("net: accept UFO datagrams from tuntap and packet")
>> Signed-off-by: Gustavo A. R. Silva <garsilva-L1vi/lXTdts+Va1GwOuvDg@public.gmane.org>
> 
> Acked-by: Willem de Bruijn <willemb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

Applied and I'll queued this up with Willem's changes for -stable.

Thanks!

Re: [PATCH] net: openvswitch: datapath: fix data type in queue_gso_packets

[Gustavo A. R. Silva]

Quoting David Miller <davem@davemloft.net>:

> From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Date: Sat, 25 Nov 2017 16:15:01 -0500
>
>> On Sat, Nov 25, 2017 at 2:14 PM, Gustavo A. R. Silva
>> <garsilva@embeddedor.com> wrote:
>>> gso_type is being used in binary AND operations together with SKB_GSO_UDP.
>>> The issue is that variable gso_type is of type unsigned short and
>>> SKB_GSO_UDP expands to more than 16 bits:
>>>
>>> SKB_GSO_UDP = 1 << 16
>>>
>>> this makes any binary AND operation between gso_type and SKB_GSO_UDP to
>>> be always zero, hence making some code unreachable and likely causing
>>> undesired behavior.
>>>
>>> Fix this by changing the data type of variable gso_type to unsigned int.
>>>
>>> Addresses-Coverity-ID: 1462223
>>> Fixes: 0c19f846d582 ("net: accept UFO datagrams from tuntap and packet")
>>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
>>
>> Acked-by: Willem de Bruijn <willemb@google.com>
>
> Applied and I'll queued this up with Willem's changes for -stable.
>
> Thanks!

Glad to help :)

Thanks
--
Gustavo A. R. Silva

Re: [PATCH] net: openvswitch: datapath: fix data type in queue_gso_packets

[Gustavo A. R. Silva]

Quoting David Miller <davem@davemloft.net>:

> From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Date: Sat, 25 Nov 2017 16:15:01 -0500
>
>> On Sat, Nov 25, 2017 at 2:14 PM, Gustavo A. R. Silva
>> <garsilva@embeddedor.com> wrote:
>>> gso_type is being used in binary AND operations together with SKB_GSO_UDP.
>>> The issue is that variable gso_type is of type unsigned short and
>>> SKB_GSO_UDP expands to more than 16 bits:
>>>
>>> SKB_GSO_UDP = 1 << 16
>>>
>>> this makes any binary AND operation between gso_type and SKB_GSO_UDP to
>>> be always zero, hence making some code unreachable and likely causing
>>> undesired behavior.
>>>
>>> Fix this by changing the data type of variable gso_type to unsigned int.
>>>
>>> Addresses-Coverity-ID: 1462223
>>> Fixes: 0c19f846d582 ("net: accept UFO datagrams from tuntap and packet")
>>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
>>
>> Acked-by: Willem de Bruijn <willemb@google.com>
>
> Applied and I'll queued this up with Willem's changes for -stable.
>
> Thanks!

Glad to help :)

Thanks