lkml.org 
[lkml]   [2022]   [Jan]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/2] ax25: add refcount in ax25_dev to avoid UAF bugs
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

\
 
 \ /
  Last update: 2022-01-31 18:38    [W:3.604 / U:0.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site