[PATCH] flow_dissector: prevent NULL pointer dereference in __skb_flow_dissect

[PATCH] flow_dissector: prevent NULL pointer dereference in __skb_flow_dissect

[Anastasia Belova]

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(-)

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;
-- 
2.30.2

Re: [PATCH] flow_dissector: prevent NULL pointer dereference in __skb_flow_dissect

[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.


>
>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
>

Re: [PATCH] flow_dissector: prevent NULL pointer dereference in __skb_flow_dissect

[Eric Dumazet]

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.

Re: [PATCH] flow_dissector: prevent NULL pointer dereference in __skb_flow_dissect

[Jiri Pirko]

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.

Re: [PATCH] flow_dissector: prevent NULL pointer dereference in __skb_flow_dissect

[Anastasia Belova]

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

Re: [PATCH] flow_dissector: prevent NULL pointer dereference in __skb_flow_dissect

[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?

>
>
>> > 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
>

Re: [PATCH] flow_dissector: prevent NULL pointer dereference in __skb_flow_dissect

[Denis Kirjanov]

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
>>
> 

Re: [PATCH] flow_dissector: prevent NULL pointer dereference in __skb_flow_dissect

[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
>>

[PATCH v2] flow_dissector: prevent NULL pointer dereference in __skb_flow_dissect

[Anastasia Belova]

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: 690e36e726d0 ("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, 2 insertions(+)

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 272f09251343..68a8228ffae3 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -1139,6 +1139,8 @@ bool __skb_flow_dissect(const struct net *net,
 
 	if (dissector_uses_key(flow_dissector,
 			       FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
+		if (!skb)
+			goto out_bad;
 		struct ethhdr *eth = eth_hdr(skb);
 		struct flow_dissector_key_eth_addrs *key_eth_addrs;
 
-- 
2.30.2

Re: [PATCH] flow_dissector: prevent NULL pointer dereference in __skb_flow_dissect

[Jiri Pirko]

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
>> > 
>
>

Re: [PATCH v2] flow_dissector: prevent NULL pointer dereference in __skb_flow_dissect

[Denis Kirjanov]

On 3/21/24 15:34, Anastasia Belova 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.
> 
> Fixes: 690e36e726d0 ("net: Allow raw buffers to be passed into the flow dissector.")
> Signed-off-by: Anastasia Belova <abelova@astralinux.ru>

As request in the previous email please show the actual data flow that leads to a null pointer
dereference.
Also please read function description:
...
 * @skb: sk_buff to extract the flow from, can be NULL if the rest are specified
...

> ---
>  net/core/flow_dissector.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 272f09251343..68a8228ffae3 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -1139,6 +1139,8 @@ bool __skb_flow_dissect(const struct net *net,
>  
>  	if (dissector_uses_key(flow_dissector,
>  			       FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
> +		if (!skb)
> +			goto out_bad;
>  		struct ethhdr *eth = eth_hdr(skb);
>  		struct flow_dissector_key_eth_addrs *key_eth_addrs;
>  

Re: [PATCH v2] flow_dissector: prevent NULL pointer dereference in __skb_flow_dissect

[Jiri Pirko]

Thu, Mar 21, 2024 at 01:34:46PM 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.
>
>Fixes: 690e36e726d0 ("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, 2 insertions(+)
>
>diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>index 272f09251343..68a8228ffae3 100644
>--- a/net/core/flow_dissector.c
>+++ b/net/core/flow_dissector.c
>@@ -1139,6 +1139,8 @@ bool __skb_flow_dissect(const struct net *net,
> 
> 	if (dissector_uses_key(flow_dissector,
> 			       FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
>+		if (!skb)
>+			goto out_bad;

Please read my recent reply to v1.

pw-bot: cr


> 		struct ethhdr *eth = eth_hdr(skb);
> 		struct flow_dissector_key_eth_addrs *key_eth_addrs;
> 
>-- 
>2.30.2
>

Re: [PATCH v2] flow_dissector: prevent NULL pointer dereference in __skb_flow_dissect

[Eric Dumazet]

On Thu, Mar 21, 2024 at 1:35 PM Anastasia Belova <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.
>
> Fixes: 690e36e726d0 ("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, 2 insertions(+)
>
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 272f09251343..68a8228ffae3 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -1139,6 +1139,8 @@ bool __skb_flow_dissect(const struct net *net,
>
>         if (dissector_uses_key(flow_dissector,
>                                FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
> +               if (!skb)
> +                       goto out_bad;
>                 struct ethhdr *eth = eth_hdr(skb);
>                 struct flow_dissector_key_eth_addrs *key_eth_addrs;
>


I think you ignored my prior feedback.

In which case can we go to this point with skb == NULL ?
How come nobody complained of crashes here ?

I think we need to know if adding code here is useful or not.

You have to understand that a patch like this might need days of work
from various teams in the world,
flooded by questionable CVE.

Re: [PATCH v2] flow_dissector: prevent NULL pointer dereference in __skb_flow_dissect

[Simon Horman]

On Thu, Mar 21, 2024 at 06:16:30PM +0100, Eric Dumazet wrote:
> On Thu, Mar 21, 2024 at 1:35 PM Anastasia Belova <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.
> >
> > Fixes: 690e36e726d0 ("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, 2 insertions(+)
> >
> > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> > index 272f09251343..68a8228ffae3 100644
> > --- a/net/core/flow_dissector.c
> > +++ b/net/core/flow_dissector.c
> > @@ -1139,6 +1139,8 @@ bool __skb_flow_dissect(const struct net *net,
> >
> >         if (dissector_uses_key(flow_dissector,
> >                                FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
> > +               if (!skb)
> > +                       goto out_bad;
> >                 struct ethhdr *eth = eth_hdr(skb);
> >                 struct flow_dissector_key_eth_addrs *key_eth_addrs;
> >
> 
> 
> I think you ignored my prior feedback.
> 
> In which case can we go to this point with skb == NULL ?
> How come nobody complained of crashes here ?
> 
> I think we need to know if adding code here is useful or not.
> 
> You have to understand that a patch like this might need days of work
> from various teams in the world,
> flooded by questionable CVE.

Hi Eric and Anastasia,

I have conducted a review of the callers of __skb_flow_dissect()
that I could find in net-next and my conclusion is that, given
current usage, the code path above will not be hit with a NULL skb.

A summary of the analysis is as follows.

bond_flow_dissect:
- Analysis: skb parameter may be NULL but FLOW_DISSECTOR_KEY_ETH_ADDRS
  is not included in flow_keys_bonding_keys
- Conclusion: Code path in question is not hit for this user

skb_flow_dissect:
skb_flow_dissect_flow_keys:
fib6_rules_early_flow_dissect:
fib4_rules_early_flow_dissect:
__skb_get_hash_symmetric:
- Analysis: data parameter is NULL, which means that skb must be non-NULL
  else a crash would occur in the following code near the top of
  __skb_flow_dissect().
	if (!data) {
		data = skb->data;
- Conclusion: Calling eth_hdr(skb) is safe for these users

Assuming my analysis is correct (please check!) then
as this code is in the fast path for many users I think it is best
not to add this unnecessary check (which I assume is Eric's concern too).