[PATCH 1/1] tty: n_gsm: Fix use-after-free in gsm_cleanup_mux

[PATCH 1/1] tty: n_gsm: Fix use-after-free in gsm_cleanup_mux

[Longlong Xia]

BUG: KASAN: slab-use-after-free in gsm_cleanup_mux+0x7e5/0x820 [n_gsm]
Read of size 8 at addr ffff88814941c700 by task poc/3395

CPU: 0 UID: 0 PID: 3395 Comm: poc Not tainted 6.11.0+ #46
Hardware name: VMware, Inc. VMware Virtual Platform/440BX
Desktop Reference Platform, BIOS 6.00 11/12/2020
Call Trace:
 <TASK>
 dump_stack_lvl+0x6c/0x90
 print_report+0xce/0x610
 kasan_complete_mode_report_info+0x5d/0x1e0
 gsm_cleanup_mux+0x7e5/0x820 [n_gsm]
 kasan_report+0xbd/0xf0
 gsm_cleanup_mux+0x7e5/0x820 [n_gsm]
 __asan_report_load8_noabort+0x14/0x20
 gsm_cleanup_mux+0x7e5/0x820 [n_gsm]
 __pfx_gsm_cleanup_mux+0x10/0x10 [n_gsm]
 __rseq_handle_notify_resume+0x188/0xc50
 __kasan_check_write+0x14/0x20
 gsmld_ioctl+0x3c3/0x15b0 [n_gsm]
 __kasan_check_write+0x14/0x20
 __pfx_gsmld_ioctl+0x10/0x10 [n_gsm]
 do_syscall_64+0x88/0x160
 __kasan_check_write+0x14/0x20
 ldsem_down_read+0x94/0x4e0
 __pfx_ldsem_down_read+0x10/0x10
 __pfx___rseq_handle_notify_resume+0x10/0x10
 switch_fpu_return+0xed/0x200
 tty_ioctl+0x660/0x1260
 __pfx___handle_mm_fault+0x10/0x10
 __pfx_tty_ioctl+0x10/0x10
 __count_memcg_events+0xf5/0x3d0
 fdget+0x2de/0x4f0
 __x64_sys_ioctl+0x132/0x1b0
 x64_sys_call+0x1205/0x20d0
 do_syscall_64+0x7c/0x160
 clear_bhb_loop+0x15/0x70
 entry_SYSCALL_64_after_hwframe+0x76/0x7e

Allocated by task 808:
 kasan_save_stack+0x28/0x50
 kasan_save_track+0x14/0x30
 kasan_save_alloc_info+0x36/0x40
 __kasan_kmalloc+0xb1/0xc0
 __kmalloc_noprof+0x1f6/0x4b0
 gsm_data_alloc.constprop.0+0x2e/0x1a0 [n_gsm]
 gsm_send+0x2f/0x5d0 [n_gsm]
 gsm_queue+0x522/0x730 [n_gsm]
 gsm1_receive+0x58b/0xb70 [n_gsm]
 gsmld_receive_buf+0x173/0x2a0 [n_gsm]
 tty_ldisc_receive_buf+0x115/0x1e0
 tty_port_default_receive_buf+0x66/0xa0
 flush_to_ldisc+0x1b0/0x7c0
 process_scheduled_works+0x2bc/0x10c0
 worker_thread+0x3d4/0x970
 kthread+0x2b6/0x390
 ret_from_fork+0x39/0x80
 ret_from_fork_asm+0x1a/0x30

Freed by task 3377:
 kasan_save_stack+0x28/0x50
 kasan_save_track+0x14/0x30
 kasan_save_free_info+0x3a/0x50
 __kasan_slab_free+0x54/0x70
 kfree+0x126/0x420
 gsm_cleanup_mux+0x3ae/0x820 [n_gsm]
 gsmld_ioctl+0x3c3/0x15b0 [n_gsm]
 tty_ioctl+0x660/0x1260
 __x64_sys_ioctl+0x132/0x1b0
 x64_sys_call+0x1205/0x20d0
 do_syscall_64+0x7c/0x160
 entry_SYSCALL_64_after_hwframe+0x76/0x7e

[Analysis]
gsm_msg on the tx_ctrl_list or tx_data_list of gsm_mux
can be freed by multi threads through ioctl,which leads
to the occurrence of uaf. Protect it by gsm tx lock.

