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] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zbqhy8U-o2uL2_us@Antony2201.local>
Date: Wed, 31 Jan 2024 20:38:51 +0100
From: Antony Antony <antony@...nome.org>
To: Dan Carpenter <dan.carpenter@...aro.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

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?

> 
>   3693                  pol = xfrm_sk_policy_lookup(sk, dir, &fl, family, if_id);
>   3694                  if (IS_ERR(pol)) {
>   3695                          XFRM_INC_STATS(net, LINUX_MIB_XFRMINPOLERROR);
>   3696                          return 0;
>   3697                  }
>   3698          }
>   3699  
>   3700          if (!pol)
>   3701                  pol = xfrm_policy_lookup(net, &fl, family, dir, if_id);
>   3702  
>   3703          if (IS_ERR(pol)) {
>   3704                  XFRM_INC_STATS(net, LINUX_MIB_XFRMINPOLERROR);
>   3705                  return 0;
>   3706          }
>   3707  
>   3708          if (!pol && dir == XFRM_POLICY_FWD)
>                             ^^^^^^^^^^^^^^^^^^^^^^
> This assumes that dir can be 2

Yes that is correct. However, this patch does not need sk != NULL at the 
same time.

>   3709                  pol = xfrm_in_fwd_icmp(skb, &fl, family, if_id);
>   3710  
>   3711          if (!pol) {
>   3712                  if (net->xfrm.policy_default[dir] == XFRM_USERPOLICY_BLOCK) {
>   3713                          XFRM_INC_STATS(net, LINUX_MIB_XFRMINNOPOLS);
>   3714                          return 0;
>   3715                  }
>   3716  
>   3717                  if (sp && secpath_has_nontransport(sp, 0, &xerr_idx)) {
>   3718                          xfrm_secpath_reject(xerr_idx, skb, &fl);
>   3719                          XFRM_INC_STATS(net, LINUX_MIB_XFRMINNOPOLS);
>   3720                          return 0;
>   3721                  }
>   3722                  return 1;
> 
> regards,
> dan carpenter
>