[PATCH v2] tty: n_gsm: restrict tty devices to attach

[PATCH v2] tty: n_gsm: restrict tty devices to attach

[Tetsuo Handa]

syzbot is reporting sleep in atomic context, for gsmld_write() is calling
con_write() with spinlock held and IRQs disabled.

Since n_gsm is designed to be used for serial port [1], reject attaching to
virtual consoles and PTY devices, by checking tty's device major/minor
numbers at gsmld_open().

Starke, Daniel commented

  Our application of this protocol is only with specific modems to enable
  circuit switched operation (handling calls, selecting/querying networks,
  etc.) while doing packet switched communication (i.e. IP traffic over
  PPP). The protocol was developed for such use cases.

at [2], but it seems that nobody can define allow list for device numbers
where this protocol should accept. Therefore, this patch defines deny list
for device numbers.

Greg Kroah-Hartman is not happy with use of hard-coded magic numbers [3],
but I don't think we want to update include/uapi/linux/major.h and add
include/uapi/linux/minor.h just for fixing this bug.

Link: https://www.kernel.org/doc/html/v6.8/driver-api/tty/n_gsm.html [1]
Link: https://lkml.kernel.org/r/DB9PR10MB588170E923A6ED8B3D6D9613E0CBA@DB9PR10MB5881.EURPRD10.PROD.OUTLOOK.COM [2]
Link: https://lkml.kernel.org/r/2024020615-stir-dragster-aeb6@gregkh [3]
Reported-by: syzbot <syzbot+dbac96d8e73b61aa559c@syzkaller.appspotmail.com>
Closes: https://syzkaller.appspot.com/bug?extid=dbac96d8e73b61aa559c
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
Adding LSM ML to CC list in order to ask for comments if Greg again
complained that we don't want to add sanity check on the kernel side.
I agree that we should fix fuzzers if fuzzers are writing random data
to /dev/mem or /dev/kmem . But for example
https://lkml.kernel.org/r/CAADnVQJQvcZOA_BbFxPqNyRbMdKTBSMnf=cKvW7NJ8LxxP54sA@mail.gmail.com
demonstrates that developers try to fix bugs on the kernel side rather
than tell fuzzers not to do artificial things.

 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 4036566febcb..14581483af78 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -3623,6 +3623,7 @@ static void gsmld_close(struct tty_struct *tty)
 static int gsmld_open(struct tty_struct *tty)
 {
 	struct gsm_mux *gsm;
+	int major;
 
 	if (!capable(CAP_NET_ADMIN))
 		return -EPERM;
@@ -3630,6 +3631,17 @@ static int gsmld_open(struct tty_struct *tty)
 	if (tty->ops->write == NULL)
 		return -EINVAL;
 
+	major = tty->driver->major;
+	/* Reject Virtual consoles */
+	if (major == 4 && tty->driver->minor_start == 1)
+		return -EINVAL;
+	/* Reject Unix98 PTY masters/slaves */
+	if (major >= 128 && major <= 143)
+		return -EINVAL;
+	/* Reject BSD PTY masters/slaves */
+	if (major >= 2 && major <= 3)
+		return -EINVAL;
+
 	/* Attach our ldisc data */
 	gsm = gsm_alloc_mux();
 	if (gsm == NULL)
-- 
2.18.4

Re: [PATCH v2] tty: n_gsm: restrict tty devices to attach

[Greg Kroah-Hartman]

On Sat, Apr 20, 2024 at 08:12:32PM +0900, Tetsuo Handa wrote:
> syzbot is reporting sleep in atomic context, for gsmld_write() is calling
> con_write() with spinlock held and IRQs disabled.
> 
> Since n_gsm is designed to be used for serial port [1], reject attaching to
> virtual consoles and PTY devices, by checking tty's device major/minor
> numbers at gsmld_open().
> 
> Starke, Daniel commented
> 
>   Our application of this protocol is only with specific modems to enable
>   circuit switched operation (handling calls, selecting/querying networks,
>   etc.) while doing packet switched communication (i.e. IP traffic over
>   PPP). The protocol was developed for such use cases.
> 
> at [2], but it seems that nobody can define allow list for device numbers
> where this protocol should accept. Therefore, this patch defines deny list
> for device numbers.
> 
> Greg Kroah-Hartman is not happy with use of hard-coded magic numbers [3],
> but I don't think we want to update include/uapi/linux/major.h and add
> include/uapi/linux/minor.h just for fixing this bug.

Sorry, but again, do it properly, nothing has changed here, so I will
not take this patch.

> Link: https://www.kernel.org/doc/html/v6.8/driver-api/tty/n_gsm.html [1]
> Link: https://lkml.kernel.org/r/DB9PR10MB588170E923A6ED8B3D6D9613E0CBA@DB9PR10MB5881.EURPRD10.PROD.OUTLOOK.COM [2]
> Link: https://lkml.kernel.org/r/2024020615-stir-dragster-aeb6@gregkh [3]
> Reported-by: syzbot <syzbot+dbac96d8e73b61aa559c@syzkaller.appspotmail.com>
> Closes: https://syzkaller.appspot.com/bug?extid=dbac96d8e73b61aa559c
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> Adding LSM ML to CC list in order to ask for comments if Greg again
> complained that we don't want to add sanity check on the kernel side.
> I agree that we should fix fuzzers if fuzzers are writing random data
> to /dev/mem or /dev/kmem . But for example
> https://lkml.kernel.org/r/CAADnVQJQvcZOA_BbFxPqNyRbMdKTBSMnf=cKvW7NJ8LxxP54sA@mail.gmail.com
> demonstrates that developers try to fix bugs on the kernel side rather
> than tell fuzzers not to do artificial things.

Again, this ldisc requires root permissions to bind to it, and we have a
very long list of known bugs in this driver, this one being only one
very tiny minor one.  To fix it properly, do it right, as stated before,
this type of odd bandage isn't ok as it doesn't actually fix/solve
anything except fuzzers doing the wrong thing (i.e. no real user will
ever do this.)

thanks,

greg k-h

Re: [PATCH v2] tty: n_gsm: restrict tty devices to attach

[Linus Torvalds]

On Sat, 20 Apr 2024 at 04:12, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> Since n_gsm is designed to be used for serial port [1], reject attaching to
> virtual consoles and PTY devices, by checking tty's device major/minor
> numbers at gsmld_open().

If we really just want to restrict it to serial devices, then do
something like, this:

   drivers/tty/n_gsm.c | 2 ++
   1 file changed, 2 insertions(+)

  diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
  index 4036566febcb..24425ef35b2b 100644
  --- a/drivers/tty/n_gsm.c
  +++ b/drivers/tty/n_gsm.c
  @@ -3629,6 +3629,8 @@ static int gsmld_open(struct tty_struct *tty)

        if (tty->ops->write == NULL)
                return -EINVAL;
  +     if (tty->ops->set_serial == NULL)
  +             return -EINVAL;

        /* Attach our ldisc data */
        gsm = gsm_alloc_mux();

which at least matches the current (largely useless) pattern of
checking for a write function.

I think all real serial sub-drivers already have that 'set_serial()'
function, and if there are some that don't, we could just add a dummy
for them. No?

Alternatively, we could go the opposite way, and have some flag in the
line discipline that says "I can be a console", and just check that in
tty_set_ldisc() for the console.

That would probably be a good idea regardless, but likely requires more effort.

But this kind of random major number testing seems wrong. It's trying
to deal with the _symptoms_, not some deeper truth.

                  Linus

Re: [PATCH v2] tty: n_gsm: restrict tty devices to attach

[Linus Torvalds]

On Sat, 20 Apr 2024 at 10:34, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Alternatively, we could go the opposite way, and have some flag in the
> line discipline that says "I can be a console", and just check that in
> tty_set_ldisc() for the console.

Actually, I take that back. It's not /dev/console that is the problem,
that just happened to be the one oops I looked at.

Most other normal tty devices just expect ->write() to be called in
normal process context, so if we do a line discipline flag, it would
have to be something like "I'm ok with being called with interrupts
disabled", and then the n_gsm ->open function would just check that.

So it would end up being just another form of that

  +     if (tty->ops->set_serial == NULL)
  +             return -EINVAL;

check - but maybe more explicit and prettier.

Because a real serial driver might not be ok with it either, if it
uses a semaphore or something.

Whatever. I think the 'set_serial' test would at least be an improvement.

            Linus

Re: [PATCH v2] tty: n_gsm: restrict tty devices to attach

[Linus Torvalds]

On Sat, 20 Apr 2024 at 11:02, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Most other normal tty devices just expect ->write() to be called in
> normal process context, so if we do a line discipline flag, it would
                                        ^^^^^^^^^^^^^^^^^^^^
> have to be something like "I'm ok with being called with interrupts
> disabled", and then the n_gsm ->open function would just check that.

Not line discipline - it would be a 'struct tty_operations' flag
saying 'my ->write() function is ok with atomic context".

             Linus

Re: [PATCH v2] tty: n_gsm: restrict tty devices to attach

[Tetsuo Handa]

On 2024/04/21 3:05, Linus Torvalds wrote:
> On Sat, 20 Apr 2024 at 11:02, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> Most other normal tty devices just expect ->write() to be called in
>> normal process context, so if we do a line discipline flag, it would
>                                         ^^^^^^^^^^^^^^^^^^^^
>> have to be something like "I'm ok with being called with interrupts
>> disabled", and then the n_gsm ->open function would just check that.
> 
> Not line discipline - it would be a 'struct tty_operations' flag
> saying 'my ->write() function is ok with atomic context".

"struct tty_ldisc_ops" says that ->write() function (e.g. gsmld_write())
is allowed to sleep and "struct tty_operations" says that ->write() function
(e.g. con_write()) is not allowed to sleep. Thus, I initially proposed
https://lkml.kernel.org/r/9cd9d3eb-418f-44cc-afcf-7283d51252d6@I-love.SAKURA.ne.jp
which makes con_write() no-op when called with IRQs disabled.

My major/minor approach is based on a suggestion from Jiri that we just somehow
disallow attaching this line discipline to a console, with a concern from Starke
that introducing cross references is hard to maintain taken into account.
https://lkml.kernel.org/r/DB9PR10MB5881526A2B8F27FE36C49134E0CBA@DB9PR10MB5881.EURPRD10.PROD.OUTLOOK.COM

Now, your 'struct tty_operations' flag saying 'my ->write() function is OK with
atomic context' is expected to be set to all drivers.

Do we instead want to add 'struct tty_operations' flag saying 'my ->write() function
is NOT OK with atomic context' and turn on the flag for the console driver?

Re: [PATCH v2] tty: n_gsm: restrict tty devices to attach

[Linus Torvalds]

On Sun, 21 Apr 2024 at 06:28, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> "struct tty_ldisc_ops" says that ->write() function (e.g. gsmld_write())
> is allowed to sleep and "struct tty_operations" says that ->write() function
> (e.g. con_write()) is not allowed to sleep.

Well, clearly con_write() *is* allowed to sleep. The very first thing
it does is that

        console_lock();

thing, which uses a sleeping semaphore.

But yes, the comment in the header does say "may not sleep".

Clearly that comment doesn't actually reflect reality - and never did.
The console lock sleeping isn't some new thing (ie it doesn't come
from the somewhat recent printk changes).

