[PATCH 1/2] io_uring: fix UAF for personality_idr

[PATCH 1/2] io_uring: fix UAF for personality_idr

[yangerkun]

Loop with follow can trigger a UAF:

void main()
{
        int ret;
        struct io_uring ring;
        struct io_uring_params p;
        int i;

        ret = io_uring_queue_init(1, &ring, 0);
        assert(ret == 0);

        for (i = 0; i < 10000; i++) {
                ret = io_uring_register_personality(&ring);
                if (ret < 0)
                        break;
        }

        ret = io_uring_unregister_personality(&ring, 1024);
        assert(ret == 0);
}

==================================================================
BUG: KASAN: use-after-free in radix_tree_next_slot
include/linux/radix-tree.h:422 [inline]
BUG: KASAN: use-after-free in idr_for_each+0x88/0x18c lib/idr.c:202
Read of size 8 at addr ffff0001096539f8 by task syz-executor.2/3166

CPU: 0 PID: 3166 Comm: syz-executor.2 Not tainted
5.10.0-00843-g352c8610ccd2 #2
Hardware name: linux,dummy-virt (DT)
Call trace:
 dump_backtrace+0x0/0x1d8 arch/arm64/kernel/stacktrace.c:132
 show_stack+0x28/0x34 arch/arm64/kernel/stacktrace.c:196
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x110/0x164 lib/dump_stack.c:118
 print_address_description+0x78/0x5c8 mm/kasan/report.c:385
 __kasan_report mm/kasan/report.c:545 [inline]
 kasan_report+0x148/0x1e4 mm/kasan/report.c:562
 check_memory_region_inline mm/kasan/generic.c:183 [inline]
 __asan_load8+0xb4/0xbc mm/kasan/generic.c:252
 radix_tree_next_slot include/linux/radix-tree.h:422 [inline]
 idr_for_each+0x88/0x18c lib/idr.c:202
 io_ring_ctx_wait_and_kill+0xf0/0x210 fs/io_uring.c:8429
 io_uring_release+0x3c/0x50 fs/io_uring.c:8454
 __fput+0x1b8/0x3a8 fs/file_table.c:281
 ____fput+0x1c/0x28 fs/file_table.c:314
 task_work_run+0xec/0x13c kernel/task_work.c:151
 exit_task_work include/linux/task_work.h:30 [inline]
 do_exit+0x384/0xd68 kernel/exit.c:809
 do_group_exit+0xb8/0x13c kernel/exit.c:906
 get_signal+0x794/0xb04 kernel/signal.c:2758
 do_signal arch/arm64/kernel/signal.c:658 [inline]
 do_notify_resume+0x1dc/0x8a8 arch/arm64/kernel/signal.c:722
 work_pending+0xc/0x180

Allocated by task 3149:
 stack_trace_save+0x80/0xb8 kernel/stacktrace.c:121
 kasan_save_stack mm/kasan/common.c:48 [inline]
 kasan_set_track mm/kasan/common.c:56 [inline]
 __kasan_kmalloc+0xdc/0x120 mm/kasan/common.c:461
 kasan_slab_alloc+0x14/0x1c mm/kasan/common.c:469
 slab_post_alloc_hook+0x50/0x8c mm/slab.h:535
 slab_alloc_node mm/slub.c:2891 [inline]
 slab_alloc mm/slub.c:2899 [inline]
 kmem_cache_alloc+0x1f4/0x2fc mm/slub.c:2904
 radix_tree_node_alloc+0x70/0x19c lib/radix-tree.c:274
 idr_get_free+0x180/0x528 lib/radix-tree.c:1504
 idr_alloc_u32+0xa8/0x164 lib/idr.c:46
 idr_alloc_cyclic+0x8c/0x150 lib/idr.c:125
 io_register_personality fs/io_uring.c:9512 [inline]
 __io_uring_register+0xed8/0x1d9c fs/io_uring.c:9741
 __do_sys_io_uring_register fs/io_uring.c:9791 [inline]
 __se_sys_io_uring_register fs/io_uring.c:9773 [inline]
 __arm64_sys_io_uring_register+0xd0/0x108 fs/io_uring.c:9773
 __invoke_syscall arch/arm64/kernel/syscall.c:36 [inline]
 invoke_syscall arch/arm64/kernel/syscall.c:48 [inline]
 el0_svc_common arch/arm64/kernel/syscall.c:158 [inline]
 do_el0_svc+0x120/0x260 arch/arm64/kernel/syscall.c:227
 el0_svc+0x20/0x2c arch/arm64/kernel/entry-common.c:367
 el0_sync_handler+0x98/0x170 arch/arm64/kernel/entry-common.c:383
 el0_sync+0x140/0x180 arch/arm64/kernel/entry.S:670

