Re: [PATCH] flow disector: check if arp_eth is null rather than arp
On Fri, 2017-01-13 at 18:25 +, Colin Ian King wrote: > On 13/01/17 18:24, Eric Dumazet wrote: > > It looks that we try very hard to add critical bugs in flow dissector. > > > > This is embarrassing really. > > > > I am questioning if the __skb_header_pointer() is correct > > > > Why using hlen - sizeof(_arp) ? > > > >arp_eth = __skb_header_pointer(skb, nhoff + sizeof(_arp), > > sizeof(_arp_eth), data, > > hlen - sizeof(_arp), > > &_arp_eth); > > > > Yep, the sizeof maybe dubious too, I overlooked that one; if somebody > can clarify that then I'll send a V2 if it needs fixing up too. I am pretty sure we should use hlen instead of (hlen - sizeof(_arp)) A V2 would be nice ;)
Re: [PATCH] flow disector: check if arp_eth is null rather than arp
On Fri, 2017-01-13 at 13:34 +, Colin King wrote: > From: Colin Ian King> > arp is being checked instead of arp_eth to see if the call to > __skb_header_pointer failed. Fix this by checking arp_eth is > null instead of arp. > > CoverityScan CID#1396428 ("Logically dead code") on 2nd > arp comparison (which should be arp_eth instead). > > Fixes: commit 55733350e5e8b70c5 ("flow disector: ARP support") > Signed-off-by: Colin Ian King > --- > net/core/flow_dissector.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c > index e3dffc7..fec48e9 100644 > --- a/net/core/flow_dissector.c > +++ b/net/core/flow_dissector.c > @@ -409,7 +409,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb, > sizeof(_arp_eth), data, > hlen - sizeof(_arp), > &_arp_eth); > - if (!arp) > + if (!arp_eth) > goto out_bad; > > if (dissector_uses_key(flow_dissector, It looks that we try very hard to add critical bugs in flow dissector. This is embarrassing really. I am questioning if the __skb_header_pointer() is correct Why using hlen - sizeof(_arp) ? arp_eth = __skb_header_pointer(skb, nhoff + sizeof(_arp), sizeof(_arp_eth), data, hlen - sizeof(_arp), &_arp_eth);
Re: [PATCH] flow disector: check if arp_eth is null rather than arp
On 13/01/17 18:24, Eric Dumazet wrote: > On Fri, 2017-01-13 at 13:34 +, Colin King wrote: >> From: Colin Ian King>> >> arp is being checked instead of arp_eth to see if the call to >> __skb_header_pointer failed. Fix this by checking arp_eth is >> null instead of arp. >> >> CoverityScan CID#1396428 ("Logically dead code") on 2nd >> arp comparison (which should be arp_eth instead). >> >> Fixes: commit 55733350e5e8b70c5 ("flow disector: ARP support") >> Signed-off-by: Colin Ian King >> --- >> net/core/flow_dissector.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c >> index e3dffc7..fec48e9 100644 >> --- a/net/core/flow_dissector.c >> +++ b/net/core/flow_dissector.c >> @@ -409,7 +409,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb, >> sizeof(_arp_eth), data, >> hlen - sizeof(_arp), >> &_arp_eth); >> -if (!arp) >> +if (!arp_eth) >> goto out_bad; >> >> if (dissector_uses_key(flow_dissector, > > It looks that we try very hard to add critical bugs in flow dissector. > > This is embarrassing really. > > I am questioning if the __skb_header_pointer() is correct > > Why using hlen - sizeof(_arp) ? > >arp_eth = __skb_header_pointer(skb, nhoff + sizeof(_arp), > sizeof(_arp_eth), data, > hlen - sizeof(_arp), > &_arp_eth); > Yep, the sizeof maybe dubious too, I overlooked that one; if somebody can clarify that then I'll send a V2 if it needs fixing up too. Colin
[PATCH] flow disector: check if arp_eth is null rather than arp
From: Colin Ian Kingarp is being checked instead of arp_eth to see if the call to __skb_header_pointer failed. Fix this by checking arp_eth is null instead of arp. CoverityScan CID#1396428 ("Logically dead code") on 2nd arp comparison (which should be arp_eth instead). Fixes: commit 55733350e5e8b70c5 ("flow disector: ARP support") Signed-off-by: Colin Ian King --- net/core/flow_dissector.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c index e3dffc7..fec48e9 100644 --- a/net/core/flow_dissector.c +++ b/net/core/flow_dissector.c @@ -409,7 +409,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb, sizeof(_arp_eth), data, hlen - sizeof(_arp), &_arp_eth); - if (!arp) + if (!arp_eth) goto out_bad; if (dissector_uses_key(flow_dissector, -- 2.10.2