Re: [PATCH 2/2] ax25: add refcount in ax25_dev to avoid UAF bugs

[Dan Carpenter <dan.carpenter@oracle.com>]

On Fri, Jan 28, 2022 at 12:47:16PM +0800, Duoming Zhou wrote:
> If we dereference ax25_dev after we call kfree(ax25_dev) in
> ax25_dev_device_down(), it will lead to concurrency UAF bugs.
> There are eight syscall functions suffer from UAF bugs, include
> ax25_bind(), ax25_release(), ax25_connect(), ax25_ioctl(),
> ax25_getname(), ax25_sendmsg(), ax25_getsockopt() and
> ax25_info_show().
> 
> One of the concurrency UAF can be shown as below:
> 
>   (USE)                       |    (FREE)
>                               |  ax25_device_event
>                               |    ax25_dev_device_down
> ax25_bind                     |    ...
>   ...                         |      kfree(ax25_dev)
>   ax25_fillin_cb()            |    ...
>     ax25_fillin_cb_from_dev() |
>   ...                         |
> 
> The root cause of UAF bugs is that kfree(ax25_dev) in
> ax25_dev_device_down() is not protected by any locks.
> When ax25_dev, which there are still pointers point to,
> is released, the concurrency UAF bug will happen.
> 
> This patch introduces refcount into ax25_dev in order to
> guarantee that there are no pointers point to it when ax25_dev
> is released.
> 
> Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>

I pointed out a few bugs in my previous email.  I've had more time to
look at it now.

Basically you just want to audit all the calls sites which call
ax25_dev_ax25dev() and make sure all the error paths decrement.  Most
of them are buggy.  I'm testing a new Smatch check which is supposed to
detect these sorts of bugs.

I think the refcount in ax25_bind() needs a matching decrement.  Where
is that?  I don't know networking well enough to know the answer to
this...

> @@ -112,20 +115,22 @@ void ax25_dev_device_down(struct net_device *dev)
>  
>  	if ((s = ax25_dev_list) == ax25_dev) {
>  		ax25_dev_list = s->next;
> +		ax25_dev_put(ax25_dev);

It would be more readable to do ax25_dev_put(ax25_dev_list).  It's weird
to put ax25_dev here and then a couple lines later

>  		spin_unlock_bh(&ax25_dev_lock);
>  		dev->ax25_ptr = NULL;
>  		dev_put_track(dev, &ax25_dev->dev_tracker);
> -		kfree(ax25_dev);
> +		ax25_dev_put(ax25_dev);

Here

>  		return;
>  	}
>  
>  	while (s != NULL && s->next != NULL) {
>  		if (s->next == ax25_dev) {
>  			s->next = ax25_dev->next;
> +			ax25_dev_put(ax25_dev);

Same.

>  			spin_unlock_bh(&ax25_dev_lock);
>  			dev->ax25_ptr = NULL;
>  			dev_put_track(dev, &ax25_dev->dev_tracker);
> -			kfree(ax25_dev);
> +			ax25_dev_put(ax25_dev);
>  			return;
>  		}
>  

regards,
dan carpenter