| Message ID | e696e720-0cd3-4505-8469-a94815b39467@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [v2] tty: n_gsm: restrict tty devices to attach | expand |
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
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
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
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
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?
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
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
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.
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
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)
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(+)