2018-10-22 16:24:37

by H. Peter Anvin

[permalink] [raw]
Subject: [PATCH stable v2 1/2] termios, tty/tty_baudrate.c: fix buffer overrun

From: "H. Peter Anvin" <[email protected]>

On architectures with CBAUDEX == 0 (Alpha and PowerPC), the code in tty_baudrate.c does
not do any limit checking on the tty_baudrate[] array, and in fact a
buffer overrun is possible on both architectures. Add a limit check to
prevent that situation.

This will be followed by a much bigger cleanup/simplification patch.

Signed-off-by: H. Peter Anvin (Intel) <[email protected]>
Requested-by: Cc: Johan Hovold <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Jiri Slaby <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Richard Henderson <[email protected]>
Cc: Ivan Kokshaysky <[email protected]>
Cc: Matt Turner <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Kate Stewart <[email protected]>
Cc: Philippe Ombredanne <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Eugene Syromiatnikov <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Cc: Alan Cox <[email protected]>
Cc: <[email protected]>
---
drivers/tty/tty_baudrate.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/tty_baudrate.c b/drivers/tty/tty_baudrate.c
index 7576ceace571..f438eaa68246 100644
--- a/drivers/tty/tty_baudrate.c
+++ b/drivers/tty/tty_baudrate.c
@@ -77,7 +77,7 @@ speed_t tty_termios_baud_rate(struct ktermios *termios)
else
cbaud += 15;
}
- return baud_table[cbaud];
+ return cbaud >= n_baud_table ? 0 : baud_table[cbaud];
}
EXPORT_SYMBOL(tty_termios_baud_rate);

@@ -113,7 +113,7 @@ speed_t tty_termios_input_baud_rate(struct ktermios *termios)
else
cbaud += 15;
}
- return baud_table[cbaud];
+ return cbaud >= n_baud_table ? 0 : baud_table[cbaud];
#else /* IBSHIFT */
return tty_termios_baud_rate(termios);
#endif /* IBSHIFT */
--
2.14.4



2018-10-22 16:24:23

by H. Peter Anvin

[permalink] [raw]
Subject: [PATCH stable v2 2/2] arch/alpha, termios: implement BOTHER, IBSHIFT and termios2

Alpha has had c_ispeed and c_ospeed, but still set speeds in c_cflags
using arbitrary flags. Because BOTHER is not defined, the general
Linux code doesn't allow setting arbitrary baud rates, and because
CBAUDEX == 0, we can have an array overrun of the baud_rate[] table in
drivers/tty/tty_baudrate.c if (c_cflags & CBAUD) == 037.

Resolve both problems by #defining BOTHER to 037 on Alpha.

However, userspace still needs to know if setting BOTHER is actually
safe given legacy kernels (does anyone actually care about that on
Alpha anymore?), so enable the TCGETS2/TCSETS*2 ioctls on Alpha, even
though they use the same structure. Define struct termios2 just for
compatibility; it is the exact same structure as struct termios. In a
future patchset, this will be cleaned up so the uapi headers are
usable from libc.

Signed-off-by: H. Peter Anvin (Intel) <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Jiri Slaby <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Richard Henderson <[email protected]>
Cc: Ivan Kokshaysky <[email protected]>
Cc: Matt Turner <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Kate Stewart <[email protected]>
Cc: Philippe Ombredanne <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Eugene Syromiatnikov <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Cc: Johan Hovold <[email protected]>
Cc: Alan Cox <[email protected]>
Cc: <[email protected]>
---
arch/alpha/include/asm/termios.h | 8 +++++++-
arch/alpha/include/uapi/asm/ioctls.h | 5 +++++
arch/alpha/include/uapi/asm/termbits.h | 17 +++++++++++++++++
3 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/arch/alpha/include/asm/termios.h b/arch/alpha/include/asm/termios.h
index 6a8c53dec57e..b7c77bb1bfd2 100644
--- a/arch/alpha/include/asm/termios.h
+++ b/arch/alpha/include/asm/termios.h
@@ -73,9 +73,15 @@
})

