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

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

[Hagar Hemdan]

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>
---
 net/ipv4/esp4.c | 3 +--
 net/ipv6/esp6.c | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
index d33d12421814..e73de3abe37c 100644
--- a/net/ipv4/esp4.c
+++ b/net/ipv4/esp4.c
@@ -238,8 +238,7 @@ static int esp_output_tail_tcp(struct xfrm_state *x, struct sk_buff *skb)
 #else
 static int esp_output_tail_tcp(struct xfrm_state *x, struct sk_buff *skb)
 {
-	kfree_skb(skb);
-
+	WARN_ON(1);
 	return -EOPNOTSUPP;
 }
 #endif
diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
index 7371886d4f9f..600402e54ccd 100644
--- a/net/ipv6/esp6.c
+++ b/net/ipv6/esp6.c
@@ -255,8 +255,7 @@ static int esp_output_tail_tcp(struct xfrm_state *x, struct sk_buff *skb)
 #else
 static int esp_output_tail_tcp(struct xfrm_state *x, struct sk_buff *skb)
 {
-	kfree_skb(skb);
-
+	WARN_ON(1);
 	return -EOPNOTSUPP;
 }
 #endif
-- 
2.40.1

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

[Simon Horman]

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.

...

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

[Hagar Hemdan]

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?

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

[Simon Horman]

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.

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

[Hagar Hemdan]

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.