Freed by task 4399:
 stack_trace_save+0x80/0xb8 kernel/stacktrace.c:121
 kasan_save_stack mm/kasan/common.c:48 [inline]
 kasan_set_track+0x38/0x6c mm/kasan/common.c:56
 kasan_set_free_info+0x20/0x40 mm/kasan/generic.c:355
 __kasan_slab_free+0x124/0x150 mm/kasan/common.c:422
 kasan_slab_free+0x10/0x1c mm/kasan/common.c:431
 slab_free_hook mm/slub.c:1544 [inline]
 slab_free_freelist_hook+0xb0/0x1ac mm/slub.c:1577
 slab_free mm/slub.c:3142 [inline]
 kmem_cache_free+0xc4/0x268 mm/slub.c:3158
 radix_tree_node_rcu_free+0x60/0x6c lib/radix-tree.c:302
 rcu_do_batch+0x180/0x404 kernel/rcu/tree.c:2479
 rcu_core+0x3e0/0x410 kernel/rcu/tree.c:2714
 rcu_core_si+0xc/0x14 kernel/rcu/tree.c:2727
 __do_softirq+0x180/0x3e0 kernel/softirq.c:298

radix_tree_next_slot called by idr_for_each will traverse all slot
regardless of whether the slot is valid. And once the last valid slot
has been remove, we will try to free the node, and lead to a UAF.

idr_destroy will do what we want. So, just stop call idr_remove in
io_unregister_personality to fix the problem.

Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: yangerkun <yangerkun@huawei.com>
---
 fs/io_uring.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 92c25b5f1349..b462c2bf0f2c 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -8494,9 +8494,9 @@ static int io_unregister_personality(struct io_ring_ctx *ctx, unsigned id)
 
 static int io_remove_personalities(int id, void *p, void *data)
 {
-	struct io_ring_ctx *ctx = data;
+	const struct cred *creds = p;
 
-	io_unregister_personality(ctx, id);
+	put_cred(creds);
 	return 0;
 }
 
-- 
2.25.4

[PATCH 2/2] io_uring: fix UAF for io_buffer_idr

[yangerkun]

The same as personality_idr, stop call idr_remove in idr_for_each.

Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: yangerkun <yangerkun@huawei.com>
---
 fs/io_uring.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index b462c2bf0f2c..7c3011756994 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -3850,8 +3850,8 @@ static int io_remove_buffers_prep(struct io_kiocb *req,
 	return 0;
 }
 
-static int __io_remove_buffers(struct io_ring_ctx *ctx, struct io_buffer *buf,
-			       int bgid, unsigned nbufs)
+static int __io_free_buffers(struct io_ring_ctx *ctx, struct io_buffer *buf,
+			       int bgid, unsigned int nbufs)
 {
 	unsigned i = 0;
 
@@ -3871,11 +3871,16 @@ static int __io_remove_buffers(struct io_ring_ctx *ctx, struct io_buffer *buf,
 	}
 	i++;
 	kfree(buf);
-	idr_remove(&ctx->io_buffer_idr, bgid);
-
 	return i;
 }
 
+static int __io_remove_buffers(struct io_ring_ctx *ctx, struct io_buffer *buf,
+			       int bgid, unsigned int nbufs)
+{
+	idr_remove(&ctx->io_buffer_idr, bgid);
+	return __io_free_buffers(ctx, buf, bgid, nbufs);
+}
+
 static int io_remove_buffers(struct io_kiocb *req, unsigned int issue_flags)
 {
 	struct io_provide_buf *p = &req->pbuf;
@@ -8345,18 +8350,18 @@ static int io_eventfd_unregister(struct io_ring_ctx *ctx)
 	return -ENXIO;
 }
 
-static int __io_destroy_buffers(int id, void *p, void *data)
+static int io_free_buffers(int id, void *p, void *data)
 {
 	struct io_ring_ctx *ctx = data;
 	struct io_buffer *buf = p;
 
-	__io_remove_buffers(ctx, buf, id, -1U);
+	__io_free_buffers(ctx, buf, id, -1U);
 	return 0;
 }
 
 static void io_destroy_buffers(struct io_ring_ctx *ctx)
 {
-	idr_for_each(&ctx->io_buffer_idr, __io_destroy_buffers, ctx);
+	idr_for_each(&ctx->io_buffer_idr, io_free_buffers, ctx);
 	idr_destroy(&ctx->io_buffer_idr);
 }
 
