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

Dan Carpenter
2024-01-31T19:50:06Z

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