So the comment is bogus and wrong.

> Thus, I initially proposed
> https://lkml.kernel.org/r/9cd9d3eb-418f-44cc-afcf-7283d51252d6@I-love.SAKURA.ne.jp
> which makes con_write() no-op when called with IRQs disabled.

The thing is, that's not the only thing that makes atomic context.

And some atomic contexts cannot be detected at run-time, they are
purely static (ie being inside a spinlock withg a !PREEMPT kernel
build).

So you cannot test for this.

The only option is to *mark* the ones that are atomic. Which was my suggestion.

> My major/minor approach is based on a suggestion from Jiri that we just somehow
> disallow attaching this line discipline to a console

Since we already know that the comment is garbage, why do you think
it's just a con_write() that has this issue?

And if it is only the console that has this issue, why are you testing
for other major/minor numbers?

> Now, your 'struct tty_operations' flag saying 'my ->write() function is OK with
> atomic context' is expected to be set to all drivers.

I'm not convinced. The only thing I know is that the comment in
question is wrong, and has been wrong for over a decade (and honestly,
probably pretty much forever).

So how confident are we that other tty write functions are ok?

Also, since you think that only con_write() has a problem, why the
heck are you then testing for ptys etc? From a quick check, the
pty->ops->write() function is fine.

                  Linus

