* [PATCH v3] workqueue: fix UAF in pwq_unbound_release_workfn()
@ 2021-07-14 9:19 Yang Yingliang
2021-07-15 8:08 ` Lai Jiangshan
2021-07-16 16:14 ` Tejun Heo
0 siblings, 2 replies; 5+ messages in thread
From: Yang Yingliang @ 2021-07-14 9:19 UTC (permalink / raw)
To: linux-kernel; +Cc: tj, jiangshanlai, xuqiang36, paskripkin
I got a UAF report when doing fuzz test:
[ 152.880091][ T8030] ==================================================================
[ 152.881240][ T8030] BUG: KASAN: use-after-free in pwq_unbound_release_workfn+0x50/0x190
[ 152.882442][ T8030] Read of size 4 at addr ffff88810d31bd00 by task kworker/3:2/8030
[ 152.883578][ T8030]
[ 152.883932][ T8030] CPU: 3 PID: 8030 Comm: kworker/3:2 Not tainted 5.13.0+ #249
[ 152.885014][ T8030] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014
[ 152.886442][ T8030] Workqueue: events pwq_unbound_release_workfn
[ 152.887358][ T8030] Call Trace:
[ 152.887837][ T8030] dump_stack_lvl+0x75/0x9b
[ 152.888525][ T8030] ? pwq_unbound_release_workfn+0x50/0x190
[ 152.889371][ T8030] print_address_description.constprop.10+0x48/0x70
[ 152.890326][ T8030] ? pwq_unbound_release_workfn+0x50/0x190
[ 152.891163][ T8030] ? pwq_unbound_release_workfn+0x50/0x190
[ 152.891999][ T8030] kasan_report.cold.15+0x82/0xdb
[ 152.892740][ T8030] ? pwq_unbound_release_workfn+0x50/0x190
[ 152.893594][ T8030] __asan_load4+0x69/0x90
[ 152.894243][ T8030] pwq_unbound_release_workfn+0x50/0x190
[ 152.895057][ T8030] process_one_work+0x47b/0x890
[ 152.895778][ T8030] worker_thread+0x5c/0x790
[ 152.896439][ T8030] ? process_one_work+0x890/0x890
[ 152.897163][ T8030] kthread+0x223/0x250
[ 152.897747][ T8030] ? set_kthread_struct+0xb0/0xb0
[ 152.898471][ T8030] ret_from_fork+0x1f/0x30
[ 152.899114][ T8030]
[ 152.899446][ T8030] Allocated by task 8884:
[ 152.900084][ T8030] kasan_save_stack+0x21/0x50
[ 152.900769][ T8030] __kasan_kmalloc+0x88/0xb0
[ 152.901416][ T8030] __kmalloc+0x29c/0x460
[ 152.902014][ T8030] alloc_workqueue+0x111/0x8e0
[ 152.902690][ T8030] __btrfs_alloc_workqueue+0x11e/0x2a0
[ 152.903459][ T8030] btrfs_alloc_workqueue+0x6d/0x1d0
[ 152.904198][ T8030] scrub_workers_get+0x1e8/0x490
[ 152.904929][ T8030] btrfs_scrub_dev+0x1b9/0x9c0
[ 152.905599][ T8030] btrfs_ioctl+0x122c/0x4e50
[ 152.906247][ T8030] __x64_sys_ioctl+0x137/0x190
[ 152.906916][ T8030] do_syscall_64+0x34/0xb0
[ 152.907535][ T8030] entry_SYSCALL_64_after_hwframe+0x44/0xae
[ 152.908365][ T8030]
[ 152.908688][ T8030] Freed by task 8884:
[ 152.909243][ T8030] kasan_save_stack+0x21/0x50
[ 152.909893][ T8030] kasan_set_track+0x20/0x30
[ 152.910541][ T8030] kasan_set_free_info+0x24/0x40
[ 152.911265][ T8030] __kasan_slab_free+0xf7/0x140
[ 152.911964][ T8030] kfree+0x9e/0x3d0
[ 152.912501][ T8030] alloc_workqueue+0x7d7/0x8e0
[ 152.913182][ T8030] __btrfs_alloc_workqueue+0x11e/0x2a0
[ 152.913949][ T8030] btrfs_alloc_workqueue+0x6d/0x1d0
[ 152.914703][ T8030] scrub_workers_get+0x1e8/0x490
[ 152.915402][ T8030] btrfs_scrub_dev+0x1b9/0x9c0
[ 152.916077][ T8030] btrfs_ioctl+0x122c/0x4e50
[ 152.916729][ T8030] __x64_sys_ioctl+0x137/0x190
[ 152.917414][ T8030] do_syscall_64+0x34/0xb0
[ 152.918034][ T8030] entry_SYSCALL_64_after_hwframe+0x44/0xae
[ 152.918872][ T8030]
[ 152.919203][ T8030] The buggy address belongs to the object at ffff88810d31bc00
[ 152.919203][ T8030] which belongs to the cache kmalloc-512 of size 512
[ 152.921155][ T8030] The buggy address is located 256 bytes inside of
[ 152.921155][ T8030] 512-byte region [ffff88810d31bc00, ffff88810d31be00)
[ 152.922993][ T8030] The buggy address belongs to the page:
[ 152.923800][ T8030] page:ffffea000434c600 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x10d318
[ 152.925249][ T8030] head:ffffea000434c600 order:2 compound_mapcount:0 compound_pincount:0
[ 152.926399][ T8030] flags: 0x57ff00000010200(slab|head|node=1|zone=2|lastcpupid=0x7ff)
[ 152.927515][ T8030] raw: 057ff00000010200 dead000000000100 dead000000000122 ffff888009c42c80
[ 152.928716][ T8030] raw: 0000000000000000 0000000080100010 00000001ffffffff 0000000000000000
[ 152.929890][ T8030] page dumped because: kasan: bad access detected
[ 152.930759][ T8030]
[ 152.931076][ T8030] Memory state around the buggy address:
[ 152.931851][ T8030] ffff88810d31bc00: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 152.932967][ T8030] ffff88810d31bc80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 152.934068][ T8030] >ffff88810d31bd00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 152.935189][ T8030] ^
[ 152.935763][ T8030] ffff88810d31bd80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 152.936847][ T8030] ffff88810d31be00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[ 152.937940][ T8030] ==================================================================
If apply_wqattrs_prepare() fails in alloc_workqueue(), it will call put_pwq()
which invoke a work queue to call pwq_unbound_release_workfn() and use the 'wq'.
The 'wq' allocated in alloc_workqueue() will be freed in error path when
apply_wqattrs_prepare() fails. So it will lead a UAF.
CPU0 CPU1
alloc_workqueue()
alloc_and_link_pwqs()
apply_wqattrs_prepare() fails
apply_wqattrs_cleanup()
schedule_work(&pwq->unbound_release_work)
kfree(wq)
worker_thread()
pwq_unbound_release_workfn() <- trigger uaf here
If apply_wqattrs_prepare() fails, the new pwq are not linked, it doesn't
hold any reference to the 'wq', 'wq' is invalid to access in the worker,
so add check pwq if linked to fix this.
Fixes: 2d5f0764b526 ("workqueue: split apply_workqueue_attrs() into 3 stages")
Reported-by: Hulk Robot <hulkci@huawei.com>
Suggested-by: Lai Jiangshan <jiangshanlai@gmail.com>
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
v3:
drop the v2 and v1 changes, add check pwq in pwq_unbound_release_workfn()
v2:
also use free_wqattrs_ctx() in workqueue_apply_unbound_cpumask()
---
kernel/workqueue.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 50142fc08902..f148eacda55a 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -3676,15 +3676,21 @@ static void pwq_unbound_release_workfn(struct work_struct *work)
unbound_release_work);
struct workqueue_struct *wq = pwq->wq;
struct worker_pool *pool = pwq->pool;
- bool is_last;
+ bool is_last = false;
- if (WARN_ON_ONCE(!(wq->flags & WQ_UNBOUND)))
- return;
+ /*
+ * when @pwq is not linked, it doesn't hold any reference to the
+ * @wq, and @wq is invalid to access.
+ */
+ if (!list_empty(&pwq->pwqs_node)) {
+ if (WARN_ON_ONCE(!(wq->flags & WQ_UNBOUND)))
+ return;
- mutex_lock(&wq->mutex);
- list_del_rcu(&pwq->pwqs_node);
- is_last = list_empty(&wq->pwqs);
- mutex_unlock(&wq->mutex);
+ mutex_lock(&wq->mutex);
+ list_del_rcu(&pwq->pwqs_node);
+ is_last = list_empty(&wq->pwqs);
+ mutex_unlock(&wq->mutex);
+ }
mutex_lock(&wq_pool_mutex);
put_unbound_pool(pool);
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3] workqueue: fix UAF in pwq_unbound_release_workfn()
2021-07-14 9:19 [PATCH v3] workqueue: fix UAF in pwq_unbound_release_workfn() Yang Yingliang
@ 2021-07-15 8:08 ` Lai Jiangshan
2021-07-15 18:52 ` Pavel Skripkin
2021-07-16 16:14 ` Tejun Heo
1 sibling, 1 reply; 5+ messages in thread
From: Lai Jiangshan @ 2021-07-15 8:08 UTC (permalink / raw)
To: Yang Yingliang; +Cc: LKML, Tejun Heo, Xu Qiang, Pavel Skripkin
On Wed, Jul 14, 2021 at 5:16 PM Yang Yingliang <yangyingliang@huawei.com> wrote:
>
> Fixes: 2d5f0764b526 ("workqueue: split apply_workqueue_attrs() into 3 stages")
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Suggested-by: Lai Jiangshan <jiangshanlai@gmail.com>
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> ---
> v3:
> drop the v2 and v1 changes, add check pwq in pwq_unbound_release_workfn()
> v2:
> also use free_wqattrs_ctx() in workqueue_apply_unbound_cpumask()
> ---
> kernel/workqueue.c | 20 +++++++++++++-------
> 1 file changed, 13 insertions(+), 7 deletions(-)
>
I'm fine with the code.
Reviewed-by: Lai Jiangshan <jiangshanlai@gmail.com>
Hello, Pavel
Could you have a test again, please?
Thanks,
Lai
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 50142fc08902..f148eacda55a 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -3676,15 +3676,21 @@ static void pwq_unbound_release_workfn(struct work_struct *work)
> unbound_release_work);
> struct workqueue_struct *wq = pwq->wq;
> struct worker_pool *pool = pwq->pool;
> - bool is_last;
> + bool is_last = false;
>
> - if (WARN_ON_ONCE(!(wq->flags & WQ_UNBOUND)))
> - return;
> + /*
> + * when @pwq is not linked, it doesn't hold any reference to the
> + * @wq, and @wq is invalid to access.
> + */
> + if (!list_empty(&pwq->pwqs_node)) {
> + if (WARN_ON_ONCE(!(wq->flags & WQ_UNBOUND)))
> + return;
>
> - mutex_lock(&wq->mutex);
> - list_del_rcu(&pwq->pwqs_node);
> - is_last = list_empty(&wq->pwqs);
> - mutex_unlock(&wq->mutex);
> + mutex_lock(&wq->mutex);
> + list_del_rcu(&pwq->pwqs_node);
> + is_last = list_empty(&wq->pwqs);
> + mutex_unlock(&wq->mutex);
> + }
>
> mutex_lock(&wq_pool_mutex);
> put_unbound_pool(pool);
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] workqueue: fix UAF in pwq_unbound_release_workfn()
2021-07-15 8:08 ` Lai Jiangshan
@ 2021-07-15 18:52 ` Pavel Skripkin
2021-07-16 10:45 ` Pavel Skripkin
0 siblings, 1 reply; 5+ messages in thread
From: Pavel Skripkin @ 2021-07-15 18:52 UTC (permalink / raw)
To: Lai Jiangshan; +Cc: Yang Yingliang, LKML, Tejun Heo, Xu Qiang
On Thu, 15 Jul 2021 16:08:12 +0800
Lai Jiangshan <jiangshanlai@gmail.com> wrote:
> On Wed, Jul 14, 2021 at 5:16 PM Yang Yingliang
> <yangyingliang@huawei.com> wrote:
>
> >
> > Fixes: 2d5f0764b526 ("workqueue: split apply_workqueue_attrs() into
> > 3 stages") Reported-by: Hulk Robot <hulkci@huawei.com>
> > Suggested-by: Lai Jiangshan <jiangshanlai@gmail.com>
> > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> > ---
> > v3:
> > drop the v2 and v1 changes, add check pwq in
> > pwq_unbound_release_workfn() v2:
> > also use free_wqattrs_ctx() in workqueue_apply_unbound_cpumask()
> > ---
> > kernel/workqueue.c | 20 +++++++++++++-------
> > 1 file changed, 13 insertions(+), 7 deletions(-)
> >
>
> I'm fine with the code.
>
> Reviewed-by: Lai Jiangshan <jiangshanlai@gmail.com>
>
> Hello, Pavel
>
> Could you have a test again, please?
>
Hi, Lai!
Sorry for late response.
Yes, I have started local syzkaller instance just now, so I will share
the results tomorrow.
> Thanks,
> Lai
>
> > diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> > index 50142fc08902..f148eacda55a 100644
> > --- a/kernel/workqueue.c
> > +++ b/kernel/workqueue.c
> > @@ -3676,15 +3676,21 @@ static void
> > pwq_unbound_release_workfn(struct work_struct *work)
> > unbound_release_work); struct workqueue_struct *wq = pwq->wq;
> > struct worker_pool *pool = pwq->pool;
> > - bool is_last;
> > + bool is_last = false;
> >
> > - if (WARN_ON_ONCE(!(wq->flags & WQ_UNBOUND)))
> > - return;
> > + /*
> > + * when @pwq is not linked, it doesn't hold any reference
> > to the
> > + * @wq, and @wq is invalid to access.
> > + */
> > + if (!list_empty(&pwq->pwqs_node)) {
> > + if (WARN_ON_ONCE(!(wq->flags & WQ_UNBOUND)))
> > + return;
> >
> > - mutex_lock(&wq->mutex);
> > - list_del_rcu(&pwq->pwqs_node);
> > - is_last = list_empty(&wq->pwqs);
> > - mutex_unlock(&wq->mutex);
> > + mutex_lock(&wq->mutex);
> > + list_del_rcu(&pwq->pwqs_node);
> > + is_last = list_empty(&wq->pwqs);
> > + mutex_unlock(&wq->mutex);
> > + }
> >
> > mutex_lock(&wq_pool_mutex);
> > put_unbound_pool(pool);
> > --
> > 2.25.1
> >
With regards,
Pavel Skripkin
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] workqueue: fix UAF in pwq_unbound_release_workfn()
2021-07-15 18:52 ` Pavel Skripkin
@ 2021-07-16 10:45 ` Pavel Skripkin
0 siblings, 0 replies; 5+ messages in thread
From: Pavel Skripkin @ 2021-07-16 10:45 UTC (permalink / raw)
To: Lai Jiangshan; +Cc: Yang Yingliang, LKML, Tejun Heo, Xu Qiang
On Thu, 15 Jul 2021 21:52:19 +0300
Pavel Skripkin <paskripkin@gmail.com> wrote:
> On Thu, 15 Jul 2021 16:08:12 +0800
> Lai Jiangshan <jiangshanlai@gmail.com> wrote:
>
> > On Wed, Jul 14, 2021 at 5:16 PM Yang Yingliang
> > <yangyingliang@huawei.com> wrote:
> >
> > >
> > > Fixes: 2d5f0764b526 ("workqueue: split apply_workqueue_attrs()
> > > into 3 stages") Reported-by: Hulk Robot <hulkci@huawei.com>
> > > Suggested-by: Lai Jiangshan <jiangshanlai@gmail.com>
> > > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> > > ---
> > > v3:
> > > drop the v2 and v1 changes, add check pwq in
> > > pwq_unbound_release_workfn() v2:
> > > also use free_wqattrs_ctx() in workqueue_apply_unbound_cpumask()
> > > ---
> > > kernel/workqueue.c | 20 +++++++++++++-------
> > > 1 file changed, 13 insertions(+), 7 deletions(-)
> > >
> >
> > I'm fine with the code.
> >
> > Reviewed-by: Lai Jiangshan <jiangshanlai@gmail.com>
> >
> > Hello, Pavel
> >
> > Could you have a test again, please?
> >
>
> Hi, Lai!
>
> Sorry for late response.
>
> Yes, I have started local syzkaller instance just now, so I will share
> the results tomorrow.
>
Hi, everyone!
With this patch applied my local syzbot instance didn't hit any of the
reported bugs [1].
Tested-by: Pavel Skripkin <paskripkin@gmail.com>
Thank you!
[1] https://lore.kernel.org/lkml/20210708162417.777bff77@gmail.com/
With regards,
Pavel Skripkin
>
> > Thanks,
> > Lai
> >
> > > diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> > > index 50142fc08902..f148eacda55a 100644
> > > --- a/kernel/workqueue.c
> > > +++ b/kernel/workqueue.c
> > > @@ -3676,15 +3676,21 @@ static void
> > > pwq_unbound_release_workfn(struct work_struct *work)
> > > unbound_release_work); struct workqueue_struct *wq = pwq->wq;
> > > struct worker_pool *pool = pwq->pool;
> > > - bool is_last;
> > > + bool is_last = false;
> > >
> > > - if (WARN_ON_ONCE(!(wq->flags & WQ_UNBOUND)))
> > > - return;
> > > + /*
> > > + * when @pwq is not linked, it doesn't hold any reference
> > > to the
> > > + * @wq, and @wq is invalid to access.
> > > + */
> > > + if (!list_empty(&pwq->pwqs_node)) {
> > > + if (WARN_ON_ONCE(!(wq->flags & WQ_UNBOUND)))
> > > + return;
> > >
> > > - mutex_lock(&wq->mutex);
> > > - list_del_rcu(&pwq->pwqs_node);
> > > - is_last = list_empty(&wq->pwqs);
> > > - mutex_unlock(&wq->mutex);
> > > + mutex_lock(&wq->mutex);
> > > + list_del_rcu(&pwq->pwqs_node);
> > > + is_last = list_empty(&wq->pwqs);
> > > + mutex_unlock(&wq->mutex);
> > > + }
> > >
> > > mutex_lock(&wq_pool_mutex);
> > > put_unbound_pool(pool);
> > > --
> > > 2.25.1
> > >
>
>
> With regards,
> Pavel Skripkin
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] workqueue: fix UAF in pwq_unbound_release_workfn()
2021-07-14 9:19 [PATCH v3] workqueue: fix UAF in pwq_unbound_release_workfn() Yang Yingliang
2021-07-15 8:08 ` Lai Jiangshan
@ 2021-07-16 16:14 ` Tejun Heo
1 sibling, 0 replies; 5+ messages in thread
From: Tejun Heo @ 2021-07-16 16:14 UTC (permalink / raw)
To: Yang Yingliang; +Cc: linux-kernel, jiangshanlai, xuqiang36, paskripkin
On Wed, Jul 14, 2021 at 05:19:33PM +0800, Yang Yingliang wrote:
> If apply_wqattrs_prepare() fails, the new pwq are not linked, it doesn't
> hold any reference to the 'wq', 'wq' is invalid to access in the worker,
> so add check pwq if linked to fix this.
>
> Fixes: 2d5f0764b526 ("workqueue: split apply_workqueue_attrs() into 3 stages")
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Suggested-by: Lai Jiangshan <jiangshanlai@gmail.com>
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
Applied to wq/for-5.14-fixes.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-07-16 16:14 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-14 9:19 [PATCH v3] workqueue: fix UAF in pwq_unbound_release_workfn() Yang Yingliang
2021-07-15 8:08 ` Lai Jiangshan
2021-07-15 18:52 ` Pavel Skripkin
2021-07-16 10:45 ` Pavel Skripkin
2021-07-16 16:14 ` Tejun Heo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).