Re: [PATCH ipsec-next 1/2] xfrm: introduce forwarding of ICMP Error messages

[Dan Carpenter <dan.carpenter@linaro.org>]

On Wed, Jan 31, 2024 at 10:48:02PM +0300, Dan Carpenter wrote:
> On Wed, Jan 31, 2024 at 08:38:51PM +0100, Antony Antony wrote:
> > HI Dan,
> > 
> > Thanks for reporting the warning.
> > 
> > On Tue, Jan 30, 2024 at 01:36:28PM +0300, Dan Carpenter wrote:
> > > 
> > > Hello Antony Antony,
> > > 
> > > The patch 63b21caba17e: "xfrm: introduce forwarding of ICMP Error
> > > messages" from Jan 19, 2024 (linux-next), leads to the following
> > > Smatch static checker warning:
> > > 
> > > 	net/xfrm/xfrm_policy.c:3708 __xfrm_policy_check()
> > > 	error: testing array offset 'dir' after use.
> > 
> > > 
> > > net/xfrm/xfrm_policy.c
> > >   3689  
> > >   3690          pol = NULL;
> > >   3691          sk = sk_to_full_sk(sk);
> > >   3692          if (sk && sk->sk_policy[dir]) {
> > >                             ^^^^^^^^^^^^^^^^
> > > If dir is XFRM_POLICY_FWD (2) then it is one element beyond the end of
> > > the ->sk_policy[] array.
> > 
> > Yes, that's correct. However, for this patch, it's necessary that sk != NULL 
> > at the same time. As far as I know, there isn't any code that would call dir 
> > = XFRM_POLICY_FWD with sk != NULL. What am I missing? Did Smatch give any 
> > hints for such a code path?
> > 
> 
> I wondered if that might be the case.  The truth is that this sort of
> dependency is too compicated for any static analysis tools that
> currently exist.  Smatch tries to track the relationship between
> "dir" and "sk" as they are passed in, but it will look the relationship
> information when we re-assign sk.  "sk = sk_to_full_sk(sk);".

s/look/lose/.  I'm tired.  I should go to bed.

regards,
dan carpenter