Re: [PATCH v2] tty: n_gsm: restrict tty devices to attach

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1219 bytes --]

On Sun, 21 Apr 2024 at 09:04, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> The only option is to *mark* the ones that are atomic. Which was my suggestion.

Actually, another option would be to just return an error at 'set_ldisc()' time.

Sadly, the actual "tty->ops->set_ldisc()" function not only returns
'void' (easy enough to change - there aren't that many of them), but
it's called too late after the old ldisc has already been dropped.
It's basically a "inform tty about new ldisc" and is not useful for a
"is this ok"?

But we could trivially add a "ldisc_ok()" function, and have the vt
driver say "I only accept N_TTY".

Something like this ENTIRELY UNTESTED patch.

Again - this is untested, and maybe there are other tty drivers that
have issues with the stranger line disciplines, but this at least
seems simple and fairly easy to explain why we do what we do..

And if pty's really need the same thing, that would be easy to add.
But I actually think that at least pty slaves should *not* limit
ldiscs, because the whole point of a pty slave is to look like another
tty. If you want to emulate a serial device over a network, the way to
do it would be with a pty.

Hmm?

                Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 2497 bytes --]

 drivers/tty/tty_ldisc.c    |  6 ++++++
 drivers/tty/vt/vt.c        | 10 ++++++++++
 include/linux/tty_driver.h |  8 ++++++++
 3 files changed, 24 insertions(+)

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 3f68e213df1f..d80e9d4c974b 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -545,6 +545,12 @@ int tty_set_ldisc(struct tty_struct *tty, int disc)
 		goto out;
 	}
 
