| Message ID | 20240516080309.1872-1-hagarhem@amazon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: esp: cleanup esp_output_tail_tcp() in case of unsupported ESPINTCP | expand |
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. ...
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?
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.
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.
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
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(-)