Messages in this thread | | | Date | Mon, 31 Jan 2022 20:37:29 +0300 | From | Dan Carpenter <> | Subject | Re: [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
| |