-- 
2.25.4

Re: [PATCH 1/2] io_uring: fix UAF for personality_idr

[yangerkun]

在 2021/3/8 14:59, yangerkun 写道:
> Loop with follow can trigger a UAF:

Add a synchronize_rcu as follow will help to improve the recurrence 
probability since we can ensure the radix_tree_node has been freed :)

@@ -8497,6 +8498,7 @@ static int io_remove_personalities(int id, void 
*p, void *data)
         struct io_ring_ctx *ctx = data;

         io_unregister_personality(ctx, id);
+       synchronize_rcu();
         return 0;
  }



> 
> void main()
> {
>          int ret;
>          struct io_uring ring;
>          struct io_uring_params p;
>          int i;
> 
>          ret = io_uring_queue_init(1, &ring, 0);
>          assert(ret == 0);
> 
>          for (i = 0; i < 10000; i++) {
>                  ret = io_uring_register_personality(&ring);
>                  if (ret < 0)
>                          break;
>          }
> 
>          ret = io_uring_unregister_personality(&ring, 1024);
>          assert(ret == 0);
> }
> 
> ==================================================================
> BUG: KASAN: use-after-free in radix_tree_next_slot
> include/linux/radix-tree.h:422 [inline]
> BUG: KASAN: use-after-free in idr_for_each+0x88/0x18c lib/idr.c:202
> Read of size 8 at addr ffff0001096539f8 by task syz-executor.2/3166
> 
> CPU: 0 PID: 3166 Comm: syz-executor.2 Not tainted
> 5.10.0-00843-g352c8610ccd2 #2
> Hardware name: linux,dummy-virt (DT)
> Call trace:
>   dump_backtrace+0x0/0x1d8 arch/arm64/kernel/stacktrace.c:132
>   show_stack+0x28/0x34 arch/arm64/kernel/stacktrace.c:196
>   __dump_stack lib/dump_stack.c:77 [inline]
>   dump_stack+0x110/0x164 lib/dump_stack.c:118
>   print_address_description+0x78/0x5c8 mm/kasan/report.c:385
>   __kasan_report mm/kasan/report.c:545 [inline]
>   kasan_report+0x148/0x1e4 mm/kasan/report.c:562
>   check_memory_region_inline mm/kasan/generic.c:183 [inline]
>   __asan_load8+0xb4/0xbc mm/kasan/generic.c:252
>   radix_tree_next_slot include/linux/radix-tree.h:422 [inline]
>   idr_for_each+0x88/0x18c lib/idr.c:202
>   io_ring_ctx_wait_and_kill+0xf0/0x210 fs/io_uring.c:8429
>   io_uring_release+0x3c/0x50 fs/io_uring.c:8454
>   __fput+0x1b8/0x3a8 fs/file_table.c:281
>   ____fput+0x1c/0x28 fs/file_table.c:314
>   task_work_run+0xec/0x13c kernel/task_work.c:151
>   exit_task_work include/linux/task_work.h:30 [inline]
>   do_exit+0x384/0xd68 kernel/exit.c:809
>   do_group_exit+0xb8/0x13c kernel/exit.c:906
>   get_signal+0x794/0xb04 kernel/signal.c:2758
>   do_signal arch/arm64/kernel/signal.c:658 [inline]
>   do_notify_resume+0x1dc/0x8a8 arch/arm64/kernel/signal.c:722
>   work_pending+0xc/0x180
> 
> Allocated by task 3149:
>   stack_trace_save+0x80/0xb8 kernel/stacktrace.c:121
>   kasan_save_stack mm/kasan/common.c:48 [inline]
>   kasan_set_track mm/kasan/common.c:56 [inline]
>   __kasan_kmalloc+0xdc/0x120 mm/kasan/common.c:461
>   kasan_slab_alloc+0x14/0x1c mm/kasan/common.c:469
>   slab_post_alloc_hook+0x50/0x8c mm/slab.h:535
>   slab_alloc_node mm/slub.c:2891 [inline]
>   slab_alloc mm/slub.c:2899 [inline]
>   kmem_cache_alloc+0x1f4/0x2fc mm/slub.c:2904
>   radix_tree_node_alloc+0x70/0x19c lib/radix-tree.c:274
>   idr_get_free+0x180/0x528 lib/radix-tree.c:1504
>   idr_alloc_u32+0xa8/0x164 lib/idr.c:46
>   idr_alloc_cyclic+0x8c/0x150 lib/idr.c:125
>   io_register_personality fs/io_uring.c:9512 [inline]
>   __io_uring_register+0xed8/0x1d9c fs/io_uring.c:9741
>   __do_sys_io_uring_register fs/io_uring.c:9791 [inline]
>   __se_sys_io_uring_register fs/io_uring.c:9773 [inline]
>   __arm64_sys_io_uring_register+0xd0/0x108 fs/io_uring.c:9773
>   __invoke_syscall arch/arm64/kernel/syscall.c:36 [inline]
>   invoke_syscall arch/arm64/kernel/syscall.c:48 [inline]
>   el0_svc_common arch/arm64/kernel/syscall.c:158 [inline]
>   do_el0_svc+0x120/0x260 arch/arm64/kernel/syscall.c:227
>   el0_svc+0x20/0x2c arch/arm64/kernel/entry-common.c:367
>   el0_sync_handler+0x98/0x170 arch/arm64/kernel/entry-common.c:383
>   el0_sync+0x140/0x180 arch/arm64/kernel/entry.S:670
> 
> Freed by task 4399:
>   stack_trace_save+0x80/0xb8 kernel/stacktrace.c:121
>   kasan_save_stack mm/kasan/common.c:48 [inline]
>   kasan_set_track+0x38/0x6c mm/kasan/common.c:56
>   kasan_set_free_info+0x20/0x40 mm/kasan/generic.c:355
>   __kasan_slab_free+0x124/0x150 mm/kasan/common.c:422
>   kasan_slab_free+0x10/0x1c mm/kasan/common.c:431
>   slab_free_hook mm/slub.c:1544 [inline]
>   slab_free_freelist_hook+0xb0/0x1ac mm/slub.c:1577
>   slab_free mm/slub.c:3142 [inline]
>   kmem_cache_free+0xc4/0x268 mm/slub.c:3158
>   radix_tree_node_rcu_free+0x60/0x6c lib/radix-tree.c:302
>   rcu_do_batch+0x180/0x404 kernel/rcu/tree.c:2479
>   rcu_core+0x3e0/0x410 kernel/rcu/tree.c:2714
>   rcu_core_si+0xc/0x14 kernel/rcu/tree.c:2727
>   __do_softirq+0x180/0x3e0 kernel/softirq.c:298
> 
> radix_tree_next_slot called by idr_for_each will traverse all slot
> regardless of whether the slot is valid. And once the last valid slot
> has been remove, we will try to free the node, and lead to a UAF.
> 
> idr_destroy will do what we want. So, just stop call idr_remove in
> io_unregister_personality to fix the problem.
> 
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: yangerkun <yangerkun@huawei.com>
> ---
>   fs/io_uring.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 92c25b5f1349..b462c2bf0f2c 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -8494,9 +8494,9 @@ static int io_unregister_personality(struct io_ring_ctx *ctx, unsigned id)
>   
>   static int io_remove_personalities(int id, void *p, void *data)
>   {
> -	struct io_ring_ctx *ctx = data;
> +	const struct cred *creds = p;
>   
> -	io_unregister_personality(ctx, id);
> +	put_cred(creds);
>   	return 0;
>   }
>   
> 

