Re: Regression in "ax25: Fix UAF bugs in ax25 timers", patch 82e31755e55fbcea6a9dfaae5fe4860ade17cbc0

[duoming@zju.edu.cn]

Hello,

On Tue, 24 May 2022 11:25:28 +0200 Thomas wrote:

> > I tried to apply your patch but the format is wrong...  :/
> 
> I thought it would help us to discuss the problem on the mailinglist
> at the code fragment.
> But I also asked for testing, and then you need a working patch
> Was my fault. Sorry for that.
> 
> 
> diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
> index 363d47f94532..498e92fb43b7 100644
> --- a/net/ax25/af_ax25.c
> +++ b/net/ax25/af_ax25.c
> @@ -86,11 +86,17 @@ static void ax25_kill_by_device(struct net_device *dev)
>  again:
>         ax25_for_each(s, &ax25_list) {
>                 if (s->ax25_dev == ax25_dev) {
> +                       del_timer_sync(&s->timer);
> +                       del_timer_sync(&s->t1timer);
> +                       del_timer_sync(&s->t2timer);
> +                       del_timer_sync(&s->t3timer);
> +                       del_timer_sync(&s->idletimer);

These del_timer_sync() functions are unnecessary, Because when the device is detaching, 
the reason parameter of ax25_disconnect() equals to ENETUNREACH, the del_timer_sync()
in ax25_disconnect() will execute.

>                         sk = s->sk;
>                         if (!sk) {
>                                 spin_unlock_bh(&ax25_list_lock);
>                                 ax25_disconnect(s, ENETUNREACH);
>                                 s->ax25_dev = NULL;
> +                               ax25_cb_del(s);
>                                 spin_lock_bh(&ax25_list_lock);
>                                 goto again;
>                         }
> @@ -104,6 +110,7 @@ static void ax25_kill_by_device(struct net_device *dev)
>                                 ax25_dev_put(ax25_dev);
>                         }
>                         release_sock(sk);
> +                       ax25_cb_del(s);
>                         spin_lock_bh(&ax25_list_lock);
>                         sock_put(sk);
>                         /* The entry could have been deleted from the
> @@ -1052,12 +1059,8 @@ static int ax25_release(struct socket *sock)
>                 sk->sk_state_change(sk);
>                 ax25_destroy_socket(ax25);
>         }
> +
>         if (ax25_dev) {
> -               del_timer_sync(&ax25->timer);
> -               del_timer_sync(&ax25->t1timer);
> -               del_timer_sync(&ax25->t2timer);
> -               del_timer_sync(&ax25->t3timer);
> -               del_timer_sync(&ax25->idletimer);
>                 dev_put_track(ax25_dev->dev, &ax25_dev->dev_tracker);
>                 ax25_dev_put(ax25_dev);
>         }

I think these del_timer_sync() functions could not be removed, otherwise the UAF bugs caused by
timer handler could not be mitigated.

      (Thread 1)                       |      (Thread 2)
ax25_dev_device_up() //(1)             |
...                                    | ax25_kill_by_device()
ax25_bind()          //(2)             |
ax25_connect()                         | ...
 ax25_std_establish_data_link()        |
  ax25_start_t1timer()                 | ax25_dev_device_down() //(3)
   mod_timer(&ax25->t1timer,..)        |
                                       | ax25_release()
   (wait a time)                       |  ...
                                       |  ax25_dev_put(ax25_dev) //(4)FREE
   ax25_t1timer_expiry()               |
    ax25->ax25_dev->values[..] //(5)USE|  ...
     ...                               |

The ax25_dev is deallocated in position (4) and use in position (5).

> In that context, I ask to revert (as I argued in my initial mail):
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/net/ax25/ax25_subr.c?id=fc6d01ff9ef03b66d4a3a23b46fc3c3d8cf92009

This patch could not be revert. Because when the device is detaching, the timer handler could be running.
If we do not wait the timer handler to finish, the NULL pointer dereference bug will happen.


      (Thread 1)                    |      (Thread 2)
ax25_connect()                      |
 ax25_std_establish_data_link()     |
  ax25_start_t1timer()              |
   mod_timer(&ax25->t1timer,..)     |
                                    | ax25_kill_by_device()
   (wait a time)                    |  ...
                                    |  s->ax25_dev = NULL; //(1)
   ax25_t1timer_expiry()            |
    ax25->ax25_dev->values[..] //(2)|  ...
     ...                            |

We set null to ax25_cb->ax25_dev in position (1) and dereference
the null pointer in position (2).

> In my diff from yesterday, I also had a part around "case AX25_STATE_0:").
> This fixes another problem.
> I removed it from my patch above: it has to discussed later as separate topic.
> 
> 
> Btw, Douming, you introduced del_timer_sync().
> ax25_stop_heartbeat(), ax25_stop_t1timer(), ax25_stop_t2timer(), ax25_stop_t3timer(),
> ax25_stop_idletimer() from ax25_timer.c use del_timer().
> 
> Why do you use del_timer_sync() directly?
> And if del_timer_sync() is needed, is it only needed in a special case,
> or isn't it cleaner to change the ax25_stop_xxx() timer functions?

We could not change del_timer() in ax25_stop_xxx() to del_timer_sync(). Because
the ax25_stop_xxx() is called by many functions such as ax25_ds_state1_machine(),
ax25_ctl_ioctl(), ax25_release() and so on. If we block and wait for timer handler
to finish in these functions, the system will hang in some cases.

Only the reason parameter of ax25_disconnect() equals to ENETUNREACH which means
the device is detaching or the ax25 socket has been destroyed should we use
del_timer_sync() to wait the timer handler to finish. 

Best regards,
Duoming Zhou