[PATCH net 0/2] net: openvswitch: fix skb leak on flow key update failure

[PATCH net 0/2] net: openvswitch: fix skb leak on flow key update failure

[Ilya Maximets]

Fixes for two issues reported by Sashiko while reviewing other patches.
The root cause is the same, fixes and the Fixes are slightly different,
so two separate patches.

Ilya Maximets (2):
  net: openvswitch: fix skb leak on flow key update failure during
    recirculation
  net: openvswitch: fix skb leak on flow key update failure during ct

 net/openvswitch/actions.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

-- 
2.55.0

[PATCH net 1/2] net: openvswitch: fix skb leak on flow key update failure during recirculation

[Ilya Maximets]

do_execute_actions() returns right away when execute_recirc() fails on
the last action as it assumes this function always takes ownership of
the skb when 'last' is true.  But when the flow key update fails, the
function doesn't free the skb and it ends up leaked.

This is a very unlikely scenario as it requires the packet to become
unparseable by applying a set of actions on a previously parseable skb,
but should be fixed nevertheless.

Reported by Sashiko.

Fixes: 971427f353f3 ("openvswitch: Add recirc and hash action.")
Cc: stable@vger.kernel.org
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 net/openvswitch/actions.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 513fca6a8e8a..5653b6642e10 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -1108,6 +1108,10 @@ static int execute_masked_set_action(struct sk_buff *skb,
 	return err;
 }
 
+/* When 'last' is true, recirc() should always consume the 'skb'.
+ * Otherwise, recirc() should keep 'skb' intact regardless what
+ * actions are executed on recirculation.
+ */
 static int execute_recirc(struct datapath *dp, struct sk_buff *skb,
 			  struct sw_flow_key *key,
 			  const struct nlattr *a, bool last)
@@ -1118,8 +1122,12 @@ static int execute_recirc(struct datapath *dp, struct sk_buff *skb,
 		int err;
 
 		err = ovs_flow_key_update(skb, key);
-		if (err)
+		if (err) {
+			if (last)
+				ovs_kfree_skb_reason(skb,
+						     OVS_DROP_ACTION_ERROR);
 			return err;
+		}
 	}
 	BUG_ON(!is_flow_key_valid(key));
 
-- 
2.55.0

Re: [net,1/2] net: openvswitch: fix skb leak on flow key update failure during recirculation

[Aaron Conole]

Ilya Maximets <i.maximets@ovn.org> writes:

> do_execute_actions() returns right away when execute_recirc() fails on
> the last action as it assumes this function always takes ownership of
> the skb when 'last' is true.  But when the flow key update fails, the
> function doesn't free the skb and it ends up leaked.
> 
> This is a very unlikely scenario as it requires the packet to become
> unparseable by applying a set of actions on a previously parseable skb,
> but should be fixed nevertheless.
> 
> Reported by Sashiko.
> 
> Fixes: 971427f353f3 ("openvswitch: Add recirc and hash action.")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---
>  net/openvswitch/actions.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)

Reviewed-by: Aaron Conole <aconole@redhat.com>

[PATCH net 2/2] net: openvswitch: fix skb leak on flow key update failure during ct

[Ilya Maximets]

ovs_ct_execute() always steals or frees the skb on failure while
ovs_flow_key_update() does not.  So, if it fails and we return right
away, the skb ends up leaked.

Fix that by breaking instead and letting the common error handling
code at the bottom of the loop to free the skb properly.

This is a very unlikely scenario as it requires the packet to become
unparseable by applying a set of actions on a previously parseable skb,
but should be fixed nevertheless.

Reported by Sashiko.

Fixes: ec0d043d05e6 ("openvswitch: Ensure flow is valid before executing ct")
Cc: stable@vger.kernel.org
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 net/openvswitch/actions.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 5653b6642e10..0500939cc8d8 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -1380,7 +1380,7 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
 			if (!is_flow_key_valid(key)) {
 				err = ovs_flow_key_update(skb, key);
 				if (err)
-					return err;
+					break;
 			}
 
 			err = ovs_ct_execute(ovs_dp_get_net(dp), skb, key,
-- 
2.55.0

Re: [net,2/2] net: openvswitch: fix skb leak on flow key update failure during ct

[Aaron Conole]

Ilya Maximets <i.maximets@ovn.org> writes:

> ovs_ct_execute() always steals or frees the skb on failure while
> ovs_flow_key_update() does not.  So, if it fails and we return right
> away, the skb ends up leaked.
> 
> Fix that by breaking instead and letting the common error handling
> code at the bottom of the loop to free the skb properly.
> 
> This is a very unlikely scenario as it requires the packet to become
> unparseable by applying a set of actions on a previously parseable skb,
> but should be fixed nevertheless.
> 
> Reported by Sashiko.
> 
> Fixes: ec0d043d05e6 ("openvswitch: Ensure flow is valid before executing ct")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---
>  net/openvswitch/actions.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Aaron Conole <aconole@redhat.com>