diff mbox series

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

Message ID 20240924093519.767036-1-xialonglong@kylinos.cn
State New
Headers show
Series [1/1] tty: n_gsm: Fix use-after-free in gsm_cleanup_mux | expand

Commit Message

Longlong Xia Sept. 24, 2024, 9:35 a.m. UTC
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(+)

Comments

Jiri Slaby Sept. 25, 2024, 7:05 a.m. UTC | #1
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);
>   }
>   
>   /**
diff mbox series

Patch

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);
 }
 
 /**