#define user_termios_to_kernel_termios(k, u) \
- copy_from_user(k, u, sizeof(struct termios))
+ copy_from_user(k, u, sizeof(struct termios2))

#define kernel_termios_to_user_termios(u, k) \
+ copy_to_user(u, k, sizeof(struct termios2))
+
+#define user_termios_to_kernel_termios_1(k, u) \
+ copy_from_user(k, u, sizeof(struct termios))
+
+#define kernel_termios_to_user_termios_1(u, k) \
copy_to_user(u, k, sizeof(struct termios))

#endif /* _ALPHA_TERMIOS_H */
diff --git a/arch/alpha/include/uapi/asm/ioctls.h b/arch/alpha/include/uapi/asm/ioctls.h
index 3729d92d3fa8..dc8c20ac7191 100644
--- a/arch/alpha/include/uapi/asm/ioctls.h
+++ b/arch/alpha/include/uapi/asm/ioctls.h
@@ -32,6 +32,11 @@
#define TCXONC _IO('t', 30)
#define TCFLSH _IO('t', 31)

+#define TCGETS2 _IOR('T', 42, struct termios2)
+#define TCSETS2 _IOW('T', 43, struct termios2)
+#define TCSETSW2 _IOW('T', 44, struct termios2)
+#define TCSETSF2 _IOW('T', 45, struct termios2)
+
#define TIOCSWINSZ _IOW('t', 103, struct winsize)
#define TIOCGWINSZ _IOR('t', 104, struct winsize)
#define TIOCSTART _IO('t', 110) /* start output, like ^Q */
diff --git a/arch/alpha/include/uapi/asm/termbits.h b/arch/alpha/include/uapi/asm/termbits.h
index de6c8360fbe3..4575ba34a0ea 100644
--- a/arch/alpha/include/uapi/asm/termbits.h
+++ b/arch/alpha/include/uapi/asm/termbits.h
@@ -26,6 +26,19 @@ struct termios {
speed_t c_ospeed; /* output speed */
};

+/* Alpha has identical termios and termios2 */
+
+struct termios2 {
+ tcflag_t c_iflag; /* input mode flags */
+ tcflag_t c_oflag; /* output mode flags */
+ tcflag_t c_cflag; /* control mode flags */
+ tcflag_t c_lflag; /* local mode flags */
+ cc_t c_cc[NCCS]; /* control characters */
+ cc_t c_line; /* line discipline (== c_cc[19]) */
+ speed_t c_ispeed; /* input speed */
+ speed_t c_ospeed; /* output speed */
+};
+
/* Alpha has matching termios and ktermios */

struct ktermios {
@@ -152,6 +165,7 @@ struct ktermios {
#define B3000000 00034
#define B3500000 00035
#define B4000000 00036
+#define BOTHER 00037

#define CSIZE 00001400
#define CS5 00000000
@@ -169,6 +183,9 @@ struct ktermios {
#define CMSPAR 010000000000 /* mark or space (stick) parity */
#define CRTSCTS 020000000000 /* flow control */

+#define CIBAUD 07600000
+#define IBSHIFT 16
+
/* c_lflag bits */
#define ISIG 0x00000080
#define ICANON 0x00000100
--
2.14.4


2018-10-23 15:01:00

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH stable v2 1/2] termios, tty/tty_baudrate.c: fix buffer overrun

On Mon, Oct 22, 2018 at 09:19:04AM -0700, H. Peter Anvin (Intel) wrote:
> From: "H. Peter Anvin" <[email protected]>
>
> On architectures with CBAUDEX == 0 (Alpha and PowerPC), the code in tty_baudrate.c does
> not do any limit checking on the tty_baudrate[] array, and in fact a
> buffer overrun is possible on both architectures. Add a limit check to
> prevent that situation.
>
> This will be followed by a much bigger cleanup/simplification patch.
>
> Signed-off-by: H. Peter Anvin (Intel) <[email protected]>
> Requested-by: Cc: Johan Hovold <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Jiri Slaby <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: Richard Henderson <[email protected]>
> Cc: Ivan Kokshaysky <[email protected]>
> Cc: Matt Turner <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Kate Stewart <[email protected]>
> Cc: Philippe Ombredanne <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Eugene Syromiatnikov <[email protected]>
> Cc: <[email protected]>
> Cc: <[email protected]>
> Cc: Alan Cox <[email protected]>
> Cc: <[email protected]>
> ---
> drivers/tty/tty_baudrate.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)

As I think Al's big termios cleanups are going to be hitting Linus's
tree soon, do you know how these patches interact with that?

This patch seems like it will not, so I'll be glad to queue that up
after my first round of patches get merged to Linus later this week, but
the second one worries me.

thanks,

greg k-h

2018-10-23 17:03:22

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH stable v2 1/2] termios, tty/tty_baudrate.c: fix buffer overrun