Signed-off-by: Longlong Xia <xialonglong@kylinos.cn>
---
 drivers/tty/n_gsm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 5d37a0984916..1ed68a6aba4e 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -3125,6 +3125,7 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm, bool disc)
 	int i;
 	struct gsm_dlci *dlci;
 	struct gsm_msg *txq, *ntxq;
+	unsigned long flags;
 
 	gsm->dead = true;
 	mutex_lock(&gsm->mutex);
@@ -3157,12 +3158,15 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm, bool disc)
 	mutex_unlock(&gsm->mutex);
 	/* Now wipe the queues */
 	tty_ldisc_flush(gsm->tty);
+
+	spin_lock_irqsave(&gsm->tx_lock, flags);
 	list_for_each_entry_safe(txq, ntxq, &gsm->tx_ctrl_list, list)
 		kfree(txq);
 	INIT_LIST_HEAD(&gsm->tx_ctrl_list);
 	list_for_each_entry_safe(txq, ntxq, &gsm->tx_data_list, list)
 		kfree(txq);
 	INIT_LIST_HEAD(&gsm->tx_data_list);
+	spin_unlock_irqrestore(&gsm->tx_lock, flags);
 }
 
 /**
-- 
2.45.2

Re: [PATCH 1/1] tty: n_gsm: Fix use-after-free in gsm_cleanup_mux

[Jiri Slaby]

Cc Daniel.

On 24. 09. 24, 11:35, Longlong Xia wrote:
> BUG: KASAN: slab-use-after-free in gsm_cleanup_mux+0x7e5/0x820 [n_gsm]
> Read of size 8 at addr ffff88814941c700 by task poc/3395
> 
> CPU: 0 UID: 0 PID: 3395 Comm: poc Not tainted 6.11.0+ #46
> Hardware name: VMware, Inc. VMware Virtual Platform/440BX
> Desktop Reference Platform, BIOS 6.00 11/12/2020
> Call Trace:
>   <TASK>
>   dump_stack_lvl+0x6c/0x90
>   print_report+0xce/0x610
>   kasan_complete_mode_report_info+0x5d/0x1e0
>   gsm_cleanup_mux+0x7e5/0x820 [n_gsm]
>   kasan_report+0xbd/0xf0
>   gsm_cleanup_mux+0x7e5/0x820 [n_gsm]
>   __asan_report_load8_noabort+0x14/0x20
>   gsm_cleanup_mux+0x7e5/0x820 [n_gsm]
>   __pfx_gsm_cleanup_mux+0x10/0x10 [n_gsm]
>   __rseq_handle_notify_resume+0x188/0xc50
>   __kasan_check_write+0x14/0x20
>   gsmld_ioctl+0x3c3/0x15b0 [n_gsm]
>   __kasan_check_write+0x14/0x20
>   __pfx_gsmld_ioctl+0x10/0x10 [n_gsm]
>   do_syscall_64+0x88/0x160
>   __kasan_check_write+0x14/0x20
>   ldsem_down_read+0x94/0x4e0
>   __pfx_ldsem_down_read+0x10/0x10
>   __pfx___rseq_handle_notify_resume+0x10/0x10
>   switch_fpu_return+0xed/0x200
>   tty_ioctl+0x660/0x1260

Could you decode the above to line numbers using 
./scripts/decode_stacktrace.sh?

And then trim the unnecessary entries like:

>   __pfx___handle_mm_fault+0x10/0x10
>   __pfx_tty_ioctl+0x10/0x10
>   __count_memcg_events+0xf5/0x3d0
>   fdget+0x2de/0x4f0
>   __x64_sys_ioctl+0x132/0x1b0
>   x64_sys_call+0x1205/0x20d0
>   do_syscall_64+0x7c/0x160
>   clear_bhb_loop+0x15/0x70
>   entry_SYSCALL_64_after_hwframe+0x76/0x7e

up to here.

BTW do you use ORC or is this with unreliable FRAME_POINTERs? I am 
asking because the stack traces contain a full load of ballast. Like 
do_syscall_64() after tty_ioctl().

> Allocated by task 808:

Also drop this:
>   kasan_save_stack+0x28/0x50
>   kasan_save_track+0x14/0x30
>   kasan_save_alloc_info+0x36/0x40
>   __kasan_kmalloc+0xb1/0xc0
>   __kmalloc_noprof+0x1f6/0x4b0

up to here ^^^.

>   gsm_data_alloc.constprop.0+0x2e/0x1a0 [n_gsm]
>   gsm_send+0x2f/0x5d0 [n_gsm]
>   gsm_queue+0x522/0x730 [n_gsm]
>   gsm1_receive+0x58b/0xb70 [n_gsm]
>   gsmld_receive_buf+0x173/0x2a0 [n_gsm]
>   tty_ldisc_receive_buf+0x115/0x1e0
>   tty_port_default_receive_buf+0x66/0xa0
>   flush_to_ldisc+0x1b0/0x7c0
>   process_scheduled_works+0x2bc/0x10c0
>   worker_thread+0x3d4/0x970
>   kthread+0x2b6/0x390
>   ret_from_fork+0x39/0x80
>   ret_from_fork_asm+0x1a/0x30
> 
> Freed by task 3377:

And here:

>   kasan_save_stack+0x28/0x50
>   kasan_save_track+0x14/0x30
>   kasan_save_free_info+0x3a/0x50
>   __kasan_slab_free+0x54/0x70

^^^

>   kfree+0x126/0x420
>   gsm_cleanup_mux+0x3ae/0x820 [n_gsm]
>   gsmld_ioctl+0x3c3/0x15b0 [n_gsm]
>   tty_ioctl+0x660/0x1260

this:

>   __x64_sys_ioctl+0x132/0x1b0
>   x64_sys_call+0x1205/0x20d0
>   do_syscall_64+0x7c/0x160
>   entry_SYSCALL_64_after_hwframe+0x76/0x7e

^^^

> [Analysis]
> gsm_msg on the tx_ctrl_list or tx_data_list of gsm_mux
> can be freed by multi threads through ioctl,which leads
> to the occurrence of uaf. Protect it by gsm tx lock.

LGTM. But Daniel might have a different opinion...

> Signed-off-by: Longlong Xia <xialonglong@kylinos.cn>
> ---
>   drivers/tty/n_gsm.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 5d37a0984916..1ed68a6aba4e 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -3125,6 +3125,7 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm, bool disc)
>   	int i;
>   	struct gsm_dlci *dlci;
>   	struct gsm_msg *txq, *ntxq;
> +	unsigned long flags;
>   
>   	gsm->dead = true;
>   	mutex_lock(&gsm->mutex);
> @@ -3157,12 +3158,15 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm, bool disc)
>   	mutex_unlock(&gsm->mutex);
>   	/* Now wipe the queues */
>   	tty_ldisc_flush(gsm->tty);
> +
> +	spin_lock_irqsave(&gsm->tx_lock, flags);