Re: [PATCH 1/2] io_uring: fix UAF for personality_idr

[Pavel Begunkov]

On 08/03/2021 06:59, yangerkun wrote:
> Loop with follow can trigger a UAF:
> 
> void main()
> {
>         int ret;
>         struct io_uring ring;
>         struct io_uring_params p;
>         int i;
> 
>         ret = io_uring_queue_init(1, &ring, 0);
>         assert(ret == 0);
> 
>         for (i = 0; i < 10000; i++) {
>                 ret = io_uring_register_personality(&ring);
>                 if (ret < 0)
>                         break;
>         }
> 
>         ret = io_uring_unregister_personality(&ring, 1024);
>         assert(ret == 0);
> }

Matthew, any chance you remember whether idr_for_each tolerates
idr_remove() from within the callback? Nothing else is happening in
parallel.



> ==================================================================
> BUG: KASAN: use-after-free in radix_tree_next_slot
> include/linux/radix-tree.h:422 [inline]
> BUG: KASAN: use-after-free in idr_for_each+0x88/0x18c lib/idr.c:202
> Read of size 8 at addr ffff0001096539f8 by task syz-executor.2/3166
> 
> CPU: 0 PID: 3166 Comm: syz-executor.2 Not tainted
> 5.10.0-00843-g352c8610ccd2 #2
> Hardware name: linux,dummy-virt (DT)
> Call trace:
>  dump_backtrace+0x0/0x1d8 arch/arm64/kernel/stacktrace.c:132
>  show_stack+0x28/0x34 arch/arm64/kernel/stacktrace.c:196
>  __dump_stack lib/dump_stack.c:77 [inline]
>  dump_stack+0x110/0x164 lib/dump_stack.c:118
>  print_address_description+0x78/0x5c8 mm/kasan/report.c:385
>  __kasan_report mm/kasan/report.c:545 [inline]
>  kasan_report+0x148/0x1e4 mm/kasan/report.c:562
>  check_memory_region_inline mm/kasan/generic.c:183 [inline]
>  __asan_load8+0xb4/0xbc mm/kasan/generic.c:252
>  radix_tree_next_slot include/linux/radix-tree.h:422 [inline]
>  idr_for_each+0x88/0x18c lib/idr.c:202
>  io_ring_ctx_wait_and_kill+0xf0/0x210 fs/io_uring.c:8429
>  io_uring_release+0x3c/0x50 fs/io_uring.c:8454
>  __fput+0x1b8/0x3a8 fs/file_table.c:281
>  ____fput+0x1c/0x28 fs/file_table.c:314
>  task_work_run+0xec/0x13c kernel/task_work.c:151
>  exit_task_work include/linux/task_work.h:30 [inline]
>  do_exit+0x384/0xd68 kernel/exit.c:809
>  do_group_exit+0xb8/0x13c kernel/exit.c:906
>  get_signal+0x794/0xb04 kernel/signal.c:2758
>  do_signal arch/arm64/kernel/signal.c:658 [inline]
>  do_notify_resume+0x1dc/0x8a8 arch/arm64/kernel/signal.c:722
>  work_pending+0xc/0x180
> 
> Allocated by task 3149:
>  stack_trace_save+0x80/0xb8 kernel/stacktrace.c:121
>  kasan_save_stack mm/kasan/common.c:48 [inline]
>  kasan_set_track mm/kasan/common.c:56 [inline]
>  __kasan_kmalloc+0xdc/0x120 mm/kasan/common.c:461
>  kasan_slab_alloc+0x14/0x1c mm/kasan/common.c:469
>  slab_post_alloc_hook+0x50/0x8c mm/slab.h:535
>  slab_alloc_node mm/slub.c:2891 [inline]
>  slab_alloc mm/slub.c:2899 [inline]
>  kmem_cache_alloc+0x1f4/0x2fc mm/slub.c:2904
>  radix_tree_node_alloc+0x70/0x19c lib/radix-tree.c:274
>  idr_get_free+0x180/0x528 lib/radix-tree.c:1504
>  idr_alloc_u32+0xa8/0x164 lib/idr.c:46
>  idr_alloc_cyclic+0x8c/0x150 lib/idr.c:125
>  io_register_personality fs/io_uring.c:9512 [inline]
>  __io_uring_register+0xed8/0x1d9c fs/io_uring.c:9741
>  __do_sys_io_uring_register fs/io_uring.c:9791 [inline]
>  __se_sys_io_uring_register fs/io_uring.c:9773 [inline]
>  __arm64_sys_io_uring_register+0xd0/0x108 fs/io_uring.c:9773
>  __invoke_syscall arch/arm64/kernel/syscall.c:36 [inline]
>  invoke_syscall arch/arm64/kernel/syscall.c:48 [inline]
>  el0_svc_common arch/arm64/kernel/syscall.c:158 [inline]
>  do_el0_svc+0x120/0x260 arch/arm64/kernel/syscall.c:227
>  el0_svc+0x20/0x2c arch/arm64/kernel/entry-common.c:367
>  el0_sync_handler+0x98/0x170 arch/arm64/kernel/entry-common.c:383
>  el0_sync+0x140/0x180 arch/arm64/kernel/entry.S:670
> 
> Freed by task 4399:
>  stack_trace_save+0x80/0xb8 kernel/stacktrace.c:121
>  kasan_save_stack mm/kasan/common.c:48 [inline]
>  kasan_set_track+0x38/0x6c mm/kasan/common.c:56
>  kasan_set_free_info+0x20/0x40 mm/kasan/generic.c:355
>  __kasan_slab_free+0x124/0x150 mm/kasan/common.c:422
>  kasan_slab_free+0x10/0x1c mm/kasan/common.c:431
>  slab_free_hook mm/slub.c:1544 [inline]
>  slab_free_freelist_hook+0xb0/0x1ac mm/slub.c:1577
>  slab_free mm/slub.c:3142 [inline]
>  kmem_cache_free+0xc4/0x268 mm/slub.c:3158
>  radix_tree_node_rcu_free+0x60/0x6c lib/radix-tree.c:302
>  rcu_do_batch+0x180/0x404 kernel/rcu/tree.c:2479
>  rcu_core+0x3e0/0x410 kernel/rcu/tree.c:2714
>  rcu_core_si+0xc/0x14 kernel/rcu/tree.c:2727
>  __do_softirq+0x180/0x3e0 kernel/softirq.c:298
> 
> radix_tree_next_slot called by idr_for_each will traverse all slot
> regardless of whether the slot is valid. And once the last valid slot
> has been remove, we will try to free the node, and lead to a UAF.
> 
> idr_destroy will do what we want. So, just stop call idr_remove in
> io_unregister_personality to fix the problem.
> 
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: yangerkun <yangerkun@huawei.com>
> ---
>  fs/io_uring.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 92c25b5f1349..b462c2bf0f2c 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -8494,9 +8494,9 @@ static int io_unregister_personality(struct io_ring_ctx *ctx, unsigned id)
>  
>  static int io_remove_personalities(int id, void *p, void *data)
>  {
> -	struct io_ring_ctx *ctx = data;
> +	const struct cred *creds = p;
>  
> -	io_unregister_personality(ctx, id);
> +	put_cred(creds);
>  	return 0;
>  }
>  
> 

