[v2] flow_dissector: prevent NULL pointer dereference in __skb_flow_dissect

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

Posted by Anastasia Belova 1 year, 10 months ago

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 v2] flow_dissector: prevent NULL pointer dereference in __skb_flow_dissect

Posted by Eric Dumazet 1 year, 10 months ago

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

Posted by Simon Horman 1 year, 10 months ago

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

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

Posted by Jiri Pirko 1 year, 10 months ago

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

Posted by Denis Kirjanov 1 year, 10 months ago


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