| 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: <2aa65b0c-2170-46c0-57a4-17b653e41f96@hartkopp.net>
Date: Thu, 6 Jul 2023 13:55:19 +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] can: raw: fix receiver memory leak
Hello Ziyang Xuan,
thanks for your patch and the found inconsistency!
The ro->ifindex value might be zero even on a bound CAN_RAW socket which
results in the use of a common filter for all CAN interfaces, see below ...
On 2023-07-05 11:25, Ziyang Xuan wrote:
(..)
> @@ -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);
>
This would be ok for raw_notify().
> @@ -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;
This would be ok too.
> @@ -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)
> + if (ro->bound && ro->dev && addr->can_ifindex == ro->dev->ifindex)
But this is wrong as the case for a bound socket for "all" CAN
interfaces (ifindex == 0) is not considered.
> 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 */
> - 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;
> +
> ro->bound = 1;
> + ro->dev = dev;
> }
>
> out:
> release_sock(sk);
> + rtnl_unlock();
Would it also fix the issue when just adding the rtnl_locks to
raw_bind() and raw_release() as suggested by you?
Many thanks,
Oliver
>
> 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);
> -
> 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();
>
Powered by blists - more mailing lists