Buffer overrun in the TWL4030 keypad driver with Nokia RX51

Buffer overrun in the TWL4030 keypad driver with Nokia RX51

[Laurent Pinchart]

Hi everybody,

I've spent the day debugging a kernel crash in the USB networking code to find 
out the problem was caused by a buffer overrun in the TWL4030 keypad driver. 

The Nokia RX51 board code (arch/arm/mach-omap2/board-rx51-peripherals.c) 
defines a key map for the matrix keypad keyboard. The hardware seems to use 
all of the 8 rows and 8 columns of the keypad, although not all possible 
locations are used.

The TWL4030 supports keypads with at most 8 rows and 8 columns. Most keys are 
defined with a row and column number between 0 and 7, except

        KEY(0xff, 2, KEY_F9),
        KEY(0xff, 4, KEY_F10),
        KEY(0xff, 5, KEY_F11),

The row number is set to 0xff. As the generic matrix keypad support 
(include/linux/input/matrix_keypad.h) supports at most 16 rows and 16 columns, 
it masks all but the lower 4 bits of the row and column numbers in the KEY 
macro.

#define MATRIX_MAX_ROWS         16
#define MATRIX_MAX_COLS         16

#define KEY(row, col, val)      ((((row) & (MATRIX_MAX_ROWS - 1)) << 24) |\
                                 (((col) & (MATRIX_MAX_COLS - 1)) << 16) |\
                                 (val & 0xffff))

This leads to an effective row number equal to 15.

The TWL4030 keypad driver (drivers/input/keyboard/twl4030_keypad.c) allocates 
in twl4030_kp_probe a 8x8 keycodes map, part of the twl4030_keypad structure.

#define TWL4030_MAX_ROWS        8       /* TWL4030 hard limit */
#define TWL4030_MAX_COLS        8
#define TWL4030_ROW_SHIFT       3
#define TWL4030_KEYMAP_SIZE     (TWL4030_MAX_ROWS * TWL4030_MAX_COLS)

struct twl4030_keypad {
        unsigned short  keymap[TWL4030_KEYMAP_SIZE];
...

It then calls matrix_keypad_build_keymap (include/linux/input/matrix_keypad.h) 
to initialize the keycodes map from the keymap defined in platform data. The 
function loops over all the keymap platform data entries, and initializes the 
corresponding keycodes map entry. The entry index is computed with

#define MATRIX_SCAN_CODE(row, col, row_shift)   (((row) << (row_shift)) + 
(col))

called with row_shift set to TWL4030_ROW_SHIFT, defined as 3.

For the 3 keys defined with a row equal to 0xff, the map entry index is then 
equal to (15 << 3) + col, which is bigger than the number of keycodes map 
entries in the twl4030_keypad structure. Writing to that invalid index 
overwrites random memory.

The 0xff row number is used to detect rows completely connected to ground (see 
the comment in twl4030_col_xlate, drivers/input/keyboard/twl4030_keypad.c). 
The related code in twl4030_col_xlate is obviously wrong, returning a column 
number too large for the allocated keycodes map if the hardware keypad has 8 
columns.

I can try to provide a patch, but I'm not familiar enough with the TWL4030 
keypad driver and the generic matrix keypad code to know what the best fix 
would be. Hopefully this should be quite clear for the TWL4030 keypad driver 
developers.

-- 
Regards,

Laurent Pinchart

Re: Buffer overrun in the TWL4030 keypad driver with Nokia RX51

[Dmitry Torokhov]

Hi Laurent.

