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

[Jiri Slaby <jirislaby@kernel.org>]

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