netdev - Re: [PATCH] net: esp: cleanup esp_output_tail_tcp() in case of unsupported ESPINTCP

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: <20240517131757.GA12613@amazon.com>
Date: Fri, 17 May 2024 13:17:57 +0000
From: Hagar Hemdan <hagarhem@...zon.com>
To: Simon Horman <horms@...nel.org>
CC: Norbert Manthey <nmanthey@...zon.de>, Steffen Klassert
	<steffen.klassert@...unet.com>, Herbert Xu <herbert@...dor.apana.org.au>,
	"David S. Miller" <davem@...emloft.net>, David Ahern <dsahern@...nel.org>,
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, "Paolo
 Abeni" <pabeni@...hat.com>, Sabrina Dubroca <sd@...asysnail.net>,
	<netdev@...r.kernel.org>, <hagarhem@...zon.de>
Subject: Re: [PATCH] net: esp: cleanup esp_output_tail_tcp() in case of
 unsupported ESPINTCP

On Fri, May 17, 2024 at 01:22:38PM +0100, Simon Horman wrote:
> On Thu, May 16, 2024 at 08:03:09AM +0000, Hagar Hemdan wrote:
> > xmit() functions should consume skb or return error codes in error
> > paths.
> > When the configuration "CONFIG_INET_ESPINTCP" is not used, the
> > implementation of the function "esp_output_tail_tcp" violates this rule.
> > The function frees the skb and returns the error code.
> > This change removes the kfree_skb from both functions, for both
> > esp4 and esp6.
> > 
> > This should not be reachable in the current code, so this change is just
> > a cleanup.
> > 
> > This bug was discovered and resolved using Coverity Static Analysis
> > Security Testing (SAST) by Synopsys, Inc.
> > 
> > Fixes: e27cca96cd68 ("xfrm: add espintcp (RFC 8229)")
> > Signed-off-by: Hagar Hemdan <hagarhem@...zon.com>
> 
> Hi Hagar,
> 
> If esp_output() may be the x->type->output callback called from esp_output()
> then I agree that this seems to be a problem as it looks like a double free
> may occur.
> 
> However, I believe that your proposed fix introduces will result in skb
> being leaked leak in the case of esp_output_done() calling
> esp_output_tail_tcp(). Perhaps a solution is for esp_output_done()
> to free the skb if esp_output_tail_tcp() fails.
> 
> I did not analyse other call-chains, but I think such analysis is needed.
> 
> ...
Hi Simon,

I see all calls to esp_output_tail_tcp() is surrounded by the condition
"x->encap && x->encap->encap_type == TCP_ENCAP_ESPINTCP" which I see
it is related to enabling of CONFIG_INET_ESPINTCP configuration 
(introduced in this commit e27cca96cd68 ("xfrm: add espintcp (RFC 8229)").

For calling of x->type->output (resolved to esp_output()) in
xfrm_output_one(), I see there is no double free here as esp_output()
calls esp_output_tail() which calls esp_output_tail_tcp() only if 
x->encap->encap_type == TCP_ENCAP_ESPINTCP which points to the first 
implementation of esp_output_tail_tcp(). This first definition 
doesn't free skb.

So my understanding is the 2nd esp_output_tail_tcp() should not be
called and this is why I called WARN_ON() as this func is unreachable.
Removing free(skb) here is just for silencing double free Coverity 
false positive.
Is there something else I miss?