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

[Hagar Hemdan <hagarhem@amazon.com>]

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@amazon.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?