+	if (tty->ops->ldisc_ok) {
+		retval = tty->ops->ldisc_ok(tty, disc);
+		if (retval)
+			goto out;
+	}
+
 	old_ldisc = tty->ldisc;
 
 	/* Shutdown the old discipline. */
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 9b5b98dfc8b4..cd87e3d1291e 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -3576,6 +3576,15 @@ static void con_cleanup(struct tty_struct *tty)
 	tty_port_put(&vc->port);
 }
 
+/*
+ * We can't deal with anything but the N_TTY ldisc,
+ * because we can sleep in our write() routine.
+ */
+static int con_ldisc_ok(struct tty_struct *tty, int ldisc)
+{
+	return ldisc == N_TTY ? 0 : -EINVAL;
+}
+
 static int default_color           = 7; /* white */
 static int default_italic_color    = 2; // green (ASCII)
 static int default_underline_color = 3; // cyan (ASCII)
@@ -3695,6 +3704,7 @@ static const struct tty_operations con_ops = {
 	.resize = vt_resize,
 	.shutdown = con_shutdown,
 	.cleanup = con_cleanup,
+	.ldisc_ok = con_ldisc_ok,
 };
 
 static struct cdev vc0_cdev;
diff --git a/include/linux/tty_driver.h b/include/linux/tty_driver.h
index 7372124fbf90..dd4b31ce6d5d 100644
--- a/include/linux/tty_driver.h
+++ b/include/linux/tty_driver.h
@@ -154,6 +154,13 @@ struct serial_struct;
  *
  *	Optional. Called under the @tty->termios_rwsem. May sleep.
  *
