* Re: [PATCH] padata: do not leak refcount in reorder_work
2025-05-18 17:45 [PATCH] padata: do not leak refcount in reorder_work Dominik Grzegorzek
@ 2025-05-19 1:39 ` Chen Ridong
2025-05-19 6:03 ` Herbert Xu
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Chen Ridong @ 2025-05-19 1:39 UTC (permalink / raw)
To: Dominik Grzegorzek, Steffen Klassert, Daniel Jordan
Cc: Herbert Xu, Chen Ridong,
open list:PADATA PARALLEL EXECUTION MECHANISM,
open list:PADATA PARALLEL EXECUTION MECHANISM
On 2025/5/19 1:45, Dominik Grzegorzek wrote:
> A recent patch that addressed a UAF introduced a reference count leak:
> the parallel_data refcount is incremented unconditionally, regardless
> of the return value of queue_work(). If the work item is already queued,
> the incremented refcount is never decremented.
>
> Fix this by checking the return value of queue_work() and decrementing
> the refcount when necessary.
>
> Resolves:
>
> Unreferenced object 0xffff9d9f421e3d80 (size 192):
> comm "cryptomgr_probe", pid 157, jiffies 4294694003
> hex dump (first 32 bytes):
> 80 8b cf 41 9f 9d ff ff b8 97 e0 89 ff ff ff ff ...A............
> d0 97 e0 89 ff ff ff ff 19 00 00 00 1f 88 23 00 ..............#.
> backtrace (crc 838fb36):
> __kmalloc_cache_noprof+0x284/0x320
> padata_alloc_pd+0x20/0x1e0
> padata_alloc_shell+0x3b/0xa0
> 0xffffffffc040a54d
> cryptomgr_probe+0x43/0xc0
> kthread+0xf6/0x1f0
> ret_from_fork+0x2f/0x50
> ret_from_fork_asm+0x1a/0x30
>
> Fixes: dd7d37ccf6b1 ("padata: avoid UAF for reorder_work")
> Signed-off-by: Dominik Grzegorzek <dominik.grzegorzek@oracle.com>
> ---
> kernel/padata.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/padata.c b/kernel/padata.c
> index b3d4eacc4f5d..7eee94166357 100644
> --- a/kernel/padata.c
> +++ b/kernel/padata.c
> @@ -358,7 +358,8 @@ static void padata_reorder(struct parallel_data *pd)
> * To avoid UAF issue, add pd ref here, and put pd ref after reorder_work finish.
> */
> padata_get_pd(pd);
> - queue_work(pinst->serial_wq, &pd->reorder_work);
> + if (!queue_work(pinst->serial_wq, &pd->reorder_work))
> + padata_put_pd(pd);
> }
> }
>
Thank you. I missed this case.
LGTM
Thanks,
Ridong
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] padata: do not leak refcount in reorder_work
2025-05-18 17:45 [PATCH] padata: do not leak refcount in reorder_work Dominik Grzegorzek
2025-05-19 1:39 ` Chen Ridong
@ 2025-05-19 6:03 ` Herbert Xu
2025-05-20 16:00 ` Daniel Jordan
2025-05-22 13:10 ` Aishwarya
3 siblings, 0 replies; 9+ messages in thread
From: Herbert Xu @ 2025-05-19 6:03 UTC (permalink / raw)
To: Dominik Grzegorzek
Cc: Steffen Klassert, Daniel Jordan, Chen Ridong,
open list:PADATA PARALLEL EXECUTION MECHANISM,
open list:PADATA PARALLEL EXECUTION MECHANISM
On Sun, May 18, 2025 at 07:45:31PM +0200, Dominik Grzegorzek wrote:
> A recent patch that addressed a UAF introduced a reference count leak:
> the parallel_data refcount is incremented unconditionally, regardless
> of the return value of queue_work(). If the work item is already queued,
> the incremented refcount is never decremented.
>
> Fix this by checking the return value of queue_work() and decrementing
> the refcount when necessary.
>
> Resolves:
>
> Unreferenced object 0xffff9d9f421e3d80 (size 192):
> comm "cryptomgr_probe", pid 157, jiffies 4294694003
> hex dump (first 32 bytes):
> 80 8b cf 41 9f 9d ff ff b8 97 e0 89 ff ff ff ff ...A............
> d0 97 e0 89 ff ff ff ff 19 00 00 00 1f 88 23 00 ..............#.
> backtrace (crc 838fb36):
> __kmalloc_cache_noprof+0x284/0x320
> padata_alloc_pd+0x20/0x1e0
> padata_alloc_shell+0x3b/0xa0
> 0xffffffffc040a54d
> cryptomgr_probe+0x43/0xc0
> kthread+0xf6/0x1f0
> ret_from_fork+0x2f/0x50
> ret_from_fork_asm+0x1a/0x30
>
> Fixes: dd7d37ccf6b1 ("padata: avoid UAF for reorder_work")
> Signed-off-by: Dominik Grzegorzek <dominik.grzegorzek@oracle.com>
> ---
> kernel/padata.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
Patch applied. Thanks.
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] padata: do not leak refcount in reorder_work
2025-05-18 17:45 [PATCH] padata: do not leak refcount in reorder_work Dominik Grzegorzek
2025-05-19 1:39 ` Chen Ridong
2025-05-19 6:03 ` Herbert Xu
@ 2025-05-20 16:00 ` Daniel Jordan
2025-05-22 13:10 ` Aishwarya
3 siblings, 0 replies; 9+ messages in thread
From: Daniel Jordan @ 2025-05-20 16:00 UTC (permalink / raw)
To: Dominik Grzegorzek
Cc: Steffen Klassert, Herbert Xu, Chen Ridong,
open list:PADATA PARALLEL EXECUTION MECHANISM,
open list:PADATA PARALLEL EXECUTION MECHANISM
On Sun, May 18, 2025 at 07:45:31PM +0200, Dominik Grzegorzek wrote:
> A recent patch that addressed a UAF introduced a reference count leak:
> the parallel_data refcount is incremented unconditionally, regardless
> of the return value of queue_work(). If the work item is already queued,
> the incremented refcount is never decremented.
>
> Fix this by checking the return value of queue_work() and decrementing
> the refcount when necessary.
>
> Resolves:
>
> Unreferenced object 0xffff9d9f421e3d80 (size 192):
> comm "cryptomgr_probe", pid 157, jiffies 4294694003
> hex dump (first 32 bytes):
> 80 8b cf 41 9f 9d ff ff b8 97 e0 89 ff ff ff ff ...A............
> d0 97 e0 89 ff ff ff ff 19 00 00 00 1f 88 23 00 ..............#.
> backtrace (crc 838fb36):
> __kmalloc_cache_noprof+0x284/0x320
> padata_alloc_pd+0x20/0x1e0
> padata_alloc_shell+0x3b/0xa0
> 0xffffffffc040a54d
> cryptomgr_probe+0x43/0xc0
> kthread+0xf6/0x1f0
> ret_from_fork+0x2f/0x50
> ret_from_fork_asm+0x1a/0x30
>
> Fixes: dd7d37ccf6b1 ("padata: avoid UAF for reorder_work")
> Signed-off-by: Dominik Grzegorzek <dominik.grzegorzek@oracle.com>
I missed it too. Thanks!
Acked-by: Daniel Jordan <daniel.m.jordan@oracle.com>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] padata: do not leak refcount in reorder_work
2025-05-18 17:45 [PATCH] padata: do not leak refcount in reorder_work Dominik Grzegorzek
` (2 preceding siblings ...)
2025-05-20 16:00 ` Daniel Jordan
@ 2025-05-22 13:10 ` Aishwarya
2025-05-22 13:44 ` Dominik Grzegorzek
2025-05-23 3:48 ` Herbert Xu
3 siblings, 2 replies; 9+ messages in thread
From: Aishwarya @ 2025-05-22 13:10 UTC (permalink / raw)
To: dominik.grzegorzek
Cc: chenridong, daniel.m.jordan, herbert, linux-crypto, linux-kernel,
steffen.klassert, broonie
Hi Dominik,
I wanted to report a regression observed while running the
`kselftest-mm` suite, specifically the
`mm_run_vmtests_sh_migration_migration_shared_anon` test, on an
Arm64 Marvell Thunder X2 (TX2) system.
The kernel was built using defconfig with the additional config
fragment from:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/testing/selftests/mm/config
This works fine on v6.15-rc7.
A bisect identified this patch as introducing the failure. Bisected
it on the tag "v6.15-rc7-7-g4a95bc121ccd" at repo:
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
Failure log:
11193 03:29:14.806502 # # running ./migration
11194 03:29:14.806876 # # -------------------
11195 03:29:14.820939 # # TAP version 13
11196 03:29:14.821236 # # 1..6
11197 03:29:14.821519 # # # Starting 6 tests from 1 test cases.
11198 03:29:14.821773 # # # RUN migration.private_anon ...
11199 03:29:34.602964 # # # OK migration.private_anon
11200 03:29:34.603418 # # ok 1 migration.private_anon
11201 03:29:34.603687 # # # RUN migration.shared_anon ...
11202 03:29:34.973479 # # Didn't migrate 1 pages
11203 03:29:34.973855 # # # migration.c:175:shared_anon:Expected migrate(ptr,
self->n1, self->n2) (-2) == 0 (0)
11204 03:29:34.984787 # # # shared_anon: Test terminated by assertion
11205 03:29:34.985105 # # # FAIL migration.shared_anon
11206 03:29:34.985365 # # not ok 2 migration.shared_anon
11207 03:29:34.988568 # # # RUN migration.private_anon_thp ...
11208 03:29:54.597572 # # # OK migration.private_anon_thp
11209 03:29:54.597951 # # ok 3 migration.private_anon_thp
11210 03:29:54.598487 # # # RUN migration.shared_anon_thp ...
11211 03:29:55.011183 # # Didn't migrate 1 pages
11212 03:29:55.011524 # # # migration.c:241:shared_anon_thp:Expected migrate(ptr,
self->n1, self->n2) (-2) == 0 (0)
11213 03:29:55.022519 # # # shared_anon_thp: Test terminated by assertion
11214 03:29:55.022834 # # # FAIL migration.shared_anon_thp
11215 03:29:55.027864 # # not ok 4 migration.shared_anon_thp
11216 03:29:55.028156 # # # RUN migration.private_anon_htlb ...
11217 03:30:14.595327 # # # OK migration.private_anon_htlb
11218 03:30:14.595777 # # ok 5 migration.private_anon_htlb
11219 03:30:14.596398 # # # RUN migration.shared_anon_htlb ...
11220 03:30:34.595239 # # # OK migration.shared_anon_htlb
11221 03:30:34.595623 # # ok 6 migration.shared_anon_htlb
11222 03:30:34.595859 # # # FAILED: 4 / 6 tests passed.
11223 03:30:34.603816 # # # Totals: pass:4 fail:2 xfail:0 xpass:0 skip:0 error:0
11224 03:30:34.604110 # # [FAIL]
11225 03:30:34.604342 # not ok 55 migration # exit=1
Thanks,
Aishwarya
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] padata: do not leak refcount in reorder_work
2025-05-22 13:10 ` Aishwarya
@ 2025-05-22 13:44 ` Dominik Grzegorzek
2025-05-22 14:02 ` Mark Brown
2025-05-23 3:48 ` Herbert Xu
1 sibling, 1 reply; 9+ messages in thread
From: Dominik Grzegorzek @ 2025-05-22 13:44 UTC (permalink / raw)
To: aishwarya.tcv
Cc: chenridong, broonie, Daniel Jordan, herbert, linux-kernel,
linux-crypto, steffen.klassert
On Thu, 2025-05-22 at 14:10 +0100, Aishwarya wrote:
> Hi Dominik,
>
> I wanted to report a regression observed while running the
> `kselftest-mm` suite, specifically the
> `mm_run_vmtests_sh_migration_migration_shared_anon` test, on an
> Arm64 Marvell Thunder X2 (TX2) system.
>
> The kernel was built using defconfig with the additional config
> fragment from:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/
> testing/selftests/mm/config
>
> This works fine on v6.15-rc7.
>
> A bisect identified this patch as introducing the failure. Bisected
> it on the tag "v6.15-rc7-7-g4a95bc121ccd" at repo:
> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
>
> Failure log:
> 11193 03:29:14.806502 # # running ./migration
> 11194 03:29:14.806876 # # -------------------
> 11195 03:29:14.820939 # # TAP version 13
> 11196 03:29:14.821236 # # 1..6
> 11197 03:29:14.821519 # # # Starting 6 tests from 1 test cases.
> 11198 03:29:14.821773 # # # RUN migration.private_anon ...
> 11199 03:29:34.602964 # # # OK migration.private_anon
> 11200 03:29:34.603418 # # ok 1 migration.private_anon
> 11201 03:29:34.603687 # # # RUN migration.shared_anon ...
> 11202 03:29:34.973479 # # Didn't migrate 1 pages
> 11203 03:29:34.973855 # # # migration.c:175:shared_anon:Expected migrate(ptr,
> self->n1, self->n2) (-2) == 0 (0)
> 11204 03:29:34.984787 # # # shared_anon: Test terminated by assertion
> 11205 03:29:34.985105 # # # FAIL migration.shared_anon
> 11206 03:29:34.985365 # # not ok 2 migration.shared_anon
> 11207 03:29:34.988568 # # # RUN migration.private_anon_thp ...
> 11208 03:29:54.597572 # # # OK migration.private_anon_thp
> 11209 03:29:54.597951 # # ok 3 migration.private_anon_thp
> 11210 03:29:54.598487 # # # RUN migration.shared_anon_thp ...
> 11211 03:29:55.011183 # # Didn't migrate 1 pages
> 11212 03:29:55.011524 # # # migration.c:241:shared_anon_thp:Expected
> migrate(ptr,
> self->n1, self->n2) (-2) == 0 (0)
> 11213 03:29:55.022519 # # # shared_anon_thp: Test terminated by assertion
> 11214 03:29:55.022834 # # # FAIL migration.shared_anon_thp
> 11215 03:29:55.027864 # # not ok 4 migration.shared_anon_thp
> 11216 03:29:55.028156 # # # RUN migration.private_anon_htlb ...
> 11217 03:30:14.595327 # # # OK migration.private_anon_htlb
> 11218 03:30:14.595777 # # ok 5 migration.private_anon_htlb
> 11219 03:30:14.596398 # # # RUN migration.shared_anon_htlb ...
> 11220 03:30:34.595239 # # # OK migration.shared_anon_htlb
> 11221 03:30:34.595623 # # ok 6 migration.shared_anon_htlb
> 11222 03:30:34.595859 # # # FAILED: 4 / 6 tests passed.
> 11223 03:30:34.603816 # # # Totals: pass:4 fail:2 xfail:0 xpass:0 skip:0
> error:0
> 11224 03:30:34.604110 # # [FAIL]
> 11225 03:30:34.604342 # not ok 55 migration # exit=1
>
> Thanks,
> Aishwarya
Hi,
Looking at the test, I don't think this is related. The test allocates some
pages and attempts to migrate them between two NUMA nodes. It seems to be a
selftest for memory management code, and I don't see how this patch could affect
its outcome.
Do you see this failure consistently, or is it only happening occasionally?
I'm wondering if it might have passed by chance when testing the real culprit
during the bisect.
Regards,
Dominik
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] padata: do not leak refcount in reorder_work
2025-05-22 13:44 ` Dominik Grzegorzek
@ 2025-05-22 14:02 ` Mark Brown
2025-05-24 6:51 ` Herbert Xu
0 siblings, 1 reply; 9+ messages in thread
From: Mark Brown @ 2025-05-22 14:02 UTC (permalink / raw)
To: Dominik Grzegorzek
Cc: aishwarya.tcv, chenridong, Daniel Jordan, herbert, linux-kernel,
linux-crypto, steffen.klassert
[-- Attachment #1: Type: text/plain, Size: 1246 bytes --]
On Thu, May 22, 2025 at 01:44:29PM +0000, Dominik Grzegorzek wrote:
> On Thu, 2025-05-22 at 14:10 +0100, Aishwarya wrote:
> > self->n1, self->n2) (-2) == 0 (0)
> > 11213 03:29:55.022519 # # # shared_anon_thp: Test terminated by assertion
> > 11214 03:29:55.022834 # # # FAIL migration.shared_anon_thp
> > 11215 03:29:55.027864 # # not ok 4 migration.shared_anon_thp
> > 11216 03:29:55.028156 # # # RUN migration.private_anon_htlb ...
> > 11217 03:30:14.595327 # # # OK migration.private_anon_htlb
> > 11218 03:30:14.595777 # # ok 5 migration.private_anon_htlb
> > 11219 03:30:14.596398 # # # RUN migration.shared_anon_htlb ...
> > 11220 03:30:34.595239 # # # OK migration.shared_anon_htlb
> > 11221 03:30:34.595623 # # ok 6 migration.shared_anon_htlb
> Looking at the test, I don't think this is related. The test allocates some
> pages and attempts to migrate them between two NUMA nodes. It seems to be a
> selftest for memory management code, and I don't see how this patch could affect
> its outcome.
The hugepages code does make use of padata and it's a hugepages test so
there's some potential connection there, definitely not an obvious one
though.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] padata: do not leak refcount in reorder_work
2025-05-22 14:02 ` Mark Brown
@ 2025-05-24 6:51 ` Herbert Xu
0 siblings, 0 replies; 9+ messages in thread
From: Herbert Xu @ 2025-05-24 6:51 UTC (permalink / raw)
To: Mark Brown
Cc: Dominik Grzegorzek, aishwarya.tcv, chenridong, Daniel Jordan,
linux-kernel, linux-crypto, steffen.klassert
On Thu, May 22, 2025 at 03:02:14PM +0100, Mark Brown wrote:
>
> The hugepages code does make use of padata and it's a hugepages test so
> there's some potential connection there, definitely not an obvious one
> though.
I just had a look at this and the hugepage use of padata has
nothing to do with the patch in question at all. In fact, the
two users of padata pretty much live in separate universes. The
only connection between them is the shared set of work structs.
So I think is a false-positive bisection result.
Thanks,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] padata: do not leak refcount in reorder_work
2025-05-22 13:10 ` Aishwarya
2025-05-22 13:44 ` Dominik Grzegorzek
@ 2025-05-23 3:48 ` Herbert Xu
1 sibling, 0 replies; 9+ messages in thread
From: Herbert Xu @ 2025-05-23 3:48 UTC (permalink / raw)
To: Aishwarya
Cc: dominik.grzegorzek, chenridong, daniel.m.jordan, linux-crypto,
linux-kernel, steffen.klassert, broonie
On Thu, May 22, 2025 at 02:10:41PM +0100, Aishwarya wrote:
>
> A bisect identified this patch as introducing the failure. Bisected
> it on the tag "v6.15-rc7-7-g4a95bc121ccd" at repo:
> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
What if you revert the patch in question as well as the one it
was supposed to fix, i.e., commit dd7d37ccf6b1 ("padata: avoid
UAF for reorder_work")? I've attached both reverts together as
a patch.
I think the original fix was broken since the bug is actually
in the Crypto API.
Thanks,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff --git a/kernel/padata.c b/kernel/padata.c
index 7eee94166357..e0af15779d80 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -352,15 +352,8 @@ static void padata_reorder(struct parallel_data *pd)
smp_mb();
reorder = per_cpu_ptr(pd->reorder_list, pd->cpu);
- if (!list_empty(&reorder->list) && padata_find_next(pd, false)) {
- /*
- * Other context(eg. the padata_serial_worker) can finish the request.
- * To avoid UAF issue, add pd ref here, and put pd ref after reorder_work finish.
- */
- padata_get_pd(pd);
- if (!queue_work(pinst->serial_wq, &pd->reorder_work))
- padata_put_pd(pd);
- }
+ if (!list_empty(&reorder->list) && padata_find_next(pd, false))
+ queue_work(pinst->serial_wq, &pd->reorder_work);
}
static void invoke_padata_reorder(struct work_struct *work)
@@ -371,8 +364,6 @@ static void invoke_padata_reorder(struct work_struct *work)
pd = container_of(work, struct parallel_data, reorder_work);
padata_reorder(pd);
local_bh_enable();
- /* Pairs with putting the reorder_work in the serial_wq */
- padata_put_pd(pd);
}
static void padata_serial_worker(struct work_struct *serial_work)
^ permalink raw reply [flat|nested] 9+ messages in thread