On October 23, 2018 7:53:51 AM PDT, Greg Kroah-Hartman <[email protected]> wrote:
>On Mon, Oct 22, 2018 at 09:19:04AM -0700, H. Peter Anvin (Intel) wrote:
>> From: "H. Peter Anvin" <[email protected]>
>>
>> On architectures with CBAUDEX == 0 (Alpha and PowerPC), the code in
>tty_baudrate.c does
>> not do any limit checking on the tty_baudrate[] array, and in fact a
>> buffer overrun is possible on both architectures. Add a limit check
>to
>> prevent that situation.
>>
>> This will be followed by a much bigger cleanup/simplification patch.
>>
>> Signed-off-by: H. Peter Anvin (Intel) <[email protected]>
>> Requested-by: Cc: Johan Hovold <[email protected]>
>> Cc: Greg Kroah-Hartman <[email protected]>
>> Cc: Jiri Slaby <[email protected]>
>> Cc: Al Viro <[email protected]>
>> Cc: Richard Henderson <[email protected]>
>> Cc: Ivan Kokshaysky <[email protected]>
>> Cc: Matt Turner <[email protected]>
>> Cc: Thomas Gleixner <[email protected]>
>> Cc: Kate Stewart <[email protected]>
>> Cc: Philippe Ombredanne <[email protected]>
>> Cc: Greg Kroah-Hartman <[email protected]>
>> Cc: Eugene Syromiatnikov <[email protected]>
>> Cc: <[email protected]>
>> Cc: <[email protected]>
>> Cc: Alan Cox <[email protected]>
>> Cc: <[email protected]>
>> ---
>> drivers/tty/tty_baudrate.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>As I think Al's big termios cleanups are going to be hitting Linus's
>tree soon, do you know how these patches interact with that?
>
>This patch seems like it will not, so I'll be glad to queue that up
>after my first round of patches get merged to Linus later this week,
>but
>the second one worries me.
>
>thanks,
>
>greg k-h

I have been working with Al; we had approached much the same problems but from different directions. Mine ended up being a bit more comprehensive as a result, so I think we're going to end up using my code with Al's reviews.

So bottom line is that it should be all good.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

2018-10-23 20:21:04

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH stable v2 1/2] termios, tty/tty_baudrate.c: fix buffer overrun

On 10/23/18 09:02, [email protected] wrote:
>>
>> As I think Al's big termios cleanups are going to be hitting Linus's
>> tree soon, do you know how these patches interact with that?
>>
>> This patch seems like it will not, so I'll be glad to queue that up
>> after my first round of patches get merged to Linus later this week,
>> but
>> the second one worries me.
>>
>> thanks,
>>
>> greg k-h
>
> I have been working with Al; we had approached much the same problems but from different directions. Mine ended up being a bit more comprehensive as a result, so I think we're going to end up using my code with Al's reviews.
>
> So bottom line is that it should be all good.
>

[Al: Feel free to yell at me if I got that wrong.]

-hpa