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

[Simon Horman <horms@kernel.org>]

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