On Fri, Jul 16, 2010 at 05:28:43PM +0200, Laurent Pinchart wrote:
> Hi everybody,
> 
> I've spent the day debugging a kernel crash in the USB networking code to find 
> out the problem was caused by a buffer overrun in the TWL4030 keypad driver. 
> 
> The Nokia RX51 board code (arch/arm/mach-omap2/board-rx51-peripherals.c) 
> defines a key map for the matrix keypad keyboard. The hardware seems to use 
> all of the 8 rows and 8 columns of the keypad, although not all possible 
> locations are used.
> 
> The TWL4030 supports keypads with at most 8 rows and 8 columns. Most keys are 
> defined with a row and column number between 0 and 7, except
> 
>         KEY(0xff, 2, KEY_F9),
>         KEY(0xff, 4, KEY_F10),
>         KEY(0xff, 5, KEY_F11),
> 
> The row number is set to 0xff. As the generic matrix keypad support 
> (include/linux/input/matrix_keypad.h) supports at most 16 rows and 16 columns, 
> it masks all but the lower 4 bits of the row and column numbers in the KEY 
> macro.

[..snipped..]

Thanks for the report. Could yo uplease try the patch below and let me
know if it works.

I have some concerns with the keymap assignments, I see that Amit
changed them during KEY(col, row) -> KEY(row, col) conversion. I marked
the entries I am concerned with with XXX.

Thanks.

-- 
Dmitry

Input: twl40300-keypad - fix handling of "all ground" rows

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

The Nokia RX51 board code (arch/arm/mach-omap2/board-rx51-peripherals.c)
defines a key map for the matrix keypad keyboard. The hardware seems to
use all of the 8 rows and 8 columns of the keypad, although not all
possible locations are used.

The TWL4030 supports keypads with at most 8 rows and 8 columns. Most keys
are defined with a row and column number between 0 and 7, except

        KEY(0xff, 2, KEY_F9),
        KEY(0xff, 4, KEY_F10),
        KEY(0xff, 5, KEY_F11),

which represent keycodes that should be emitted when entire row is
connected to the ground. The driver handles this case as if we had an
extra column in the key matrix. Unfortunately we do not allocate enough
space and end up owerwriting some random memory.

Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 arch/arm/mach-omap2/board-rx51-peripherals.c |   28 ++++++++++++++++++++------
 drivers/input/keyboard/twl4030_keypad.c      |   17 ++++++++++------
 2 files changed, 32 insertions(+), 13 deletions(-)


