| Message ID | 20240320125635.1444-1-abelova@astralinux.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | flow_dissector: prevent NULL pointer dereference in __skb_flow_dissect | expand |
Wed, Mar 20, 2024 at 01:56:35PM CET, abelova@astralinux.ru wrote: >skb is an optional parameter, so it may be NULL. >Add check defore dereference in eth_hdr. > >Found by Linux Verification Center (linuxtesting.org) with SVACE. Either drop this line which provides no value, or attach a link to the actual report. > >Fixes: 67a900cc0436 ("flow_dissector: introduce support for Ethernet addresses") This looks incorrect. I believe that this is the offending commit: commit 690e36e726d00d2528bc569809048adf61550d80 Author: David S. Miller <davem@davemloft.net> Date: Sat Aug 23 12:13:41 2014 -0700 net: Allow raw buffers to be passed into the flow dissector. >Signed-off-by: Anastasia Belova <abelova@astralinux.ru> >--- > 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 272f09251343..05db3a8aa771 100644 >--- a/net/core/flow_dissector.c >+++ b/net/core/flow_dissector.c >@@ -1137,7 +1137,7 @@ bool __skb_flow_dissect(const struct net *net, > rcu_read_unlock(); > } > >- if (dissector_uses_key(flow_dissector, >+ if (skb && dissector_uses_key(flow_dissector, > FLOW_DISSECTOR_KEY_ETH_ADDRS)) { > struct ethhdr *eth = eth_hdr(skb); > struct flow_dissector_key_eth_addrs *key_eth_addrs; Looks like FLOW_DISSECT_RET_OUT_BAD should be returned in case the FLOW_DISSECTOR_KEY_ETH_ADDRS are selected and there is no skb, no? pw-bot: cr >-- >2.30.2 >
On Wed, Mar 20, 2024 at 2:38 PM Jiri Pirko <jiri@resnulli.us> wrote: > > Wed, Mar 20, 2024 at 01:56:35PM CET, abelova@astralinux.ru wrote: > >skb is an optional parameter, so it may be NULL. > >Add check defore dereference in eth_hdr. > > > >Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Either drop this line which provides no value, or attach a link to the > actual report. > > > > > >Fixes: 67a900cc0436 ("flow_dissector: introduce support for Ethernet addresses") > > This looks incorrect. I believe that this is the offending commit: > commit 690e36e726d00d2528bc569809048adf61550d80 > Author: David S. Miller <davem@davemloft.net> > Date: Sat Aug 23 12:13:41 2014 -0700 > > net: Allow raw buffers to be passed into the flow dissector. > > > > >Signed-off-by: Anastasia Belova <abelova@astralinux.ru> > >--- > > 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 272f09251343..05db3a8aa771 100644 > >--- a/net/core/flow_dissector.c > >+++ b/net/core/flow_dissector.c > >@@ -1137,7 +1137,7 @@ bool __skb_flow_dissect(const struct net *net, > > rcu_read_unlock(); > > } > > > >- if (dissector_uses_key(flow_dissector, > >+ if (skb && dissector_uses_key(flow_dissector, > > FLOW_DISSECTOR_KEY_ETH_ADDRS)) { > > struct ethhdr *eth = eth_hdr(skb); > > struct flow_dissector_key_eth_addrs *key_eth_addrs; > > Looks like FLOW_DISSECT_RET_OUT_BAD should be returned in case the > FLOW_DISSECTOR_KEY_ETH_ADDRS are selected and there is no skb, no? > It would be nice knowing in which context we could have a NULL skb and FLOW_DISSECTOR_KEY_ETH_ADDRS at the same time. It seems this fix is based on some kind of static analysis, but no real bug.
Wed, Mar 20, 2024 at 02:43:22PM CET, edumazet@google.com wrote: >On Wed, Mar 20, 2024 at 2:38 PM Jiri Pirko <jiri@resnulli.us> wrote: >> >> Wed, Mar 20, 2024 at 01:56:35PM CET, abelova@astralinux.ru wrote: >> >skb is an optional parameter, so it may be NULL. >> >Add check defore dereference in eth_hdr. >> > >> >Found by Linux Verification Center (linuxtesting.org) with SVACE. >> >> Either drop this line which provides no value, or attach a link to the >> actual report. >> >> >> > >> >Fixes: 67a900cc0436 ("flow_dissector: introduce support for Ethernet addresses") >> >> This looks incorrect. I believe that this is the offending commit: >> commit 690e36e726d00d2528bc569809048adf61550d80 >> Author: David S. Miller <davem@davemloft.net> >> Date: Sat Aug 23 12:13:41 2014 -0700 >> >> net: Allow raw buffers to be passed into the flow dissector. >> >> >> >> >Signed-off-by: Anastasia Belova <abelova@astralinux.ru> >> >--- >> > 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 272f09251343..05db3a8aa771 100644 >> >--- a/net/core/flow_dissector.c >> >+++ b/net/core/flow_dissector.c >> >@@ -1137,7 +1137,7 @@ bool __skb_flow_dissect(const struct net *net, >> > rcu_read_unlock(); >> > } >> > >> >- if (dissector_uses_key(flow_dissector, >> >+ if (skb && dissector_uses_key(flow_dissector, >> > FLOW_DISSECTOR_KEY_ETH_ADDRS)) { >> > struct ethhdr *eth = eth_hdr(skb); >> > struct flow_dissector_key_eth_addrs *key_eth_addrs; >> >> Looks like FLOW_DISSECT_RET_OUT_BAD should be returned in case the >> FLOW_DISSECTOR_KEY_ETH_ADDRS are selected and there is no skb, no? >> > >It would be nice knowing in which context we could have a NULL skb and >FLOW_DISSECTOR_KEY_ETH_ADDRS >at the same time. > >It seems this fix is based on some kind of static analysis, but no real bug. Yeah, I agree. That's the main reason I asked for the link to the report.
20/03/24 16:38, Jiri Pirko пишет: > Wed, Mar 20, 2024 at 01:56:35PM CET, abelova@astralinux.ru wrote: >> skb is an optional parameter, so it may be NULL. >> Add check defore dereference in eth_hdr. >> >> Found by Linux Verification Center (linuxtesting.org) with SVACE. > Either drop this line which provides no value, or attach a link to the > actual report. > It is an established practice for our project. You can find 700+ applied patches with similar line: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?qt=grep&q=linuxtesting.org >> Fixes: 67a900cc0436 ("flow_dissector: introduce support for Ethernet addresses") > This looks incorrect. I believe that this is the offending commit: > commit 690e36e726d00d2528bc569809048adf61550d80 > Author: David S. Miller <davem@davemloft.net> > Date: Sat Aug 23 12:13:41 2014 -0700 > > net: Allow raw buffers to be passed into the flow dissector. > Got it. > >> Signed-off-by: Anastasia Belova <abelova@astralinux.ru> >> --- >> 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 272f09251343..05db3a8aa771 100644 >> --- a/net/core/flow_dissector.c >> +++ b/net/core/flow_dissector.c >> @@ -1137,7 +1137,7 @@ bool __skb_flow_dissect(const struct net *net, >> rcu_read_unlock(); >> } >> >> - if (dissector_uses_key(flow_dissector, >> + if (skb && dissector_uses_key(flow_dissector, >> FLOW_DISSECTOR_KEY_ETH_ADDRS)) { >> struct ethhdr *eth = eth_hdr(skb); >> struct flow_dissector_key_eth_addrs *key_eth_addrs; > Looks like FLOW_DISSECT_RET_OUT_BAD should be returned in case the > FLOW_DISSECTOR_KEY_ETH_ADDRS are selected and there is no skb, no? I agree, I'll send the second version. Anastasia Belova
Thu, Mar 21, 2024 at 10:36:53AM CET, abelova@astralinux.ru wrote: > > >20/03/24 16:38, Jiri Pirko пишет: >> Wed, Mar 20, 2024 at 01:56:35PM CET, abelova@astralinux.ru wrote: >> > skb is an optional parameter, so it may be NULL. >> > Add check defore dereference in eth_hdr. >> > >> > Found by Linux Verification Center (linuxtesting.org) with SVACE. >> Either drop this line which provides no value, or attach a link to the >> actual report. >> > >It is an established practice for our project. You can find 700+ applied >patches with similar line: >https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?qt=grep&q=linuxtesting.org Okay. So would it be possible to attach a link to the actual report? > > >> > Fixes: 67a900cc0436 ("flow_dissector: introduce support for Ethernet addresses") >> This looks incorrect. I believe that this is the offending commit: >> commit 690e36e726d00d2528bc569809048adf61550d80 >> Author: David S. Miller <davem@davemloft.net> >> Date: Sat Aug 23 12:13:41 2014 -0700 >> >> net: Allow raw buffers to be passed into the flow dissector. >> > >Got it. > >> >> > Signed-off-by: Anastasia Belova <abelova@astralinux.ru> >> > --- >> > 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 272f09251343..05db3a8aa771 100644 >> > --- a/net/core/flow_dissector.c >> > +++ b/net/core/flow_dissector.c >> > @@ -1137,7 +1137,7 @@ bool __skb_flow_dissect(const struct net *net, >> > rcu_read_unlock(); >> > } >> > >> > - if (dissector_uses_key(flow_dissector, >> > + if (skb && dissector_uses_key(flow_dissector, >> > FLOW_DISSECTOR_KEY_ETH_ADDRS)) { >> > struct ethhdr *eth = eth_hdr(skb); >> > struct flow_dissector_key_eth_addrs *key_eth_addrs; >> Looks like FLOW_DISSECT_RET_OUT_BAD should be returned in case the >> FLOW_DISSECTOR_KEY_ETH_ADDRS are selected and there is no skb, no? > >I agree, I'll send the second version. > >Anastasia Belova >
On 3/21/24 13:57, Jiri Pirko wrote: > Thu, Mar 21, 2024 at 10:36:53AM CET, abelova@astralinux.ru wrote: >> >> >> 20/03/24 16:38, Jiri Pirko пишет: >>> Wed, Mar 20, 2024 at 01:56:35PM CET, abelova@astralinux.ru wrote: >>>> skb is an optional parameter, so it may be NULL. >>>> Add check defore dereference in eth_hdr. >>>> >>>> Found by Linux Verification Center (linuxtesting.org) with SVACE. >>> Either drop this line which provides no value, or attach a link to the >>> actual report. >>> >> >> It is an established practice for our project. You can find 700+ applied >> patches with similar line: >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?qt=grep&q=linuxtesting.org > > Okay. So would it be possible to attach a link to the actual report? > >> >> >>>> Fixes: 67a900cc0436 ("flow_dissector: introduce support for Ethernet addresses") >>> This looks incorrect. I believe that this is the offending commit: >>> commit 690e36e726d00d2528bc569809048adf61550d80 >>> Author: David S. Miller <davem@davemloft.net> >>> Date: Sat Aug 23 12:13:41 2014 -0700 >>> >>> net: Allow raw buffers to be passed into the flow dissector. >>> >> >> Got it. Looks like it's a static checker, there is no actual bug report or kernel oops/crash >> >>> >>>> Signed-off-by: Anastasia Belova <abelova@astralinux.ru> >>>> --- >>>> 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 272f09251343..05db3a8aa771 100644 >>>> --- a/net/core/flow_dissector.c >>>> +++ b/net/core/flow_dissector.c >>>> @@ -1137,7 +1137,7 @@ bool __skb_flow_dissect(const struct net *net, >>>> rcu_read_unlock(); >>>> } >>>> >>>> - if (dissector_uses_key(flow_dissector, >>>> + if (skb && dissector_uses_key(flow_dissector, >>>> FLOW_DISSECTOR_KEY_ETH_ADDRS)) { >>>> struct ethhdr *eth = eth_hdr(skb); >>>> struct flow_dissector_key_eth_addrs *key_eth_addrs; >>> Looks like FLOW_DISSECT_RET_OUT_BAD should be returned in case the >>> FLOW_DISSECTOR_KEY_ETH_ADDRS are selected and there is no skb, no? >> >> I agree, I'll send the second version. >> >> Anastasia Belova >> >
21/03/24 13:57, Jiri Pirko пишет: > Thu, Mar 21, 2024 at 10:36:53AM CET, abelova@astralinux.ru wrote: >> >> 20/03/24 16:38, Jiri Pirko пишет: >>> Wed, Mar 20, 2024 at 01:56:35PM CET, abelova@astralinux.ru wrote: >>>> skb is an optional parameter, so it may be NULL. >>>> Add check defore dereference in eth_hdr. >>>> >>>> Found by Linux Verification Center (linuxtesting.org) with SVACE. >>> Either drop this line which provides no value, or attach a link to the >>> actual report. >>> >> It is an established practice for our project. You can find 700+ applied >> patches with similar line: >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?qt=grep&q=linuxtesting.org > Okay. So would it be possible to attach a link to the actual report? Unfortunately no as far as results of the SVACE static analysis tool are not available publicly at the moment. Also I agree that this is quite a minor fix, but I still insist that it would be better to add a check. > >> >>>> Fixes: 67a900cc0436 ("flow_dissector: introduce support for Ethernet addresses") >>> This looks incorrect. I believe that this is the offending commit: >>> commit 690e36e726d00d2528bc569809048adf61550d80 >>> Author: David S. Miller <davem@davemloft.net> >>> Date: Sat Aug 23 12:13:41 2014 -0700 >>> >>> net: Allow raw buffers to be passed into the flow dissector. >>> >> Got it. >> >>>> Signed-off-by: Anastasia Belova <abelova@astralinux.ru> >>>> --- >>>> 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 272f09251343..05db3a8aa771 100644 >>>> --- a/net/core/flow_dissector.c >>>> +++ b/net/core/flow_dissector.c >>>> @@ -1137,7 +1137,7 @@ bool __skb_flow_dissect(const struct net *net, >>>> rcu_read_unlock(); >>>> } >>>> >>>> - if (dissector_uses_key(flow_dissector, >>>> + if (skb && dissector_uses_key(flow_dissector, >>>> FLOW_DISSECTOR_KEY_ETH_ADDRS)) { >>>> struct ethhdr *eth = eth_hdr(skb); >>>> struct flow_dissector_key_eth_addrs *key_eth_addrs; >>> Looks like FLOW_DISSECT_RET_OUT_BAD should be returned in case the >>> FLOW_DISSECTOR_KEY_ETH_ADDRS are selected and there is no skb, no? >> I agree, I'll send the second version. >> >> Anastasia Belova >>
Thu, Mar 21, 2024 at 01:04:22PM CET, abelova@astralinux.ru wrote: > > >21/03/24 13:57, Jiri Pirko пишет: >> Thu, Mar 21, 2024 at 10:36:53AM CET, abelova@astralinux.ru wrote: >> > >> > 20/03/24 16:38, Jiri Pirko пишет: >> > > Wed, Mar 20, 2024 at 01:56:35PM CET, abelova@astralinux.ru wrote: >> > > > skb is an optional parameter, so it may be NULL. >> > > > Add check defore dereference in eth_hdr. >> > > > >> > > > Found by Linux Verification Center (linuxtesting.org) with SVACE. >> > > Either drop this line which provides no value, or attach a link to the >> > > actual report. >> > > >> > It is an established practice for our project. You can find 700+ applied >> > patches with similar line: >> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?qt=grep&q=linuxtesting.org >> Okay. So would it be possible to attach a link to the actual report? > >Unfortunately no as far as results of the SVACE static analysis tool are >not available publicly at the moment. So, don't mention it, has no value what so ever. No place for advertisements like this. > > >Also I agree that this is quite a minor fix, but I still insist >that it would be better to add a check. It is not possible (prove us wrong) to hit this bug in real world. No point to fix nobug. > >> >> > >> > > > Fixes: 67a900cc0436 ("flow_dissector: introduce support for Ethernet addresses") >> > > This looks incorrect. I believe that this is the offending commit: >> > > commit 690e36e726d00d2528bc569809048adf61550d80 >> > > Author: David S. Miller <davem@davemloft.net> >> > > Date: Sat Aug 23 12:13:41 2014 -0700 >> > > >> > > net: Allow raw buffers to be passed into the flow dissector. >> > > >> > Got it. >> > >> > > > Signed-off-by: Anastasia Belova <abelova@astralinux.ru> >> > > > --- >> > > > 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 272f09251343..05db3a8aa771 100644 >> > > > --- a/net/core/flow_dissector.c >> > > > +++ b/net/core/flow_dissector.c >> > > > @@ -1137,7 +1137,7 @@ bool __skb_flow_dissect(const struct net *net, >> > > > rcu_read_unlock(); >> > > > } >> > > > >> > > > - if (dissector_uses_key(flow_dissector, >> > > > + if (skb && dissector_uses_key(flow_dissector, >> > > > FLOW_DISSECTOR_KEY_ETH_ADDRS)) { >> > > > struct ethhdr *eth = eth_hdr(skb); >> > > > struct flow_dissector_key_eth_addrs *key_eth_addrs; >> > > Looks like FLOW_DISSECT_RET_OUT_BAD should be returned in case the >> > > FLOW_DISSECTOR_KEY_ETH_ADDRS are selected and there is no skb, no? >> > I agree, I'll send the second version. >> > >> > Anastasia Belova >> > > >
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c index 272f09251343..05db3a8aa771 100644 --- a/net/core/flow_dissector.c +++ b/net/core/flow_dissector.c @@ -1137,7 +1137,7 @@ bool __skb_flow_dissect(const struct net *net, rcu_read_unlock(); } - if (dissector_uses_key(flow_dissector, + if (skb && dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ETH_ADDRS)) { struct ethhdr *eth = eth_hdr(skb); struct flow_dissector_key_eth_addrs *key_eth_addrs;
skb is an optional parameter, so it may be NULL. Add check defore dereference in eth_hdr. Found by Linux Verification Center (linuxtesting.org) with SVACE. Fixes: 67a900cc0436 ("flow_dissector: introduce support for Ethernet addresses") Signed-off-by: Anastasia Belova <abelova@astralinux.ru> --- net/core/flow_dissector.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)