-- 
Pavel Begunkov

Re: [PATCH 1/2] io_uring: fix UAF for personality_idr

[Matthew Wilcox]

On Mon, Mar 08, 2021 at 10:46:37AM +0000, Pavel Begunkov wrote:
> Matthew, any chance you remember whether idr_for_each tolerates
> idr_remove() from within the callback? Nothing else is happening in
> parallel.

No, that's not allowed.  The design of the IDR is that you would free
the thing being pointed to and then call idr_destroy() afterwards to
free the IDR's data structures.  But this should use an XArray anyway.
Compile-tested only.

PS, I found this commit:

commit 41726c9a50e7464beca7112d0aebf3a0090c62d2
Author: Jens Axboe <axboe@kernel.dk>
Date:   Sun Feb 23 13:11:42 2020 -0700

    io_uring: fix personality idr leak
    
    We somehow never free the idr, even though we init it for every ctx.
    Free it when the rest of the ring data is freed.

The IDR hasn't needed to be freed since I reimplemented it on top of
the radix tree in 2016.  The same is true for the XArray, which is
why idr_destroy() is simply deleted below instead of replaced with
xa_destroy().

From 8b0b4d331bdd9861eaac7322eba7a2669f18be80 Mon Sep 17 00:00:00 2001
From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
Date: Mon, 8 Mar 2021 08:04:38 -0500
Subject: [PATCH] io_uring: Convert personality_idr to XArray

