net: openvswitch: fix flow memory leak in ovs_flow_cmd_new

Syzkaller reports a memory leak of new_flow in ovs_flow_cmd_new() as it is not freed when an allocation of a key fails. BUG: memory leak unreferenced object 0xffff888116668000 (size 632): comm "syz-executor231", pid 1090, jiffies 4294844701 (age 18.871s) hex dump (first 32 bytes): 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [<00000000defa3494>] kmem_cache_zalloc include/linux/slab.h:654 [inline] [<00000000defa3494>] ovs_flow_alloc+0x19/0x180 net/openvswitch/flow_table.c:77 [<00000000c67d8873>] ovs_flow_cmd_new+0x1de/0xd40 net/openvswitch/datapath.c:957 [<0000000010a539a8>] genl_family_rcv_msg_doit+0x22d/0x330 net/netlink/genetlink.c:739 [<00000000dff3302d>] genl_family_rcv_msg net/netlink/genetlink.c:783 [inline] [<00000000dff3302d>] genl_rcv_msg+0x328/0x590 net/netlink/genetlink.c:800 [<000000000286dd87>] netlink_rcv_skb+0x153/0x430 net/netlink/af_netlink.c:2515 [<0000000061fed410>] genl_rcv+0x24/0x40 net/netlink/genetlink.c:811 [<000000009dc0f111>] netlink_unicast_kernel net/netlink/af_netlink.c:1313 [inline] [<000000009dc0f111>] netlink_unicast+0x545/0x7f0 net/netlink/af_netlink.c:1339 [<000000004a5ee816>] netlink_sendmsg+0x8e7/0xde0 net/netlink/af_netlink.c:1934 [<00000000482b476f>] sock_sendmsg_nosec net/socket.c:651 [inline] [<00000000482b476f>] sock_sendmsg+0x152/0x190 net/socket.c:671 [<00000000698574ba>] ____sys_sendmsg+0x70a/0x870 net/socket.c:2356 [<00000000d28d9e11>] ___sys_sendmsg+0xf3/0x170 net/socket.c:2410 [<0000000083ba9120>] __sys_sendmsg+0xe5/0x1b0 net/socket.c:2439 [<00000000c00628f8>] do_syscall_64+0x30/0x40 arch/x86/entry/common.c:46 [<000000004abfdcf4>] entry_SYSCALL_64_after_hwframe+0x61/0xc6 To fix this the patch rearranges the goto labels to reflect the order of object allocations and adds appropriate goto statements on the error paths. Found by Linux Verification Center (linuxtesting.org) with Syzkaller.

Fedor Pchelkin committed 3 years ago

Commit 0c598ae1 parent a1f47751 file · +6 −6Browse files

Showing the changes to net/openvswitch (1 of 1 file). Show all

Unified · Split

net/openvswitch/datapath.c+6−6

@@ -1004,14 +1004,14 @@ static int ovs_flow_cmd_new(struct sk_buff skb, struct genl_info info) · 1003 unmodified lines
1004	1004	key = kzalloc(sizeof(*key), GFP_KERNEL);
1005	1005	if (!key) {
1006	1006	error = -ENOMEM;
1007		− goto err_kfree_key;
	1007	+ goto err_kfree_flow;
1008	1008	}
1009	1009
1010	1010	ovs_match_init(&match, key, false, &mask);
1011	1011	error = ovs_nla_get_match(net, &match, a[OVS_FLOW_ATTR_KEY],
1012	1012	a[OVS_FLOW_ATTR_MASK], log);
1013	1013	if (error)
1014		− goto err_kfree_flow;
	1014	+ goto err_kfree_key;
1015	1015
1016	1016	ovs_flow_mask_key(&new_flow->key, key, true, &mask);
1017	1017
@@ -1019,14 +1019,14 @@ static int ovs_flow_cmd_new(struct sk_buff skb, struct genl_info info) · 1 unmodified line
1019	1019	error = ovs_nla_get_identifier(&new_flow->id, a[OVS_FLOW_ATTR_UFID],
1020	1020	key, log);
1021	1021	if (error)
1022		− goto err_kfree_flow;
	1022	+ goto err_kfree_key;
1023	1023
1024	1024	/* Validate actions. */
1025	1025	error = ovs_nla_copy_actions(net, a[OVS_FLOW_ATTR_ACTIONS],
1026	1026	&new_flow->key, &acts, log);
1027	1027	if (error) {
1028	1028	OVS_NLERR(log, "Flow actions may not be safe on all matching packets.");
1029		− goto err_kfree_flow;
	1029	+ goto err_kfree_key;
1030	1030	}
1031	1031
1032	1032	reply = ovs_flow_cmd_alloc_info(acts, &new_flow->id, info, false,
@@ -1126,10 +1126,10 @@ err_unlock_ovs: · 93 unmodified lines
1126	1126	kfree_skb(reply);
1127	1127	err_kfree_acts:
1128	1128	ovs_nla_free_flow_actions(acts);
1129		−err_kfree_flow:
1130		− ovs_flow_free(new_flow, false);
1131	1129	err_kfree_key:
1132	1130	kfree(key);
	1131	+err_kfree_flow:
	1132	+ ovs_flow_free(new_flow, false);
1133	1133	error:
1134	1134	return error;
1135	1135	}