| 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
| ||
|
Message-ID: <3981949.J2oczr2A6u@localhost.localdomain>
Date: Mon, 06 Dec 2021 20:06:06 +0100
From: "Fabio M. De Francesco" <fmdefrancesco@...il.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jiri Slaby <jirislaby@...nel.org>,
linux-kernel@...r.kernel.org,
Linus Torvalds <torvalds@...ux-foundation.org>,
Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Cc: syzbot+5f47a8cea6a12b77a876@...kaller.appspotmail.com,
Marco Elver <elver@...gle.com>,
Max Filippov <jcmvbkbc@...il.com>,
David Sterba <dsterba@...e.com>,
Bhaskar Chowdhury <unixbhaskar@...il.com>,
nick black <dankamongmen@...il.com>,
Igor Matheus Andrade Torrente <igormtorrente@...il.com>
Subject: Re: [PATCH] tty: n_hdlc: make n_hdlc_tty_wakeup() asynchronous
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@...kaller.appspotmail.com>
> Analyzed-by: Fabio M. De Francesco <fmdefrancesco@...il.com>
> Suggested-by: Linus Torvalds <torvalds@...ux-foundation.org>
> Confirmed-by: Jiri Slaby <jirislaby@...nel.org>
> Signed-off-by: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
> ---
> 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@...il.com>
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@i-love.sakura.ne.jp/
[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
>
>
>
Powered by blists - more mailing lists