Received-SPF: pass (google.com: domain of linux-kernel+bounces-111374-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Fri, 22 Mar 2024 11:41:15 +0000
From: Simon Horman <horms@kernel.org>
To: Eric Dumazet <edumazet@google.com>
Cc: Anastasia Belova <abelova@astralinux.ru>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>, Jiri Pirko <jiri@resnulli.us>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	lvc-project@linuxtesting.org
Subject: Re: [PATCH v2] flow_dissector: prevent NULL pointer dereference in
 __skb_flow_dissect
Message-ID: <20240322114115.GB372561@kernel.org>
References: <Zfrmv4u0tVcYGS5n@nanopsycho>
 <20240321123446.7012-1-abelova@astralinux.ru>
 <CANn89iK1SO32Zggz5fh4J=NmrVW5RjkdbxJ+-ULP8ysmKXLGvg@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CANn89iK1SO32Zggz5fh4J=NmrVW5RjkdbxJ+-ULP8ysmKXLGvg@mail.gmail.com>

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