netdev - Re: [ovs-dev] [PATCH net] openvswitch: fix memory leak at failed datapath creation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <7d8df50d-48a9-3a14-786d-1e1452a1e73b@ovn.org>
Date:   Wed, 24 Aug 2022 17:11:15 +0200
From:   Ilya Maximets <i.maximets@....org>
To:     Andrey Zhadchenko <andrey.zhadchenko@...tuozzo.com>,
        netdev@...r.kernel.org
Cc:     dev@...nvswitch.org, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, davem@...emloft.net, i.maximets@....org,
        Aaron Conole <aconole@...hat.com>
Subject: Re: [ovs-dev] [PATCH net] openvswitch: fix memory leak at failed
 datapath creation

On 8/23/22 17:12, Andrey Zhadchenko via dev wrote:
> ovs_dp_cmd_new()->ovs_dp_change()->ovs_dp_set_upcall_portids()
> allocates array via kmalloc.
> If for some reason new_vport() fails during ovs_dp_cmd_new()
> dp->upcall_portids must be freed.
> Add missing kfree.
> 
> Kmemleak example:
> unreferenced object 0xffff88800c382500 (size 64):
>   comm "dump_state", pid 323, jiffies 4294955418 (age 104.347s)
>   hex dump (first 32 bytes):
>     5e c2 79 e4 1f 7a 38 c7 09 21 38 0c 80 88 ff ff  ^.y..z8..!8.....
>     03 00 00 00 0a 00 00 00 14 00 00 00 28 00 00 00  ............(...
>   backtrace:
>     [<0000000071bebc9f>] ovs_dp_set_upcall_portids+0x38/0xa0
>     [<000000000187d8bd>] ovs_dp_change+0x63/0xe0
>     [<000000002397e446>] ovs_dp_cmd_new+0x1f0/0x380
>     [<00000000aa06f36e>] genl_family_rcv_msg_doit+0xea/0x150
>     [<000000008f583bc4>] genl_rcv_msg+0xdc/0x1e0
>     [<00000000fa10e377>] netlink_rcv_skb+0x50/0x100
>     [<000000004959cece>] genl_rcv+0x24/0x40
>     [<000000004699ac7f>] netlink_unicast+0x23e/0x360
>     [<00000000c153573e>] netlink_sendmsg+0x24e/0x4b0
>     [<000000006f4aa380>] sock_sendmsg+0x62/0x70
>     [<00000000d0068654>] ____sys_sendmsg+0x230/0x270
>     [<0000000012dacf7d>] ___sys_sendmsg+0x88/0xd0
>     [<0000000011776020>] __sys_sendmsg+0x59/0xa0
>     [<000000002e8f2dc1>] do_syscall_64+0x3b/0x90
>     [<000000003243e7cb>] entry_SYSCALL_64_after_hwframe+0x63/0xcd
> 
> Fixes: b83d23a2a38b ("openvswitch: Introduce per-cpu upcall dispatch")
> Acked-by: Aaron Conole <aconole@...hat.com>
> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@...tuozzo.com>
> ---
>  net/openvswitch/datapath.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index 7e8a39a35627..3eb1dc435276 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -1802,7 +1802,7 @@ static int ovs_dp_cmd_new(struct sk_buff *skb, struct genl_info *info)
>  				ovs_dp_reset_user_features(skb, info);
>  		}
>  
> -		goto err_unlock_and_destroy_meters;
> +		goto err_destroy_pids;
>  	}
>  
>  	err = ovs_dp_cmd_fill_info(dp, reply, info->snd_portid,
> @@ -1817,6 +1817,9 @@ static int ovs_dp_cmd_new(struct sk_buff *skb, struct genl_info *info)
>  	ovs_notify(&dp_datapath_genl_family, reply, info);
>  	return 0;
>  
> +err_destroy_pids:
> +	if (rcu_dereference_raw(dp->upcall_portids))
> +		kfree(rcu_dereference_raw(dp->upcall_portids));

kfree() as all typical implementations of free()-like functions
should be perfectly fine to call with a NULL argument.

Also, dereferencing RCU pointer twice is a bad pattern.  I know,
it's not a real problem here, since no other threads seen that
pointer yet.  But it's probably better to avoid bad patterns
anyway, as someone may copy it in the future to other place where
it will be a problem.

Best regards, Ilya Maximets.