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: <Yxx5sJh/TLzSR5xU@x-berg.in-berlin.de> Date: Sat, 10 Sep 2022 13:49:04 +0200 From: Thomas Osterried <thomas@...erried.de> To: Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org> Cc: "David S . Miller" <davem@...emloft.net>, Paolo Abeni <pabeni@...hat.com>, Bernard Pidoux <f6bvp@...e.fr>, Duoming Zhou <duoming@....edu.cn>, netdev@...r.kernel.org, linux-hams@...r.kernel.org Subject: Re: [AX25] patch did not fix -- was: ax25: fix incorrect dev_tracker usage Hello, please allow me the question what the patch tries to fix. 1. we add sessions to the list of active sessions ax25_cb_add(ax25), and remove them on close. Why do we need a tracker? 2. ax25_dev.c: ax25_dev_device_up(): netdev_hold(dev, &ax25_dev->dev_tracker, GFP_KERNEL); ax25_dev_device_down(): netdev_put(dev, &ax25_dev->dev_tracker); ax25_dev_free(): netdev_put(ax25_dev->dev, &ax25_dev->dev_tracker); Just to be sure: It's the list of active ax25-interface, correct? -> Looks good to me. 3. On device status change, i.e. NETDEV_DOWN, ax25_device_event() calls ax25_kill_by_device(dev) and ax25_dev_device_down(dev) (we saw in 2)). Before patches 7c6327c77d509e78bff76f2a4551fcfee851682e / d7c4c9e075f8cc6d88d277bc24e5d99297f03c06 we did in ax25_relase(): netdev_put(ax25_dev->dev, &ax25_dev->dev_tracker); and in ax25_bind(); netdev_hold(ax25_dev->dev, &ax25_dev->dev_tracker, GFP_ATOMIC); ax25_kill_by_device() traverses through the list of active connections (ax25_list): if (sk->sk_socket) { netdev_put(ax25_dev->dev, &ax25_dev->dev_tracker); ax25_dev_put(ax25_dev); } The patches mentioned above were: ax25_release(): netdev_put(ax25_dev->dev, &ax25->dev_tracker); ax25_bind(): netdev_hold(ax25_dev->dev, &ax25->dev_tracker, GFP_ATOMIC); -> Previously, each session (ax25_cb) was added to it's device with the dev_tracker of the _device_ (&ax25_dev->dev_tracker) Now, each session (ax25_cb) is added with it's own dev_tracker &ax25->dev_tracker But: ax25_kill_by_device() goes through the list of active sessions and does netdev_put(ax25_dev->dev, &ax25_dev->dev_tracker); instead of the new concept; I would have expected: netdev_put(ax25_dev->dev, &s->dev_tracker); If we netdev_put() to a non-existent tracker, this may explain the warnings unregister_netdevice: waiting for bpq1 to become free. Usage count = -2 and refcount_t: underflow; use-after-free. that I observed in my previous mail. 4. Again, my question for understanding about the dev_tracker concept: should _all_ in- and outgoing sessions be tracked? If so, I argue we have to add - new outbound sessions to the tracker (i.E. if a IP mode VC frame is sent) - new inbound sessions to the tracker Also, ax25_bind() currently does not track all sessions: /* * User already set interface with SO_BINDTODEVICE */ if (ax25->ax25_dev != NULL) goto done; ... if (ax25_dev) { ax25_fillin_cb(ax25, ax25_dev); netdev_hold(ax25_dev->dev, &ax25->dev_tracker, GFP_ATOMIC); } done: ax25_cb_add(ax25); -> If user has previously called ax25_setsockopt() with SO_BINDTODEVICE, the device has been added to ax25->ax25_dev. But ax25_bind() does not add it to the tracker list (see above, ax25->ax25_dev is != NULL). ax25_connect() does also currently not track all sessions: There's a spcial condition sock_flag(sk, SOCK_ZAPPED), where connect() is allowed to be called without having gone through ax25_bind(). Via ax25_rt_autobind(ax25, ..) it get's the appropriete ax25>dev. -> ax25_fillin_cb(ax25, ax25->ax25_dev); ax25_cb_add(ax25); => No tracker is added for this session. ax25_kill_by_device() does remove the tracker. I argued above, it removes the wrong tracker here: if (sk->sk_socket) { netdev_put(ax25_dev->dev, &ax25_dev->dev_tracker); ax25_dev_put(ax25_dev); } ..instead of &s->dev_tracker. And if we have trackers for all sessions (not only those that came via ax25_bind() from userspace), it's not enough to remove the tracker only for sessions with sk->socket. -> I would have expeted ax25_for_each(s, &ax25_list) { if (s->ax25_dev == ax25_dev) { if (s->ax25_dev == ax25_dev) { netdev_put(ax25_dev->dev, &s->dev_tracker); ... Finally: On normal session close (userspace program, or idle timer expiry), we need to have timers running for correct AX.25-session-close: If we are in connected state (AX25_STATE_3 or AX25_STATE_4), we need to send DISC in interval until max retry reached, or until we receive a DM-). In ax25_release(), we see: if (ax25_dev) { ... netdev_put(ax25_dev->dev, &ax25->dev_tracker); ax25_dev_put(ax25_dev); But we need have timers running for ax25_cb's that still refer to the network dev (and need to be there until correct session termination), we have a conflict here, because netdev_put() assures here we are not allowed to refer to the dev anymore. That needs to be resolved. Context: diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c index bbac3cb4dc99d..d82a51e69386b 100644 --- a/net/ax25/af_ax25.c +++ b/net/ax25/af_ax25.c @@ -1066,7 +1066,7 @@ static int ax25_release(struct socket *sock) del_timer_sync(&ax25->t3timer); del_timer_sync(&ax25->idletimer); } - netdev_put(ax25_dev->dev, &ax25_dev->dev_tracker); + netdev_put(ax25_dev->dev, &ax25->dev_tracker); ax25_dev_put(ax25_dev); } @@ -1147,7 +1147,7 @@ static int ax25_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len) if (ax25_dev) { ax25_fillin_cb(ax25, ax25_dev); - netdev_hold(ax25_dev->dev, &ax25_dev->dev_tracker, GFP_ATOMIC); + netdev_hold(ax25_dev->dev, &ax25->dev_tracker, GFP_ATOMIC); } done:
Powered by blists - more mailing lists