* [PATCH v3] vt: keyboard: avoid signed integer overflow in k_ascii
@ 2020-05-25 23:27 Dmitry Torokhov
2020-05-25 23:29 ` Dmitry Torokhov
0 siblings, 1 reply; 2+ messages in thread
From: Dmitry Torokhov @ 2020-05-25 23:27 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Jiri Slaby, linux-kernel
When k_ascii is invoked several times in a row there is a potential for
signed integer overflow:
UBSAN: Undefined behaviour in drivers/tty/vt/keyboard.c:888:19 signed integer overflow:
10 * 1111111111 cannot be represented in type 'int'
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.6.11 #1
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
Call Trace:
<IRQ>
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0xce/0x128 lib/dump_stack.c:118
ubsan_epilogue+0xe/0x30 lib/ubsan.c:154
handle_overflow+0xdc/0xf0 lib/ubsan.c:184
__ubsan_handle_mul_overflow+0x2a/0x40 lib/ubsan.c:205
k_ascii+0xbf/0xd0 drivers/tty/vt/keyboard.c:888
kbd_keycode drivers/tty/vt/keyboard.c:1477 [inline]
kbd_event+0x888/0x3be0 drivers/tty/vt/keyboard.c:1495
While it can be worked around by using check_mul_overflow()/
check_add_overflow(), it is better to introduce a separate flag to
signal that number pad is being used to compose a symbol, and
change type of the accumulator from signed to unsigned, thus
avoiding undefined behavior when it overflows.
Reported-by: Kyungtae Kim <kt0755@gmail.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
- marking the patch as v3 as it is a successor of Kyungtae's patches.
drivers/tty/vt/keyboard.c | 26 ++++++++++++++++----------
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index 15d33fa0c925..568b2171f335 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -127,7 +127,11 @@ static DEFINE_SPINLOCK(func_buf_lock); /* guard 'func_buf' and friends */
static unsigned long key_down[BITS_TO_LONGS(KEY_CNT)]; /* keyboard key bitmap */
static unsigned char shift_down[NR_SHIFT]; /* shift state counters.. */
static bool dead_key_next;
-static int npadch = -1; /* -1 or number assembled on pad */
+
+/* Handles a number being assembled on the number pad */
+static bool npadch_active;
+static unsigned int npadch_value;
+
static unsigned int diacr;
static char rep; /* flag telling character repeat */
@@ -845,12 +849,12 @@ static void k_shift(struct vc_data *vc, unsigned char value, char up_flag)
shift_state &= ~(1 << value);
/* kludge */
- if (up_flag && shift_state != old_state && npadch != -1) {
+ if (up_flag && shift_state != old_state && npadch_active) {
if (kbd->kbdmode == VC_UNICODE)
- to_utf8(vc, npadch);
+ to_utf8(vc, npadch_value);
else
- put_queue(vc, npadch & 0xff);
- npadch = -1;
+ put_queue(vc, npadch_value & 0xff);
+ npadch_active = false;
}
}
@@ -868,7 +872,7 @@ static void k_meta(struct vc_data *vc, unsigned char value, char up_flag)
static void k_ascii(struct vc_data *vc, unsigned char value, char up_flag)
{
- int base;
+ unsigned int base;
if (up_flag)
return;
@@ -882,10 +886,12 @@ static void k_ascii(struct vc_data *vc, unsigned char value, char up_flag)
base = 16;
}
- if (npadch == -1)
- npadch = value;
- else
- npadch = npadch * base + value;
+ if (!npadch_active) {
+ npadch_value = 0;
+ npadch_active = true;
+ }
+
+ npadch_value = npadch_value * base + value;
}
static void k_lock(struct vc_data *vc, unsigned char value, char up_flag)
--
2.27.0.rc0.183.gde8f92d652-goog
--
Dmitry
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v3] vt: keyboard: avoid signed integer overflow in k_ascii
2020-05-25 23:27 [PATCH v3] vt: keyboard: avoid signed integer overflow in k_ascii Dmitry Torokhov
@ 2020-05-25 23:29 ` Dmitry Torokhov
0 siblings, 0 replies; 2+ messages in thread
From: Dmitry Torokhov @ 2020-05-25 23:29 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Jiri Slaby, linux-kernel, Kyungtae Kim
[ Sorry, forgot to CC Kyungtae ]
On Mon, May 25, 2020 at 04:27:40PM -0700, Dmitry Torokhov wrote:
> When k_ascii is invoked several times in a row there is a potential for
> signed integer overflow:
>
> UBSAN: Undefined behaviour in drivers/tty/vt/keyboard.c:888:19 signed integer overflow:
> 10 * 1111111111 cannot be represented in type 'int'
> CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.6.11 #1
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> Call Trace:
> <IRQ>
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0xce/0x128 lib/dump_stack.c:118
> ubsan_epilogue+0xe/0x30 lib/ubsan.c:154
> handle_overflow+0xdc/0xf0 lib/ubsan.c:184
> __ubsan_handle_mul_overflow+0x2a/0x40 lib/ubsan.c:205
> k_ascii+0xbf/0xd0 drivers/tty/vt/keyboard.c:888
> kbd_keycode drivers/tty/vt/keyboard.c:1477 [inline]
> kbd_event+0x888/0x3be0 drivers/tty/vt/keyboard.c:1495
>
> While it can be worked around by using check_mul_overflow()/
> check_add_overflow(), it is better to introduce a separate flag to
> signal that number pad is being used to compose a symbol, and
> change type of the accumulator from signed to unsigned, thus
> avoiding undefined behavior when it overflows.
>
> Reported-by: Kyungtae Kim <kt0755@gmail.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>
> - marking the patch as v3 as it is a successor of Kyungtae's patches.
>
> drivers/tty/vt/keyboard.c | 26 ++++++++++++++++----------
> 1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> index 15d33fa0c925..568b2171f335 100644
> --- a/drivers/tty/vt/keyboard.c
> +++ b/drivers/tty/vt/keyboard.c
> @@ -127,7 +127,11 @@ static DEFINE_SPINLOCK(func_buf_lock); /* guard 'func_buf' and friends */
> static unsigned long key_down[BITS_TO_LONGS(KEY_CNT)]; /* keyboard key bitmap */
> static unsigned char shift_down[NR_SHIFT]; /* shift state counters.. */
> static bool dead_key_next;
> -static int npadch = -1; /* -1 or number assembled on pad */
> +
> +/* Handles a number being assembled on the number pad */
> +static bool npadch_active;
> +static unsigned int npadch_value;
> +
> static unsigned int diacr;
> static char rep; /* flag telling character repeat */
>
> @@ -845,12 +849,12 @@ static void k_shift(struct vc_data *vc, unsigned char value, char up_flag)
> shift_state &= ~(1 << value);
>
> /* kludge */
> - if (up_flag && shift_state != old_state && npadch != -1) {
> + if (up_flag && shift_state != old_state && npadch_active) {
> if (kbd->kbdmode == VC_UNICODE)
> - to_utf8(vc, npadch);
> + to_utf8(vc, npadch_value);
> else
> - put_queue(vc, npadch & 0xff);
> - npadch = -1;
> + put_queue(vc, npadch_value & 0xff);
> + npadch_active = false;
> }
> }
>
> @@ -868,7 +872,7 @@ static void k_meta(struct vc_data *vc, unsigned char value, char up_flag)
>
> static void k_ascii(struct vc_data *vc, unsigned char value, char up_flag)
> {
> - int base;
> + unsigned int base;
>
> if (up_flag)
> return;
> @@ -882,10 +886,12 @@ static void k_ascii(struct vc_data *vc, unsigned char value, char up_flag)
> base = 16;
> }
>
> - if (npadch == -1)
> - npadch = value;
> - else
> - npadch = npadch * base + value;
> + if (!npadch_active) {
> + npadch_value = 0;
> + npadch_active = true;
> + }
> +
> + npadch_value = npadch_value * base + value;
> }
>
> static void k_lock(struct vc_data *vc, unsigned char value, char up_flag)
> --
> 2.27.0.rc0.183.gde8f92d652-goog
>
>
> --
> Dmitry
--
Dmitry
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-05-25 23:29 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-25 23:27 [PATCH v3] vt: keyboard: avoid signed integer overflow in k_ascii Dmitry Torokhov
2020-05-25 23:29 ` Dmitry Torokhov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).