netdev - Re: [devel-ipsec] [PATCH v2 ipsec-next 2/2] xfrm: fix source address in icmp error generation from IPsec gateway

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: <ZWKABgdw2lImWXrZ@Antony2201.local>
Date: Sun, 26 Nov 2023 00:15:18 +0100
From: Antony Antony <antony@...nome.org>
To: Steffen Klassert <steffen.klassert@...unet.com>
Cc: Antony Antony <antony.antony@...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: [devel-ipsec] [PATCH v2 ipsec-next 2/2] xfrm: fix source address
 in icmp error generation from IPsec gateway

On Fri, Nov 17, 2023 at 10:21:37AM +0100, Steffen Klassert via Devel wrote:
> On Fri, Oct 27, 2023 at 10:16:52AM +0200, Antony Antony wrote:
> > 
> > diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
> > index e63a3bf99617..bec234637122 100644
> > --- a/net/ipv4/icmp.c
> > +++ b/net/ipv4/icmp.c
> > @@ -555,7 +555,6 @@ static struct rtable *icmp_route_lookup(struct net *net,
> >  					    XFRM_LOOKUP_ICMP);
> >  	if (!IS_ERR(rt2)) {
> >  		dst_release(&rt->dst);
> > -		memcpy(fl4, &fl4_dec, sizeof(*fl4));
> 
> This is not really IPsec code. The change needs either an
> Ack of one of the netdev Maintainers, or it has to go

I understand your concern. I chose to submit the change to ipsec-next as it 
is directly related to the outcome of a successful xfrm_lookup().

> through the nedev tree. Also, please consider this as
> a fix.

It is a fix:) I considered including a 'Fixes:' tag initially but ultimately 
decided against it.  My hesitation stemmed from the concern that if this fix 
were backported, it could inadvertently trigger regressions in someone’s 
test suite. This might lead to requests for a revert through the ipsec tree, 
which I am keen to avoid.

However, I do concur that this submission qualifies as a fix. Is there a way
to include the 'Fixes:' tag while also advising against backporting it to 
reduce the risk of potential regressions?

I will add the 'Fixes:' tag to the new version. When it comes to backport I 
will recomend not to backport this fix. Please keep an eye out for those 
messages. This could get backported to all curently maintained releases!

The key reason for pairing this update with my other patch ("xfrm: introduce 
forwarding of ICMP Error messages") is to proactively address any potential 
claims of a regression. Without this new patch, it's  conceivable that the 
changes could be misinterpreted as causing a regression, especially 
considering that the commit this patch addresses is 12 years old! By 
submitting them together, it should help clarify that these changes are, in 
fact, rectifying long-standing issues rather than introducing new ones.

I believe applying two patches together will provide a clearer context for 
both the changes and help streamline their acceptance and integration.

thanks,
-antony