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 for Android: free password hash cracker in your pocket

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

Message-ID: <c973a8fc-2baa-49e8-9c8f-fd63ef348f92@moroto.mountain>
Date: Wed, 31 Jan 2024 22:48:02 +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 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);".

So what we do in this case, is we just ignore the warning and if anyone
has questions about it they will look up this conversation on
lore.kernel.org to find the explanation.

No need to worry about trying to silence the checker...

regards,
dan carpenter