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  PHC 
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sat, 10 Sep 2022 13:49:04 +0200
From:   Thomas Osterried <>
To:     Eric Dumazet <>,
        Jakub Kicinski <>
Cc:     "David S . Miller" <>,
        Paolo Abeni <>,
        Bernard Pidoux <>,
        Duoming Zhou <>,,
Subject: Re: [AX25] patch did not fix --  was: ax25: fix incorrect
 dev_tracker usage


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:
     netdev_hold(dev, &ax25_dev->dev_tracker, GFP_KERNEL);
     netdev_put(dev, &ax25_dev->dev_tracker);
     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_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);
    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
       if (sk->sk_socket) {

    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_kill_by_device() goes through the list of active sessions and
           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
           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
     - 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);

    -> 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);
      => 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) {
   ..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);

   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);
   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.


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)
-		netdev_put(ax25_dev->dev, &ax25_dev->dev_tracker);
+		netdev_put(ax25_dev->dev, &ax25->dev_tracker);
@@ -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);

Powered by blists - more mailing lists