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

[Simon Horman <horms@kernel.org>]

On Fri, May 17, 2024 at 01:17:57PM +0000, Hagar Hemdan wrote:
> 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()

Hi Hagar,

FTR, I meant to say "If ... called from xfrm_output_one()",
but I don't think that effects the direction of the conversation
at this point.

> > 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?

Thanks, I missed the important detail that calls to esp_output_tail_tcp()
are guarded by "x->encap && x->encap->encap_type == TCP_ENCAP_ESPINTCP".

Assuming that condition is always false if CONFIG_INET_ESPINTCP is not set,
then I agree with your analysis and I don't see any problems with your
patch.

It might be worth calling out in the commit message that the WARN_ON
is added because esp_output_tail_tcp() should never be called if
CONFIG_INET_ESPINTCP is not set.