You can't call idr_remove() from within a idr_for_each() callback,
but you can call xa_erase() from an xa_for_each() loop, so switch the
entire personality_idr from the IDR to the XArray.  This manifests as a
use-after-free as idr_for_each() attempts to walk the rest of the node
after removing the last entry from it.

Fixes: 071698e13ac6 ("io_uring: allow registering credentials")
Reported-by: Pavel Begunkov <asml.silence@gmail.com>
Reported-by: yangerkun <yangerkun@huawei.com>
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/io_uring.c | 47 ++++++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 92c25b5f1349..72355903daa1 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -408,7 +408,8 @@ struct io_ring_ctx {
 
 	struct idr		io_buffer_idr;
 
-	struct idr		personality_idr;
+	struct xarray		personalities;
+	u32			pers_next;
 
 	struct {
 		unsigned		cached_cq_tail;
@@ -1131,7 +1132,7 @@ static struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
 	init_completion(&ctx->ref_comp);
 	init_completion(&ctx->sq_thread_comp);
 	idr_init(&ctx->io_buffer_idr);
-	idr_init(&ctx->personality_idr);
+	xa_init_flags(&ctx->personalities, XA_FLAGS_ALLOC1);
 	mutex_init(&ctx->uring_lock);
 	init_waitqueue_head(&ctx->wait);
 	spin_lock_init(&ctx->completion_lock);
@@ -5921,7 +5922,7 @@ static int io_issue_sqe(struct io_kiocb *req, unsigned int issue_flags)
 
 		if (!(issue_flags & IO_URING_F_NONBLOCK))
 			mutex_lock(&ctx->uring_lock);
-		new_creds = idr_find(&ctx->personality_idr, req->work.personality);
+		new_creds = xa_load(&ctx->personalities, req->work.personality);
 		if (!(issue_flags & IO_URING_F_NONBLOCK))
 			mutex_unlock(&ctx->uring_lock);
 		if (!new_creds)
@@ -8418,7 +8419,6 @@ static void io_ring_ctx_free(struct io_ring_ctx *ctx)
 	mutex_unlock(&ctx->uring_lock);
 	io_eventfd_unregister(ctx);
 	io_destroy_buffers(ctx);
-	idr_destroy(&ctx->personality_idr);
 
 #if defined(CONFIG_UNIX)
 	if (ctx->ring_sock) {
@@ -8483,7 +8483,7 @@ static int io_unregister_personality(struct io_ring_ctx *ctx, unsigned id)
 {
 	const struct cred *creds;
 
-	creds = idr_remove(&ctx->personality_idr, id);
+	creds = xa_erase(&ctx->personalities, id);
 	if (creds) {
 		put_cred(creds);
 		return 0;
@@ -8492,14 +8492,6 @@ static int io_unregister_personality(struct io_ring_ctx *ctx, unsigned id)
 	return -EINVAL;
 }
 
-static int io_remove_personalities(int id, void *p, void *data)
-{
-	struct io_ring_ctx *ctx = data;
-
-	io_unregister_personality(ctx, id);
-	return 0;
-}
-
 static bool io_run_ctx_fallback(struct io_ring_ctx *ctx)
 {
 	struct callback_head *work, *next;
@@ -8541,13 +8533,17 @@ static void io_ring_exit_work(struct work_struct *work)
 
 static void io_ring_ctx_wait_and_kill(struct io_ring_ctx *ctx)
 {
+	unsigned long index;
+	struct creds *creds;
+
 	mutex_lock(&ctx->uring_lock);
 	percpu_ref_kill(&ctx->refs);
 	/* if force is set, the ring is going away. always drop after that */
 	ctx->cq_overflow_flushed = 1;
 	if (ctx->rings)
 		__io_cqring_overflow_flush(ctx, true, NULL, NULL);
-	idr_for_each(&ctx->personality_idr, io_remove_personalities, ctx);
+	xa_for_each(&ctx->personalities, index, creds)
+		io_unregister_personality(ctx, index);
 	mutex_unlock(&ctx->uring_lock);
 
 	io_kill_timeouts(ctx, NULL, NULL);
@@ -9127,10 +9123,9 @@ SYSCALL_DEFINE6(io_uring_enter, unsigned int, fd, u32, to_submit,
 }
 
 #ifdef CONFIG_PROC_FS
-static int io_uring_show_cred(int id, void *p, void *data)
+static int io_uring_show_cred(struct seq_file *m, unsigned int id,
+		const struct cred *cred)
 {
-	const struct cred *cred = p;
-	struct seq_file *m = data;
 	struct user_namespace *uns = seq_user_ns(m);
 	struct group_info *gi;
 	kernel_cap_t cap;
@@ -9198,9 +9193,13 @@ static void __io_uring_show_fdinfo(struct io_ring_ctx *ctx, struct seq_file *m)
 		seq_printf(m, "%5u: 0x%llx/%u\n", i, buf->ubuf,
 						(unsigned int) buf->len);
 	}
-	if (has_lock && !idr_is_empty(&ctx->personality_idr)) {
+	if (has_lock && !xa_empty(&ctx->personalities)) {
+		unsigned long index;
+		const struct cred *cred;
+
 		seq_printf(m, "Personalities:\n");
-		idr_for_each(&ctx->personality_idr, io_uring_show_cred, m);
+		xa_for_each(&ctx->personalities, index, cred)
+			io_uring_show_cred(m, index, cred);
 	}
 	seq_printf(m, "PollList:\n");
 	spin_lock_irq(&ctx->completion_lock);
@@ -9532,14 +9531,16 @@ static int io_probe(struct io_ring_ctx *ctx, void __user *arg, unsigned nr_args)
 static int io_register_personality(struct io_ring_ctx *ctx)
 {
 	const struct cred *creds;
+	u32 id;
 	int ret;
 
 	creds = get_current_cred();
 
-	ret = idr_alloc_cyclic(&ctx->personality_idr, (void *) creds, 1,
-				USHRT_MAX, GFP_KERNEL);
-	if (ret < 0)
-		put_cred(creds);
+	ret = xa_alloc_cyclic(&ctx->personalities, &id, (void *)creds,
+			XA_LIMIT(0, USHRT_MAX), &ctx->pers_next, GFP_KERNEL);
+	if (!ret)
+		return id;
+	put_cred(creds);
 	return ret;
 }
 
-- 
2.30.0

Re: [PATCH 1/2] io_uring: fix UAF for personality_idr

[Pavel Begunkov]

On 08/03/2021 13:20, Matthew Wilcox wrote:
> On Mon, Mar 08, 2021 at 10:46:37AM +0000, Pavel Begunkov wrote:
>> Matthew, any chance you remember whether idr_for_each tolerates
>> idr_remove() from within the callback? Nothing else is happening in
>> parallel.
> 
> No, that's not allowed.  The design of the IDR is that you would free
> the thing being pointed to and then call idr_destroy() afterwards to

Gotcha, thanks!

> free the IDR's data structures.  But this should use an XArray anyway.
> Compile-tested only.

Yeah, I remember this patch, looks good but 1 comments below.
Anyway, I'll rebase and resend it shortly for convenience.

[...]
> @@ -9532,14 +9531,16 @@ static int io_probe(struct io_ring_ctx *ctx, void __user *arg, unsigned nr_args)
>  static int io_register_personality(struct io_ring_ctx *ctx)
>  {
>  	const struct cred *creds;
> +	u32 id;
>  	int ret;
>  
>  	creds = get_current_cred();
>  
> -	ret = idr_alloc_cyclic(&ctx->personality_idr, (void *) creds, 1,
> -				USHRT_MAX, GFP_KERNEL);
> -	if (ret < 0)
> -		put_cred(creds);
> +	ret = xa_alloc_cyclic(&ctx->personalities, &id, (void *)creds,
> +			XA_LIMIT(0, USHRT_MAX), &ctx->pers_next, GFP_KERNEL);

ids are >=1, because 0 is kind of a reserved value for io_uring, so I guess

XA_LIMIT(1, USHRT_MAX)

> +	if (!ret)
> +		return id;
> +	put_cred(creds);
>  	return ret;
>  }
>  
> 

-- 
Pavel Begunkov

Re: [PATCH 1/2] io_uring: fix UAF for personality_idr

[Stefan Metzmacher]

Hi Matthew,

> -	ret = idr_alloc_cyclic(&ctx->personality_idr, (void *) creds, 1,
> -				USHRT_MAX, GFP_KERNEL);
> -	if (ret < 0)
> -		put_cred(creds);
> +	ret = xa_alloc_cyclic(&ctx->personalities, &id, (void *)creds,
> +			XA_LIMIT(0, USHRT_MAX), &ctx->pers_next, GFP_KERNEL);
> +	if (!ret)
> +		return id;
> +	put_cred(creds);
>  	return ret;

I guess this should be XA_LIMIT(1, USHRT_MAX) instead?
'0' should not be a valid id.

metze

Re: [PATCH 1/2] io_uring: fix UAF for personality_idr

[Matthew Wilcox]

On Mon, Mar 08, 2021 at 01:54:14PM +0000, Pavel Begunkov wrote:
> > +	ret = xa_alloc_cyclic(&ctx->personalities, &id, (void *)creds,
> > +			XA_LIMIT(0, USHRT_MAX), &ctx->pers_next, GFP_KERNEL);
> 
> ids are >=1, because 0 is kind of a reserved value for io_uring, so I guess
> 
> XA_LIMIT(1, USHRT_MAX)

+       xa_init_flags(&ctx->personalities, XA_FLAGS_ALLOC1);

takes care of not being able to allocate ID 0.