| Message ID | 20240321123446.7012-1-abelova@astralinux.ru (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] flow_dissector: prevent NULL pointer dereference in __skb_flow_dissect | expand |
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; >
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 >
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.
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).
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;
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(+)