diff --git a/arch/arm/mach-omap2/board-rx51-peripherals.c b/arch/arm/mach-omap2/board-rx51-peripherals.c
index abdf321..ea32143 100644
--- a/arch/arm/mach-omap2/board-rx51-peripherals.c
+++ b/arch/arm/mach-omap2/board-rx51-peripherals.c
@@ -175,6 +175,10 @@ static void __init rx51_add_gpio_keys(void)
 #endif /* CONFIG_KEYBOARD_GPIO || CONFIG_KEYBOARD_GPIO_MODULE */
 
 static int board_keymap[] = {
+	/*
+	 * Note that KEY(x, 8, KEY_XXX) entries represent "entire row
+	 * connected to the ground" matrix state.
+	 */
 	KEY(0, 0, KEY_Q),
 	KEY(0, 1, KEY_O),
 	KEY(0, 2, KEY_P),
@@ -182,6 +186,8 @@ static int board_keymap[] = {
 	KEY(0, 4, KEY_BACKSPACE),
 	KEY(0, 6, KEY_A),
 	KEY(0, 7, KEY_S),
+//	KEY(0, 8, KEY_F6),	// XXX was removed
+
 	KEY(1, 0, KEY_W),
 	KEY(1, 1, KEY_D),
 	KEY(1, 2, KEY_F),
@@ -190,6 +196,8 @@ static int board_keymap[] = {
 	KEY(1, 5, KEY_J),
 	KEY(1, 6, KEY_K),
 	KEY(1, 7, KEY_L),
+//	KEY(1, 8, KEY_F7),	// XXX was moved to (7, 1)
+
 	KEY(2, 0, KEY_E),
 	KEY(2, 1, KEY_DOT),
 	KEY(2, 2, KEY_UP),
@@ -197,6 +205,8 @@ static int board_keymap[] = {
 	KEY(2, 5, KEY_Z),
 	KEY(2, 6, KEY_X),
 	KEY(2, 7, KEY_C),
+	KEY(2, 8, KEY_F9),	// XXX was F8 and moved to (7, 2), F9 was (4, 8)
+
 	KEY(3, 0, KEY_R),
 	KEY(3, 1, KEY_V),
 	KEY(3, 2, KEY_B),
@@ -205,20 +215,24 @@ static int board_keymap[] = {
 	KEY(3, 5, KEY_SPACE),
 	KEY(3, 6, KEY_SPACE),
 	KEY(3, 7, KEY_LEFT),
+
 	KEY(4, 0, KEY_T),
 	KEY(4, 1, KEY_DOWN),
 	KEY(4, 2, KEY_RIGHT),
 	KEY(4, 4, KEY_LEFTCTRL),
-	KEY(4, 5, KEY_RIGHTALT),
-	KEY(4, 6, KEY_LEFTSHIFT),
+	KEY(4, 5, KEY_RIGHTALT),	// XXX was LEFTSHIFT
+	KEY(4, 6, KEY_LEFTSHIFT),	// XXX was FN which is now removed
+	KEY(4, 8, KEY_10),		// XXX was F9 and moved to (2, 8), F10 was (5, 8)
+
+
 	KEY(5, 0, KEY_Y),
+	KEY(5, 8, KEY_11),		// XXX was F10 and moved to (4, 8), F11 is new
+
 	KEY(6, 0, KEY_U),
+
 	KEY(7, 0, KEY_I),
-	KEY(7, 1, KEY_F7),
-	KEY(7, 2, KEY_F8),
-	KEY(0xff, 2, KEY_F9),
-	KEY(0xff, 4, KEY_F10),
-	KEY(0xff, 5, KEY_F11),
+	KEY(7, 1, KEY_F7),		// XXX was undefined
+	KEY(7, 2, KEY_F8),		// XXX was undefined
 };
 
 static struct matrix_keymap_data board_map_data = {
diff --git a/drivers/input/keyboard/twl4030_keypad.c b/drivers/input/keyboard/twl4030_keypad.c
index 7aa59e0..fb16b5e 100644
--- a/drivers/input/keyboard/twl4030_keypad.c
+++ b/drivers/input/keyboard/twl4030_keypad.c
@@ -51,8 +51,12 @@
  */
 #define TWL4030_MAX_ROWS	8	/* TWL4030 hard limit */
 #define TWL4030_MAX_COLS	8
-#define TWL4030_ROW_SHIFT	3
-#define TWL4030_KEYMAP_SIZE	(TWL4030_MAX_ROWS * TWL4030_MAX_COLS)
+/*
+ * Note that we add space for an extra column so that we can handle
+ * row lines connected to the gnd (see twl4030_col_xlate()).
+ */
+#define TWL4030_ROW_SHIFT	4
+#define TWL4030_KEYMAP_SIZE	(TWL4030_MAX_ROWS << TWL4030_ROW_SHIFT)
 
 struct twl4030_keypad {
 	unsigned short	keymap[TWL4030_KEYMAP_SIZE];
@@ -182,7 +186,7 @@ static int twl4030_read_kp_matrix_state(struct twl4030_keypad *kp, u16 *state)
 	return ret;
 }
 
-static int twl4030_is_in_ghost_state(struct twl4030_keypad *kp, u16 *key_state)
+static bool twl4030_is_in_ghost_state(struct twl4030_keypad *kp, u16 *key_state)
 {
 	int i;
 	u16 check = 0;
@@ -191,12 +195,12 @@ static int twl4030_is_in_ghost_state(struct twl4030_keypad *kp, u16 *key_state)
 		u16 col = key_state[i];
 
 		if ((col & check) && hweight16(col) > 1)
-			return 1;
+			return true;
 
 		check |= col;
 	}
 
-	return 0;
+	return false;
 }
 
 static void twl4030_kp_scan(struct twl4030_keypad *kp, bool release_all)
@@ -225,7 +229,8 @@ static void twl4030_kp_scan(struct twl4030_keypad *kp, bool release_all)
 		if (!changed)
 			continue;
 
-		for (col = 0; col < kp->n_cols; col++) {
+		/* Extra column handles "all gnd" rows */
+		for (col = 0; col < kp->n_cols + 1; col++) {
 			int code;
 
 			if (!(changed & (1 << col)))

Re: Buffer overrun in the TWL4030 keypad driver with Nokia RX51

[Laurent Pinchart]

Hi Dmitry,

On Saturday 17 July 2010 23:37:05 Dmitry Torokhov wrote:
> On Fri, Jul 16, 2010 at 05:28:43PM +0200, Laurent Pinchart wrote:
> > 
> > I've spent the day debugging a kernel crash in the USB networking code to
> > find out the problem was caused by a buffer overrun in the TWL4030
> > keypad driver.
> > 
> > The Nokia RX51 board code (arch/arm/mach-omap2/board-rx51-peripherals.c)
> > defines a key map for the matrix keypad keyboard. The hardware seems to
> > use all of the 8 rows and 8 columns of the keypad, although not all
> > possible locations are used.
> > 
> > The TWL4030 supports keypads with at most 8 rows and 8 columns. Most keys
> > are defined with a row and column number between 0 and 7, except
> > 
> >         KEY(0xff, 2, KEY_F9),
> >         KEY(0xff, 4, KEY_F10),
> >         KEY(0xff, 5, KEY_F11),
> > 
> > The row number is set to 0xff. As the generic matrix keypad support
> > (include/linux/input/matrix_keypad.h) supports at most 16 rows and 16
> > columns, it masks all but the lower 4 bits of the row and column numbers
> > in the KEY macro.
> 
> [..snipped..]
> 
> Thanks for the report. Could yo uplease try the patch below and let me
> know if it works.

The patch fixes the crash at startup, but the F9, F10 and F11 key events are 
never reported. That might be because those keys are not wired up to anything 
though. All keys on the keyboard, as well as the F7 and F8 keys (volume up and 
down on the of the case) generate the proper events. All other "keys" 
(keyboard slider switch, power button, focus button, lock switch, proximity 
sensor) report events through other devices.

> I have some concerns with the keymap assignments, I see that Amit
> changed them during KEY(col, row) -> KEY(row, col) conversion. I marked
> the entries I am concerned with with XXX.

F7, F8, right alt and left shift are properly mapped. I don't know what F9, 
F10 and F11 are supposed to be.

-- 
Regards,

Laurent Pinchart

Re: Buffer overrun in the TWL4030 keypad driver with Nokia RX51

[Dmitry Torokhov]

On Tuesday, July 20, 2010 04:06:08 am Laurent Pinchart wrote:
> Hi Dmitry,
> 
> On Saturday 17 July 2010 23:37:05 Dmitry Torokhov wrote:
> > On Fri, Jul 16, 2010 at 05:28:43PM +0200, Laurent Pinchart wrote:
> > > I've spent the day debugging a kernel crash in the USB networking code
> > > to find out the problem was caused by a buffer overrun in the TWL4030
> > > keypad driver.
> > > 
> > > The Nokia RX51 board code
> > > (arch/arm/mach-omap2/board-rx51-peripherals.c) defines a key map for
> > > the matrix keypad keyboard. The hardware seems to use all of the 8
> > > rows and 8 columns of the keypad, although not all possible locations
> > > are used.
> > > 
> > > The TWL4030 supports keypads with at most 8 rows and 8 columns. Most
> > > keys are defined with a row and column number between 0 and 7, except
> > > 
> > >         KEY(0xff, 2, KEY_F9),
> > >         KEY(0xff, 4, KEY_F10),
> > >         KEY(0xff, 5, KEY_F11),
> > > 
> > > The row number is set to 0xff. As the generic matrix keypad support
> > > (include/linux/input/matrix_keypad.h) supports at most 16 rows and 16
> > > columns, it masks all but the lower 4 bits of the row and column
> > > numbers in the KEY macro.
> > 
> > [..snipped..]
> > 
> > Thanks for the report. Could yo uplease try the patch below and let me
> > know if it works.
> 
> The patch fixes the crash at startup,

Great.

> but the F9, F10 and F11 key events
> are never reported. That might be because those keys are not wired up to
> anything though.

I would not know... If you see all keys on the device being handled then I 
guess it's the case... Tony, Amit, any ideas?

> All keys on the keyboard, as well as the F7 and F8 keys
> (volume up and down on the of the case)

I guess we need to remap them to KEY_VOLUMEUP and KEY_VOLUMEDOWN then. So is 
F7 == Up and F8 == Down?

> generate the proper events. All
> other "keys" (keyboard slider switch, power button, focus button, lock
> switch, proximity sensor) report events through other devices.
> 
> > I have some concerns with the keymap assignments, I see that Amit
> > changed them during KEY(col, row) -> KEY(row, col) conversion. I marked
> > the entries I am concerned with with XXX.
> 
> F7, F8, right alt and left shift are properly mapped. I don't know what F9,
> F10 and F11 are supposed to be.

OK, thanks. Since you don't crash anymore I think it is worth pushing
it out.

-- 
Dmitry

Re: Buffer overrun in the TWL4030 keypad driver with Nokia RX51

[Laurent Pinchart]

Hi Dmitry,

On Tuesday 20 July 2010 20:07:45 Dmitry Torokhov wrote:
> On Tuesday, July 20, 2010 04:06:08 am Laurent Pinchart wrote:
> > On Saturday 17 July 2010 23:37:05 Dmitry Torokhov wrote:
> > > On Fri, Jul 16, 2010 at 05:28:43PM +0200, Laurent Pinchart wrote:
> > > > I've spent the day debugging a kernel crash in the USB networking
> > > > code to find out the problem was caused by a buffer overrun in the
> > > > TWL4030 keypad driver.
> > > > 
> > > > The Nokia RX51 board code
> > > > (arch/arm/mach-omap2/board-rx51-peripherals.c) defines a key map for
> > > > the matrix keypad keyboard. The hardware seems to use all of the 8
> > > > rows and 8 columns of the keypad, although not all possible locations
> > > > are used.
> > > > 
> > > > The TWL4030 supports keypads with at most 8 rows and 8 columns. Most
> > > > keys are defined with a row and column number between 0 and 7, except
> > > > 
> > > >         KEY(0xff, 2, KEY_F9),
> > > >         KEY(0xff, 4, KEY_F10),
> > > >         KEY(0xff, 5, KEY_F11),
> > > > 
> > > > The row number is set to 0xff. As the generic matrix keypad support
> > > > (include/linux/input/matrix_keypad.h) supports at most 16 rows and 16
> > > > columns, it masks all but the lower 4 bits of the row and column
> > > > numbers in the KEY macro.
> > > 
> > > [..snipped..]
> > > 
> > > Thanks for the report. Could yo uplease try the patch below and let me
> > > know if it works.
> > 
> > The patch fixes the crash at startup,
> 
> Great.
> 
> > but the F9, F10 and F11 key events
> > are never reported. That might be because those keys are not wired up to
> > anything though.
> 
> I would not know... If you see all keys on the device being handled then I
> guess it's the case... Tony, Amit, any ideas?
> 
> > All keys on the keyboard, as well as the F7 and F8 keys
> > (volume up and down on the of the case)
> 
> I guess we need to remap them to KEY_VOLUMEUP and KEY_VOLUMEDOWN then. So
> is F7 == Up and F8 == Down?

It's the other way around. F7 is volume down, F8 is volume up. Note that the 
same keys have different meanings depending on the context. In the photo 
viewer, they are used to zoom (volume down zooms out, volume up zooms in).

> > generate the proper events. All other "keys" (keyboard slider switch,
> > power button, focus button, lock switch, proximity sensor) report events
> > through other devices.
> > 
> > > I have some concerns with the keymap assignments, I see that Amit
> > > changed them during KEY(col, row) -> KEY(row, col) conversion. I marked
> > > the entries I am concerned with with XXX.
> > 
> > F7, F8, right alt and left shift are properly mapped. I don't know what
> > F9, F10 and F11 are supposed to be.
> 
> OK, thanks. Since you don't crash anymore I think it is worth pushing
> it out.

-- 
Regards,

Laurent Pinchart