Perhaps use guard(spinlock_irqsave) instead?

>   	list_for_each_entry_safe(txq, ntxq, &gsm->tx_ctrl_list, list)
>   		kfree(txq);
>   	INIT_LIST_HEAD(&gsm->tx_ctrl_list);
>   	list_for_each_entry_safe(txq, ntxq, &gsm->tx_data_list, list)
>   		kfree(txq);
>   	INIT_LIST_HEAD(&gsm->tx_data_list);
> +	spin_unlock_irqrestore(&gsm->tx_lock, flags);
>   }
>   
>   /**

-- 
js
suse labs

Re: [PATCH 1/1] tty: n_gsm: Fix use-after-free in gsm_cleanup_mux

[Starke, Daniel]

> > [Analysis]
> > gsm_msg on the tx_ctrl_list or tx_data_list of gsm_mux
> > can be freed by multi threads through ioctl,which leads
> > to the occurrence of uaf. Protect it by gsm tx lock.
> 
> LGTM. But Daniel might have a different opinion...

Looks also good to me. I did not test it, but this will most likely solve
one of the many multi-threading issues in the n_gsm driver.

Best regards,
Daniel Starke

[PATCH v2 1/1] tty: n_gsm: Fix use-after-free in gsm_cleanup_mux

[Longlong Xia]

BUG: KASAN: slab-use-after-free in gsm_cleanup_mux+0x77b/0x7b0
drivers/tty/n_gsm.c:3160 [n_gsm]
Read of size 8 at addr ffff88815fe99c00 by task poc/3379
CPU: 0 UID: 0 PID: 3379 Comm: poc Not tainted 6.11.0+ #56
Hardware name: VMware, Inc. VMware Virtual Platform/440BX
Desktop Reference Platform, BIOS 6.00 11/12/2020
Call Trace:
 <TASK>
 gsm_cleanup_mux+0x77b/0x7b0 drivers/tty/n_gsm.c:3160 [n_gsm]
 __pfx_gsm_cleanup_mux+0x10/0x10 drivers/tty/n_gsm.c:3124 [n_gsm]
 __pfx_sched_clock_cpu+0x10/0x10 kernel/sched/clock.c:389
 update_load_avg+0x1c1/0x27b0 kernel/sched/fair.c:4500
 __pfx_min_vruntime_cb_rotate+0x10/0x10 kernel/sched/fair.c:846
 __rb_insert_augmented+0x492/0xbf0 lib/rbtree.c:161
 gsmld_ioctl+0x395/0x1450 drivers/tty/n_gsm.c:3408 [n_gsm]
 _raw_spin_lock_irqsave+0x92/0xf0 arch/x86/include/asm/atomic.h:107
 __pfx_gsmld_ioctl+0x10/0x10 drivers/tty/n_gsm.c:3822 [n_gsm]
 ktime_get+0x5e/0x140 kernel/time/timekeeping.c:195
 ldsem_down_read+0x94/0x4e0 arch/x86/include/asm/atomic64_64.h:79
 __pfx_ldsem_down_read+0x10/0x10 drivers/tty/tty_ldsem.c:338
 __pfx_do_vfs_ioctl+0x10/0x10 fs/ioctl.c:805
 tty_ioctl+0x643/0x1100 drivers/tty/tty_io.c:2818

Allocated by task 65:
 gsm_data_alloc.constprop.0+0x27/0x190 drivers/tty/n_gsm.c:926 [n_gsm]
 gsm_send+0x2c/0x580 drivers/tty/n_gsm.c:819 [n_gsm]
 gsm1_receive+0x547/0xad0 drivers/tty/n_gsm.c:3038 [n_gsm]
 gsmld_receive_buf+0x176/0x280 drivers/tty/n_gsm.c:3609 [n_gsm]
 tty_ldisc_receive_buf+0x101/0x1e0 drivers/tty/tty_buffer.c:391
 tty_port_default_receive_buf+0x61/0xa0 drivers/tty/tty_port.c:39
 flush_to_ldisc+0x1b0/0x750 drivers/tty/tty_buffer.c:445
 process_scheduled_works+0x2b0/0x10d0 kernel/workqueue.c:3229
 worker_thread+0x3dc/0x950 kernel/workqueue.c:3391
 kthread+0x2a3/0x370 kernel/kthread.c:389
 ret_from_fork+0x2d/0x70 arch/x86/kernel/process.c:147
 ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:257

Freed by task 3367:
 kfree+0x126/0x420 mm/slub.c:4580
 gsm_cleanup_mux+0x36c/0x7b0 drivers/tty/n_gsm.c:3160 [n_gsm]
 gsmld_ioctl+0x395/0x1450 drivers/tty/n_gsm.c:3408 [n_gsm]
 tty_ioctl+0x643/0x1100 drivers/tty/tty_io.c:2818

[Analysis]
gsm_msg on the tx_ctrl_list or tx_data_list of gsm_mux
can be freed by multi threads through ioctl,which leads
to the occurrence of uaf. Protect it by gsm tx lock.

Signed-off-by: Longlong Xia <xialonglong@kylinos.cn>
Suggested-by: Jiri Slaby <jirislaby@kernel.org>
---
v1 -> v2:
    - Use ORC and clean up commit messages.
    - Use guard marco as suggested by Jiri Slaby.

 drivers/tty/n_gsm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 5d37a0984916..252849910588 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -3157,6 +3157,8 @@ static void gsm_cleanup_mux(struct gsm_mux *gsm, bool disc)
 	mutex_unlock(&gsm->mutex);
 	/* Now wipe the queues */
 	tty_ldisc_flush(gsm->tty);
+
+	guard(spinlock_irqsave)(&gsm->tx_lock);
 	list_for_each_entry_safe(txq, ntxq, &gsm->tx_ctrl_list, list)
 		kfree(txq);
 	INIT_LIST_HEAD(&gsm->tx_ctrl_list);
-- 
2.45.2