Re: [PATCH 1/1] tty: n_gsm: Avoid sleeping during .write() whilst atomic

[Lee Jones]

On Tue, 03 Oct 2023, Greg Kroah-Hartman wrote:

> On Tue, Oct 03, 2023 at 06:00:20PM +0100, Lee Jones wrote:
> > The important part of the call stack being:
> > 
> >   gsmld_write()             # Takes a lock and disables IRQs
> >     con_write()
> >       console_lock()
> 
> Wait, why is the n_gsm line discipline being used for a console?
> 
> What hardware/protocol wants this to happen?
> 
> gsm I thought was for a very specific type of device, not a console.
> 
> As per:
> 	https://www.kernel.org/doc/html/v5.9/driver-api/serial/n_gsm.html
> this is a specific modem protocol, why is con_write() being called?

What it's meant for and what random users can make it do are likely to
be quite separate questions.  This scenario is user driven and can be
replicated simply by issuing a few syscalls (open, ioctl, write).

> >         __might_sleep()
> >           __might_resched() # Complains that IRQs are disabled
> > 
> > To fix this, let's ensure mutual exclusion by using a protected shared
> > variable (busy) instead.  We'll use the current locking mechanism to
> > protect it, but ensure that the locks are released and IRQs re-enabled
> > by the time we transit further down the call chain which may sleep.
> > 
> > Cc: Daniel Starke <daniel.starke@xxxxxxxxxxx>
> > Cc: Fedor Pchelkin <pchelkin@xxxxxxxxx>
> > Cc: Jiri Slaby <jirislaby@xxxxxxxxxx>
> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Cc: linux-serial@xxxxxxxxxxxxxxx
> > Reported-by: syzbot+5f47a8cea6a12b77a876@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Signed-off-by: Lee Jones <lee@xxxxxxxxxx>
> > ---
> >  drivers/tty/n_gsm.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> > index 1f3aba607cd51..b83a97d58381f 100644
> > --- a/drivers/tty/n_gsm.c
> > +++ b/drivers/tty/n_gsm.c
> > @@ -270,6 +270,7 @@ struct gsm_mux {
> >  	struct tty_struct *tty;		/* The tty our ldisc is bound to */
> >  	spinlock_t lock;
> >  	struct mutex mutex;
> > +	bool busy;
> >  	unsigned int num;
> >  	struct kref ref;
> >  
> > @@ -3253,6 +3254,7 @@ static struct gsm_mux *gsm_alloc_mux(void)
> >  	gsm->dead = true;	/* Avoid early tty opens */
> >  	gsm->wait_config = false; /* Disabled */
> >  	gsm->keep_alive = 0;	/* Disabled */
> > +	gsm->busy = false;
> >  
> >  	/* Store the instance to the mux array or abort if no space is
> >  	 * available.
> > @@ -3718,11 +3720,21 @@ static ssize_t gsmld_write(struct tty_struct *tty, struct file *file,
> >  
> >  	ret = -ENOBUFS;
> >  	spin_lock_irqsave(&gsm->tx_lock, flags);
> > +	if (gsm->busy) {
> > +		spin_unlock_irqrestore(&gsm->tx_lock, flags);
> > +		return -EBUSY;
> 
> So you just "busted" the re-entrant call chain here, are you sure this
> is ok for this protocl?  Can it handle -EBUSY?

I should have marked this submission as RFC.  Mea culpa.  Please
consider it as such going forward.  Feedback like this is highly
valuable.

> Daniel, any thoughts?
> 
> And Lee, you really don't have this hardware, right?  So why are you
> dealing with bug reports for it?  :)

'cos Syzkaller.

And no, as per the splat above, this was reproduced in qemu.

-- 
Lee Jones [李琼斯]