Received-SPF: pass (google.com: domain of linux-kernel+bounces-148310-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 17 Apr 2024 19:19:48 +0900
From: Yewon Choi <woni9911@gmail.com>
To: "Starke, Daniel" <daniel.starke@siemens.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jirislaby@kernel.org>, linux-kernel@vger.kernel.org,
	linux-serial@vger.kernel.org,
	"Dae R. Jeong" <threeearcat@gmail.com>,
	syzbot+6e3e8f30f269f5028e5d@syzkaller.appspotmail.com
Subject: Re: tty: n_gsm: race condition in gsmld_ioctl
Message-ID: <Zh+iROsYkqoRsJQj@libra05>
References: <Zh5Zj35zeobGGzKj@libra05>
 <69b4cd22-3a02-4d5c-a110-152b8ba8200b@kernel.org>
 <DB9PR10MB5881D2170678C169FB42A423E0082@DB9PR10MB5881.EURPRD10.PROD.OUTLOOK.COM>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <DB9PR10MB5881D2170678C169FB42A423E0082@DB9PR10MB5881.EURPRD10.PROD.OUTLOOK.COM>

On Tue, Apr 16, 2024 at 12:26:30PM +0000, Starke, Daniel wrote:
> > 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.
> 

Thank you for your comprehensive explanation.

As you described, there are many concurrency issues entangled each other and 
fixing single one will be rather confusing.
I hope this mail is supportive on solving the problems.

Sincerely,
Yewon Choi

> Best regards,
> Daniel Starke