+ * @ldisc_ok: ``int ()(struct tty_struct *tty, int ldisc)``
+ *
+ *	This routine allows the @tty driver to decide if it can deal
+ *	with a particular @ldisc.
+ *
+ *	Optional. Called under the @tty->ldisc_sem and @tty->termios_rwsem.
+ *
  * @set_ldisc: ``void ()(struct tty_struct *tty)``
  *
  *	This routine allows the @tty driver to be notified when the device's
@@ -372,6 +379,7 @@ struct tty_operations {
 	void (*hangup)(struct tty_struct *tty);
 	int (*break_ctl)(struct tty_struct *tty, int state);
 	void (*flush_buffer)(struct tty_struct *tty);
+	int (*ldisc_ok)(struct tty_struct *tty, int ldisc);
 	void (*set_ldisc)(struct tty_struct *tty);
 	void (*wait_until_sent)(struct tty_struct *tty, int timeout);
 	void (*send_xchar)(struct tty_struct *tty, u8 ch);

Re: [PATCH v2] tty: n_gsm: restrict tty devices to attach

[Tetsuo Handa]

On 2024/04/22 1:04, Linus Torvalds wrote:
>> Now, your 'struct tty_operations' flag saying 'my ->write() function is OK with
>> atomic context' is expected to be set to all drivers.
> 
> I'm not convinced. The only thing I know is that the comment in
> question is wrong, and has been wrong for over a decade (and honestly,
> probably pretty much forever).
> 
> So how confident are we that other tty write functions are ok?
> 
> Also, since you think that only con_write() has a problem, why the
> heck are you then testing for ptys etc? From a quick check, the
> pty->ops->write() function is fine.

I tried to make deny-listing as close as allow-listing. But it seems that
ipw_write() in drivers/tty/ipwireless/tty.c (e.g. /dev/ttyIPWp0 ) does
sleep as well as con_write() in drivers/tty/vt/vt.c .

I couldn't judge serial_write() in drivers/usb/serial/usb-serial.c (e.g.
/dev/ttyUSB0 ). But since device number for /dev/ttyIPWp0 is assigned
dynamically, we can't rely on major/minor for detecting /dev/ttyIPWp0 from
gsmld_open() side.

That is, we need to handle this problem from "struct tty_operations" side
(like I initially tried).



On 2024/04/22 2:18, Linus Torvalds wrote:
> On Sun, 21 Apr 2024 at 09:04, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> The only option is to *mark* the ones that are atomic. Which was my suggestion.

Since majority of "struct tty_operations"->write() seems to be atomic,
I prefer updating only ones which cannot be atomic.

> 
> Actually, another option would be to just return an error at 'set_ldisc()' time.
> 
> Sadly, the actual "tty->ops->set_ldisc()" function not only returns
> 'void' (easy enough to change - there aren't that many of them), but
> it's called too late after the old ldisc has already been dropped.
> It's basically a "inform tty about new ldisc" and is not useful for a
> "is this ok"?
> 
> But we could trivially add a "ldisc_ok()" function, and have the vt
> driver say "I only accept N_TTY".
> 
> Something like this ENTIRELY UNTESTED patch.
> 
> Again - this is untested, and maybe there are other tty drivers that
> have issues with the stranger line disciplines, but this at least
> seems simple and fairly easy to explain why we do what we do..

This patch works for me. You can propose a formal patch.

Re: [PATCH v2] tty: n_gsm: restrict tty devices to attach

[Linus Torvalds]

On Tue, 23 Apr 2024 at 08:26, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> On 2024/04/22 1:04, Linus Torvalds wrote:
> >
> > Actually, another option would be to just return an error at 'set_ldisc()' time.
>
> This patch works for me. You can propose a formal patch.

Ok, I wrote a commit message, added your tested-by, and sent it out

    https://lore.kernel.org/all/20240423163339.59780-1-torvalds@linux-foundation.org/

let's see if anybody has better ideas, but that patch at least looks
palatable to me.

                  Linus