linux-kernel - RE: tty: n_gsm: race condition in gsmld

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <DB9PR10MB5881D2170678C169FB42A423E0082@DB9PR10MB5881.EURPRD10.PROD.OUTLOOK.COM>
Date: Tue, 16 Apr 2024 12:26:30 +0000
From: "Starke, Daniel" <daniel.starke@...mens.com>
To: Jiri Slaby <jirislaby@...nel.org>, Yewon Choi <woni9911@...il.com>, Greg
 Kroah-Hartman <gregkh@...uxfoundation.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-serial@...r.kernel.org"
	<linux-serial@...r.kernel.org>
CC: "Dae R. Jeong" <threeearcat@...il.com>,
	"syzbot+6e3e8f30f269f5028e5d@...kaller.appspotmail.com"
	<syzbot+6e3e8f30f269f5028e5d@...kaller.appspotmail.com>
Subject: RE: tty: n_gsm: race condition in gsmld_ioctl

> We think either (1) gsm_dlci_alloc() should hold a lock(mutex) and do 
> internal check about whether gsm->dlci[addr] is NUll or not, OR
> (2) all callers of gsm_dlci_alloc() should hold gsm->mutex and check 
> whether gsm->dlci[addr] is NUll or not (like gsmtty_install()).
> 
> Could you check this? If it makes sense, we will write a patch 
> following one of the suggestions.

We are currently dealing with multiple race conditions in the n_gsm. Not
only for the syzkaller reports but in recent exploits for example, too.
I believe we need an overall concept/solution for these.

Currently, the following actors can race:
- ioctl (e.g. resetting the mux or one of its DLCIs)
- ldisc callbacks (e.g. receiving SABM or DISC in gsm_queue())
- tty callbacks (e.g. gsmtty_hangup())
- internal write task (gsmld_write_task())
- internal timers (e.g. gsm_control_keep_alive())
- driver removal

Each with another and ioctl's from multiple threads.

Guarding these is not trivial for the following reasons:
- execution context may not allow sleep (timers, write task, tty callbacks?)
- creating an atomic section may conflict inner sleeps (e.g. ioctl)
- dead-locking needs to be considered
- locking may introduce high performance bottlenecks

Still, not guarding one of the racing actors does not appears to be
possible as I see it.

Unfortunately, for the same reason the sleep while atomic issue when using
a console on a virtual tty has not been fixed yet, I have no solution at
hand. We are dealing with a quite complex situation here.

Nevertheless, creating many small patches here and there should not be our
solution for obvious reasons. This does not give a complete solution and
makes it harder to find the remaining corner cases.

I can assist to find a solution here, but I have not enough time to do this
alone at the moment.

Best regards,
Daniel Starke