Re: [PATCH] tty: n_hdlc: make n_hdlc_tty_wakeup() asynchronous

[Fabio M. De Francesco]

On Monday, December 6, 2021 12:44:38 PM CET Tetsuo Handa wrote:
> syzbot is reporting that an unprivileged user who logged in from tty
> console can crash the system using a reproducer shown below [1], for
> n_hdlc_tty_wakeup() is synchronously calling n_hdlc_send_frames().
> 
> ----------
>   #include <sys/ioctl.h>
>   #include <unistd.h>
> 
>   int main(int argc, char *argv[])
>   {
>     const int disc = 0xd;
> 
>     ioctl(1, TIOCSETD, &disc);
>     while (1) {
>       ioctl(1, TCXONC, 0);
>       write(1, "", 1);
>       ioctl(1, TCXONC, 1); /* Kernel panic - not syncing: scheduling while atomic */
>     }
>   }
> ----------
> 
> Linus suspected that "struct tty_ldisc"->ops->write_wakeup() must not
> sleep, and Jiri confirmed it from include/linux/tty_ldisc.h. Thus, defer
> n_hdlc_send_frames() from n_hdlc_tty_wakeup() to a WQ context like
> net/nfc/nci/uart.c does.
> 
> Link: https://syzkaller.appspot.com/bug?extid=5f47a8cea6a12b77a876 [1]
> Reported-by: syzbot <syzbot+5f47a8cea6a12b77a876@xxxxxxxxxxxxxxxxxxxxxxxxx>
> Analyzed-by: Fabio M. De Francesco <fmdefrancesco@xxxxxxxxx>
> Suggested-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Confirmed-by: Jiri Slaby <jirislaby@xxxxxxxxxx>
> Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> ---
>  drivers/tty/n_hdlc.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)

This looks to be the correct solution, at least for fixing the SAC bug that 
Syzbot reported.

I say "at least" only because (for the moment) we have lost the synchronization 
that the spinlocks in n_hdlc_tty_ioctl() were meant to assure. 

As we have discussed, now n_hdlc_tty_wakeup() returns immediately after 
calling schedule_work(). Therefore, n_hdlc_tty_ioctl() releases the spinlock 
without it being notified whether or not n_hdlc_send_frames() has had the 
chance to run and complete.[1][2][3]

Only a minor note: since the purpose of the new "write_work"  is to start tty, 
I'd have chosen different name, like "start_work" or "start_write_work" and I'd 
have used "n_hdlc_tty_start_work()" instead of "n_hdlc_tty_write_work()" for the 
callback.

Since I have analyzed and discussed this bug with you and others, I assume that 
I got the necessary knowledge of this subject that allows me to review this patch.

Therefore, despite the due reservations about the loss of alternation and 
synchronization between __stop_tty () and __start_tty (), this work is useful for 
fixing the reported bug, so I'd like to give my tag...

Reviewed-by: Fabio M. De Francesco <fmdefrancesco@xxxxxxxxx>

Thanks for your patch,

Fabio M. De Francesco

[1] https://lore.kernel.org/lkml/3017492.JFOoIcAZ2s@localhost.localdomain/
[2] https://lore.kernel.org/lkml/5d055fbd-e94a-fe54-d3e0-982dc455ed1a@xxxxxxxxxxxxxxxxxxx/
[3] https://lore.kernel.org/lkml/2064700.cDd5PexU1D@localhost.localdomain/

> 
> diff --git a/drivers/tty/n_hdlc.c b/drivers/tty/n_hdlc.c
> index 7e0884ecc74f..23ba1fc99df8 100644
> --- a/drivers/tty/n_hdlc.c
> +++ b/drivers/tty/n_hdlc.c
> @@ -140,6 +140,8 @@ struct n_hdlc {
>  	struct n_hdlc_buf_list	rx_buf_list;
>  	struct n_hdlc_buf_list	tx_free_buf_list;
>  	struct n_hdlc_buf_list	rx_free_buf_list;
> +	struct work_struct	write_work;
> +	struct tty_struct	*tty_for_write_work;
>  };
>  
>  /*
> @@ -154,6 +156,7 @@ static struct n_hdlc_buf *n_hdlc_buf_get(struct n_hdlc_buf_list *list);
>  /* Local functions */
>  
>  static struct n_hdlc *n_hdlc_alloc(void);
> +static void n_hdlc_tty_write_work(struct work_struct *work);
>  
>  /* max frame size for memory allocations */
>  static int maxframe = 4096;
> @@ -210,6 +213,8 @@ static void n_hdlc_tty_close(struct tty_struct *tty)
>  	wake_up_interruptible(&tty->read_wait);
>  	wake_up_interruptible(&tty->write_wait);
>  
> +	cancel_work_sync(&n_hdlc->write_work);
> +
>  	n_hdlc_free_buf_list(&n_hdlc->rx_free_buf_list);
>  	n_hdlc_free_buf_list(&n_hdlc->tx_free_buf_list);
>  	n_hdlc_free_buf_list(&n_hdlc->rx_buf_list);
> @@ -241,6 +246,8 @@ static int n_hdlc_tty_open(struct tty_struct *tty)
>  		return -ENFILE;
>  	}
>  
> +	INIT_WORK(&n_hdlc->write_work, n_hdlc_tty_write_work);
> +	n_hdlc->tty_for_write_work = tty;
>  	tty->disc_data = n_hdlc;
>  	tty->receive_room = 65536;
>  
> @@ -334,6 +341,20 @@ static void n_hdlc_send_frames(struct n_hdlc *n_hdlc, struct tty_struct *tty)
>  		goto check_again;
>  }	/* end of n_hdlc_send_frames() */
>  
> +/**
> + * n_hdlc_tty_write_work - Asynchronous callback for transmit wakeup
> + * @work: pointer to work_struct
> + *
> + * Called when low level device driver can accept more send data.
> + */
> +static void n_hdlc_tty_write_work(struct work_struct *work)
> +{
> +	struct n_hdlc *n_hdlc = container_of(work, struct n_hdlc, write_work);
> +	struct tty_struct *tty = n_hdlc->tty_for_write_work;
> +
> +	n_hdlc_send_frames(n_hdlc, tty);
> +}	/* end of n_hdlc_tty_write_work() */
> +
>  /**
>   * n_hdlc_tty_wakeup - Callback for transmit wakeup
>   * @tty: pointer to associated tty instance data
> @@ -344,7 +365,7 @@ static void n_hdlc_tty_wakeup(struct tty_struct *tty)
>  {
>  	struct n_hdlc *n_hdlc = tty->disc_data;
>  
> -	n_hdlc_send_frames(n_hdlc, tty);
> +	schedule_work(&n_hdlc->write_work);
>  }	/* end of n_hdlc_tty_wakeup() */
>  
>  /**
> -- 
> 2.18.4
> 
> 
>