| 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
| ||
|
Message-ID: <5c396de6-afab-6af9-f9d9-a698b9367873@hartkopp.net>
Date: Mon, 10 Jul 2023 21:01:29 +0200
From: Oliver Hartkopp <socketcan@...tkopp.net>
To: Ziyang Xuan <william.xuanziyang@...wei.com>, mkl@...gutronix.de,
davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, linux-can@...r.kernel.org, netdev@...r.kernel.org,
penguin-kernel@...ove.SAKURA.ne.jp
Subject: Re: [PATCH net v2] can: raw: fix receiver memory leak
Hello William,
On 07.07.23 09:53, Ziyang Xuan wrote:
> Got kmemleak errors with the following ltp can_filter testcase:
>
> for ((i=1; i<=100; i++))
> do
> ./can_filter &
> sleep 0.1
> done
>
> ==============================================================
> [<00000000db4a4943>] can_rx_register+0x147/0x360 [can]
> [<00000000a289549d>] raw_setsockopt+0x5ef/0x853 [can_raw]
> [<000000006d3d9ebd>] __sys_setsockopt+0x173/0x2c0
> [<00000000407dbfec>] __x64_sys_setsockopt+0x61/0x70
> [<00000000fd468496>] do_syscall_64+0x33/0x40
> [<00000000b7e47d51>] entry_SYSCALL_64_after_hwframe+0x61/0xc6
>
> It's a bug in the concurrent scenario of unregister_netdevice_many()
> and raw_release() as following:
>
> cpu0 cpu1
> unregister_netdevice_many(can_dev)
> unlist_netdevice(can_dev) // dev_get_by_index() return NULL after this
> net_set_todo(can_dev)
> raw_release(can_socket)
> dev = dev_get_by_index(, ro->ifindex); // dev == NULL
> if (dev) { // receivers in dev_rcv_lists not free because dev is NULL
> raw_disable_allfilters(, dev, );
> dev_put(dev);
> }
> ...
> ro->bound = 0;
> ...
>
> call_netdevice_notifiers(NETDEV_UNREGISTER, )
> raw_notify(, NETDEV_UNREGISTER, )
> if (ro->bound) // invalid because ro->bound has been set 0
> raw_disable_allfilters(, dev, ); // receivers in dev_rcv_lists will never be freed
>
> Add a net_device pointer member in struct raw_sock to record bound can_dev,
> and use rtnl_lock to serialize raw_socket members between raw_bind(), raw_release(),
> raw_setsockopt() and raw_notify(). Use ro->dev to decide whether to free receivers in
> dev_rcv_lists.
>
> Fixes: 8d0caedb7596 ("can: bcm/raw/isotp: use per module netdevice notifier")
> Signed-off-by: Ziyang Xuan <william.xuanziyang@...wei.com>
> ---
> v2:
> - Fix the case for a bound socket for "all" CAN interfaces (ifindex == 0) in raw_bind().
> ---
> net/can/raw.c | 61 ++++++++++++++++++++++-----------------------------
> 1 file changed, 26 insertions(+), 35 deletions(-)
>
> diff --git a/net/can/raw.c b/net/can/raw.c
> index 15c79b079184..7078821f35e0 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c
> @@ -84,6 +84,7 @@ struct raw_sock {
> struct sock sk;
> int bound;
> int ifindex;
> + struct net_device *dev;
> struct list_head notifier;
> int loopback;
> int recv_own_msgs;
> @@ -277,7 +278,7 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg,
> if (!net_eq(dev_net(dev), sock_net(sk)))
> return;
>
> - if (ro->ifindex != dev->ifindex)
> + if (ro->dev != dev)
> return;
>
> switch (msg) {
> @@ -292,6 +293,7 @@ static void raw_notify(struct raw_sock *ro, unsigned long msg,
>
> ro->ifindex = 0;
> ro->bound = 0;
> + ro->dev = NULL;
> ro->count = 0;
> release_sock(sk);
>
> @@ -337,6 +339,7 @@ static int raw_init(struct sock *sk)
>
> ro->bound = 0;
> ro->ifindex = 0;
> + ro->dev = NULL;
>
> /* set default filter to single entry dfilter */
> ro->dfilter.can_id = 0;
> @@ -385,19 +388,13 @@ static int raw_release(struct socket *sock)
>
> lock_sock(sk);
>
> + rtnl_lock();
> /* remove current filters & unregister */
> if (ro->bound) {
> - if (ro->ifindex) {
> - struct net_device *dev;
> -
> - dev = dev_get_by_index(sock_net(sk), ro->ifindex);
> - if (dev) {
> - raw_disable_allfilters(dev_net(dev), dev, sk);
> - dev_put(dev);
> - }
> - } else {
> + if (ro->dev)
> + raw_disable_allfilters(dev_net(ro->dev), ro->dev, sk);
> + else
> raw_disable_allfilters(sock_net(sk), NULL, sk);
> - }
> }
>
> if (ro->count > 1)
> @@ -405,8 +402,10 @@ static int raw_release(struct socket *sock)
>
> ro->ifindex = 0;
> ro->bound = 0;
> + ro->dev = NULL;
> ro->count = 0;
> free_percpu(ro->uniq);
> + rtnl_unlock();
>
> sock_orphan(sk);
> sock->sk = NULL;
> @@ -422,6 +421,7 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len)
> struct sockaddr_can *addr = (struct sockaddr_can *)uaddr;
> struct sock *sk = sock->sk;
> struct raw_sock *ro = raw_sk(sk);
> + struct net_device *dev = NULL;
> int ifindex;
> int err = 0;
> int notify_enetdown = 0;
> @@ -431,14 +431,13 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len)
> if (addr->can_family != AF_CAN)
> return -EINVAL;
>
> + rtnl_lock();
> lock_sock(sk);
>
> if (ro->bound && addr->can_ifindex == ro->ifindex)
> goto out;
>
> if (addr->can_ifindex) {
> - struct net_device *dev;
> -
> dev = dev_get_by_index(sock_net(sk), addr->can_ifindex);
> if (!dev) {
> err = -ENODEV;
> @@ -465,28 +464,23 @@ static int raw_bind(struct socket *sock, struct sockaddr *uaddr, int len)
> }
>
> if (!err) {
> + /* unregister old filters */
> if (ro->bound) {
> - /* unregister old filters */
Please move this comment back as we only unregister old filters when the
socket is bound.
> - if (ro->ifindex) {
> - struct net_device *dev;
> -
> - dev = dev_get_by_index(sock_net(sk),
> - ro->ifindex);
> - if (dev) {
> - raw_disable_allfilters(dev_net(dev),
> - dev, sk);
> - dev_put(dev);
> - }
> - } else {
> + if (ro->dev)
> + raw_disable_allfilters(dev_net(ro->dev),
> + ro->dev, sk);
> + else
> raw_disable_allfilters(sock_net(sk), NULL, sk);
> - }
> }
> ro->ifindex = ifindex;
> +
Why did you add an additional empty line here?
Please remove.
> ro->bound = 1;
> + ro->dev = dev;
> }
>
> out:
> release_sock(sk);
> + rtnl_unlock();
>
> if (notify_enetdown) {
> sk->sk_err = ENETDOWN;
> @@ -553,9 +547,9 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
> rtnl_lock();
> lock_sock(sk);
>
> - if (ro->bound && ro->ifindex) {
> - dev = dev_get_by_index(sock_net(sk), ro->ifindex);
> - if (!dev) {
> + dev = ro->dev;
> + if (ro->bound && dev) {
> + if (dev->reg_state != NETREG_REGISTERED) {
> if (count > 1)
> kfree(filter);
> err = -ENODEV;
> @@ -596,7 +590,6 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
> ro->count = count;
>
> out_fil:
> - dev_put(dev);
> release_sock(sk);
> rtnl_unlock();
>
> @@ -614,9 +607,9 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
> rtnl_lock();
> lock_sock(sk);
>
> - if (ro->bound && ro->ifindex) {
> - dev = dev_get_by_index(sock_net(sk), ro->ifindex);
> - if (!dev) {
> + dev = ro->dev;
> + if (ro->bound && dev) {
> + if (dev->reg_state != NETREG_REGISTERED) {
> err = -ENODEV;
> goto out_err;
> }
> @@ -627,7 +620,6 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
> /* (try to) register the new err_mask */
> err = raw_enable_errfilter(sock_net(sk), dev, sk,
> err_mask);
> -
And here you removed an empty line?
Please omit such mix of fixing a bug and change the coding style.
> if (err)
> goto out_err;
>
> @@ -640,7 +632,6 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
> ro->err_mask = err_mask;
>
> out_err:
> - dev_put(dev);
> release_sock(sk);
> rtnl_unlock();
>
The rest looks fine now, many thanks!
It also reduces the code size.
When you send the v3 you can add these tags:
Reviewed-by: Oliver Hartkopp <socketcan@...tkopp.net>
Acked-by: Oliver Hartkopp <socketcan@...tkopp.net>
Best regards,
Oliver
Powered by blists - more mailing lists