[PATCH net] net: openvswitch: fix possible kfree_skb of ERR_PTR

[PATCH net] net: openvswitch: fix possible kfree_skb of ERR_PTR

[Adrian Moreno]

After the patch in the "Fixes" tag, the allocation of the "reply" skb
can happen either before or after locking the ovs_mutex.

However, error cleanups still follow the classical reversed order,
assuming "reply" is allocated before locking: it is freed after unlocking.

If "reply" allocation happens after locking the mutex and it fails,
"reply" is left with an ERR_PTR, and execution jumps to the correspondent
cleanup stage which will try to free an invalid pointer.

Fix this by setting the pointer to NULL after having saved its error
value.

Fixes: 893f139b9a6c ("openvswitch: Minimize ovs_flow_cmd_new|set critical sections.")

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 net/openvswitch/datapath.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index bbbde50fc649..f0164817d9b7 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -1316,6 +1316,7 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
 
 		if (IS_ERR(reply)) {
 			error = PTR_ERR(reply);
+			reply = NULL;
 			goto err_unlock_ovs;
 		}
 	}
-- 
2.54.0

Re: [ovs-dev] [PATCH net] net: openvswitch: fix possible kfree_skb of ERR_PTR

[Aaron Conole]

Hi Adrian,

Adrian Moreno via dev <ovs-dev@openvswitch.org> writes:

> After the patch in the "Fixes" tag, the allocation of the "reply" skb
> can happen either before or after locking the ovs_mutex.
>
> However, error cleanups still follow the classical reversed order,
> assuming "reply" is allocated before locking: it is freed after unlocking.
>
> If "reply" allocation happens after locking the mutex and it fails,
> "reply" is left with an ERR_PTR, and execution jumps to the correspondent
> cleanup stage which will try to free an invalid pointer.
>
> Fix this by setting the pointer to NULL after having saved its error
> value.
>
> Fixes: 893f139b9a6c ("openvswitch: Minimize ovs_flow_cmd_new|set
> critical sections.")
>
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---

Good catch - I guess this should only happen when modifying an existing
flow without putting any actions (and that would be only from an
implicit drop case since the actions list would be empty).  CC'ing
Minxi, since he's recently had interest in the selftests area and may be
able to help with writing a test case for the scenario.

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