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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Wed, 31 Jan 2024 22:50:01 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Antony Antony <antony@...nome.org>
Cc: Antony Antony <antony.antony@...unet.com>,
	Steffen Klassert <steffen.klassert@...unet.com>,
	Herbert Xu <herbert@...dor.apana.org.au>,
	"David S. Miller" <davem@...emloft.net>, devel@...ux-ipsec.org,
	Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org
Subject: Re: [PATCH ipsec-next 1/2] xfrm: introduce forwarding of ICMP Error
 messages

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