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 04:57:07PM +0100, Simon Horman wrote:
> 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.

Hi Simon,

Thanks. yes, I will update the commit msg in rev2.