[PATCH] Input: joydev - avoid an undefined shift in the legacy 0x read

[PATCH] Input: joydev - avoid an undefined shift in the legacy 0x read

[Guanglei Zhu]

The legacy 0.x read path shifts the axis values by the calibration
constants that JS_SET_CAL installs:

	data.x = (joydev->abs[0] / 256 + 128) >> joydev->glue.JS_CORR.x;
	data.y = (joydev->abs[1] / 256 + 128) >> joydev->glue.JS_CORR.y;

JS_CORR is copied straight from userspace as three s32 values and
never validated, so the shift count can be negative or wider than
the type, which is undefined behavior: the shift compiles to
whatever the architecture does with an out-of-range count, and the
compiler is free to assume it cannot happen.

Clamp the shift count to the width of the shifted value.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Guanglei Zhu <zhugl3@xiaopeng.com>
---
 drivers/input/joydev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
index 32459fd8a7c1..4740099599bf 100644
--- a/drivers/input/joydev.c
+++ b/drivers/input/joydev.c
@@ -353,8 +353,8 @@ static ssize_t joydev_0x_read(struct joydev_client *client,
 	for (data.buttons = i = 0; i < 32 && i < joydev->nkey; i++)
 		data.buttons |=
 			test_bit(joydev->keypam[i], input->key) ? (1 << i) : 0;
-	data.x = (joydev->abs[0] / 256 + 128) >> joydev->glue.JS_CORR.x;
-	data.y = (joydev->abs[1] / 256 + 128) >> joydev->glue.JS_CORR.y;
+	data.x = (joydev->abs[0] / 256 + 128) >> clamp(joydev->glue.JS_CORR.x, 0, 31);
+	data.y = (joydev->abs[1] / 256 + 128) >> clamp(joydev->glue.JS_CORR.y, 0, 31);
 
 	/*
 	 * Reset reader's event queue
-- 
2.50.1

[PATCH] Input: joydev - avoid signed overflow in the broken-axis correction

[Guanglei Zhu]

The JS_CORR_BROKEN correction multiplies two user-controlled 32-bit
values before shifting and clamping the result:

	value = value > corr->coef[0] ? (value < corr->coef[1] ? 0 :
			((corr->coef[3] * (value - corr->coef[1])) >> 14)) :
			((corr->coef[2] * (value - corr->coef[0])) >> 14);

The coefficients come from JSIOCSCORR without any range check, so
the product can overflow the type, which is undefined behavior: the
compiler is allowed to assume it does not happen and optimize
accordingly.

Do the multiplication in 64 bits, which is bit-identical for every
in-range product.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Guanglei Zhu <zhugl3@xiaopeng.com>
---
 drivers/input/joydev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
index 32459fd8a7c1..947d7671a59f 100644
--- a/drivers/input/joydev.c
+++ b/drivers/input/joydev.c
@@ -74,8 +74,8 @@ static int joydev_correct(int value, struct js_corr *corr)
 
 	case JS_CORR_BROKEN:
 		value = value > corr->coef[0] ? (value < corr->coef[1] ? 0 :
-			((corr->coef[3] * (value - corr->coef[1])) >> 14)) :
-			((corr->coef[2] * (value - corr->coef[0])) >> 14);
+			(((s64)corr->coef[3] * (value - corr->coef[1])) >> 14)) :
+			(((s64)corr->coef[2] * (value - corr->coef[0])) >> 14);
 		break;
 
 	default:
-- 
2.50.1

[PATCH] Input: adi - bound the two-stream packet merge

[Guanglei Zhu]

When a second device shares the wire, adi_move_bits() appends the
second packet to the first one at an offset taken from the
10-bit length field of the ID packet:

	for (i = 1; i <= adi[1].ret; i++)
		adi[0].data[((length - 1) >> 1) + i + 1] = adi[1].data[i];

	adi[0].ret += adi[1].ret;

Nothing compares the merged packet against the size of the buffer:
adi[0].data is the last member of struct adi, and struct adi_port
holds two of them back to back, so an oversized length field makes
the loop write over the second struct adi - its device pointer, its
length and its packet - and the inflated ret later walks the reads
past the buffer as well. The connect path also calls
adi_move_bits() before the length has been validated at all.

Reject a merge that does not fit in the buffer.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Guanglei Zhu <zhugl3@xiaopeng.com>
---
 drivers/input/joystick/adi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/input/joystick/adi.c b/drivers/input/joystick/adi.c
index e07b71978509..03535e58d6a6 100644
--- a/drivers/input/joystick/adi.c
+++ b/drivers/input/joystick/adi.c
@@ -168,6 +168,9 @@ static void adi_move_bits(struct adi_port *port, int length)
 
 	if (adi[0].ret <= 0 || adi[1].ret <= 0) return;
 	if (adi[0].data[0] & 0x20 || ~adi[1].data[0] & 0x20) return;
+	if (adi[0].ret + adi[1].ret > ADI_MAX_LENGTH ||
+	    ((length - 1) >> 1) + adi[1].ret + 1 >= ADI_MAX_LENGTH)
+		return;
 
 	for (i = 1; i <= adi[1].ret; i++)
 		adi[0].data[((length - 1) >> 1) + i + 1] = adi[1].data[i];
-- 
2.50.1

[PATCH] Input: db9 - enlarge the Saturn multitap packet buffer

[Guanglei Zhu]

The Saturn multitap path fills the packet buffer block by block:

	for (i = 0; i < 60; i += 10) {
		data[i] = db9_saturn_read_analog(port, type, powered);
		if (data[i] != 0xff)
			/* read each pad */
			for (j = 0; j < (data[i] & 0x0f); j++)
				data[i + j + 1] = db9_saturn_read_analog(port, type, powered);
	}

Each block is ten bytes apart, and the low nibble of the block
header - taken from the device - allows up to fifteen more bytes,
so the last block can write up to data[65]. The buffer, however,
is sixty bytes, and the six bytes past it belong to the rest of
the stack frame.

Give the buffer the size the protocol layout actually requires.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Guanglei Zhu <zhugl3@xiaopeng.com>
---
 drivers/input/joystick/db9.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/input/joystick/db9.c b/drivers/input/joystick/db9.c
index e5d9b71aab99..73d1e7a296bb 100644
--- a/drivers/input/joystick/db9.c
+++ b/drivers/input/joystick/db9.c
@@ -253,7 +253,8 @@ static unsigned char db9_saturn_read_packet(struct parport *port, unsigned char
 /*
  * db9_saturn_report() analyzes packet and reports.
  */
-static int db9_saturn_report(unsigned char id, unsigned char data[60], struct input_dev *devs[], int n, int max_pads)
+static int db9_saturn_report(unsigned char id, unsigned char data[66],
+			      struct input_dev *devs[], int n, int max_pads)
 {
 	struct input_dev *dev;
 	int tmp, i, j;
@@ -321,7 +322,7 @@ static int db9_saturn_report(unsigned char id, unsigned char data[60], struct in
 
 static int db9_saturn(int mode, struct parport *port, struct input_dev *devs[])
 {
-	unsigned char id, data[60];
+	unsigned char id, data[66];
 	int type, n, max_pads;
 	int tmp, i;
 
-- 
2.50.1

[PATCH] Input: lpc32xx-keys - reject matrices bigger than the hardware has

[Guanglei Zhu]

The scanner state array holds the last state of eight matrix
columns, and the interrupt handler walks the matrix size that the
device tree declared:

	u8 lastkeystates[8];
	...
	for (i = 0; i < kscandat->matrix_sz; i++)
		lpc32xx_mod_states(kscandat, i);

The probe path only checks that rows and columns match; the
hardware supports square matrices from 1x1 up to 8x8, but nothing
stops a device tree from declaring more, which makes the state
array index run past the field into the rest of the driver data -
including the keymap pointer the same code dereferences.

Reject matrices larger than the hardware supports.

Fixes: 69690bec400e ("Input: add support for key scan interface of the LPC32xx SoC")
Cc: stable@vger.kernel.org
Signed-off-by: Guanglei Zhu <zhugl3@xiaopeng.com>
---
 drivers/input/keyboard/lpc32xx-keys.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/input/keyboard/lpc32xx-keys.c b/drivers/input/keyboard/lpc32xx-keys.c
index 285d5c32fe70..1d0b082e7881 100644
--- a/drivers/input/keyboard/lpc32xx-keys.c
+++ b/drivers/input/keyboard/lpc32xx-keys.c
@@ -142,6 +142,11 @@ static int lpc32xx_parse_dt(struct device *dev,
 		return -EINVAL;
 	}
 
+	if (rows > 8) {
+		dev_err(dev, "rows and columns must be at most 8!\n");
+		return -EINVAL;
+	}
+
 	kscandat->matrix_sz = rows;
 	kscandat->row_shift = get_count_order(columns);
 
-- 
2.50.1

[PATCH] Input: mt6779-keypad - check the reported row and column

[Guanglei Zhu]

The interrupt handler turns register bits into a matrix position
and indexes the keycode array with it:

		keypad->calc_row_col(key, &row, &col);

		scancode = MATRIX_SCAN_CODE(row, col, row_shift);
		...
		input_report_key(keypad->input_dev, keycode[scancode], pressed);

The keycode array is sized after the rows and columns the device
tree declared, but neither the probe nor the handler checks that a
reported position actually fits those dimensions. A register glitch
or a matrix smaller than the hardware's fixed layout indexes past
the array and reports whatever sits there as a key code.

Skip positions outside the declared matrix.

Fixes: f28af984e771 ("Input: mt6779-keypad - add MediaTek keypad driver")
Cc: stable@vger.kernel.org
Signed-off-by: Guanglei Zhu <zhugl3@xiaopeng.com>
---
 drivers/input/keyboard/mt6779-keypad.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/input/keyboard/mt6779-keypad.c b/drivers/input/keyboard/mt6779-keypad.c
index e5eb025c5e99..effd154f5bf9 100644
--- a/drivers/input/keyboard/mt6779-keypad.c
+++ b/drivers/input/keyboard/mt6779-keypad.c
@@ -73,6 +73,9 @@ static irqreturn_t mt6779_keypad_irq_handler(int irq, void *dev_id)
 		key = bit_nr / 32 * 16 + bit_nr % 32;
 		keypad->calc_row_col(key, &row, &col);
 
+		if (row >= keypad->n_rows || col >= keypad->n_cols)
+			continue;
+
 		scancode = MATRIX_SCAN_CODE(row, col, row_shift);
 		/* 1: not pressed, 0: pressed */
 		pressed = !test_bit(bit_nr, new_state);
-- 
2.50.1

[PATCH] Input: tegra-kbc - check the pin numbers from the device tree

[Guanglei Zhu]

The row and column pin lists are read straight from the device tree
and used to index the fixed pin configuration array:

	num_rows = of_property_read_variable_u32_array(np, "nvidia,kbc-row-pins",
				rows_cfg, 1, KBC_MAX_GPIO);
	...
	for (i = 0; i < num_rows; i++) {
		kbc->pin_cfg[rows_cfg[i]].type = PIN_CFG_ROW;
		kbc->pin_cfg[rows_cfg[i]].num = i;
	}

The helper only limits how many entries it reads, not what they
contain, and the pin_cfg array holds KBC_MAX_GPIO entries, so a pin
number from the device tree indexes past the array into the rest
of the driver data. The later configuration check validates the
row and column counts stored this way, not the index itself, so it
cannot undo the write.

Reject pin numbers the array does not have.

Fixes: 88390243a993 ("Input: tegra-kbc - add support for rows/columns configuration from dt")
Cc: stable@vger.kernel.org
Signed-off-by: Guanglei Zhu <zhugl3@xiaopeng.com>
---
 drivers/input/keyboard/tegra-kbc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c
index e6177f2bdf8d..f086ccf1a075 100644
--- a/drivers/input/keyboard/tegra-kbc.c
+++ b/drivers/input/keyboard/tegra-kbc.c
@@ -522,6 +522,10 @@ static int tegra_kbc_parse_dt(struct tegra_kbc *kbc)
 	}
 
 	for (i = 0; i < num_rows; i++) {
+		if (rows_cfg[i] >= KBC_MAX_GPIO) {
+			dev_err(kbc->dev, "invalid row pin %u\n", rows_cfg[i]);
+			return -EINVAL;
+		}
 		kbc->pin_cfg[rows_cfg[i]].type = PIN_CFG_ROW;
 		kbc->pin_cfg[rows_cfg[i]].num = i;
 	}
@@ -538,6 +542,10 @@ static int tegra_kbc_parse_dt(struct tegra_kbc *kbc)
 	}
 
 	for (i = 0; i < num_cols; i++) {
+		if (cols_cfg[i] >= KBC_MAX_GPIO) {
+			dev_err(kbc->dev, "invalid col pin %u\n", cols_cfg[i]);
+			return -EINVAL;
+		}
 		kbc->pin_cfg[cols_cfg[i]].type = PIN_CFG_COL;
 		kbc->pin_cfg[cols_cfg[i]].num = i;
 	}
-- 
2.50.1

[PATCH] Input: cyapa_gen3 - clamp the reported finger count to the array

[Guanglei Zhu]

The finger count is taken from the top nibble of a status byte the
trackpad firmware fills in, and used to walk the touch array:

	num_fingers = (data->finger_btn >> 4) & 0x0f;
	for (i = 0; i < num_fingers; i++) {
		const struct cyapa_touch *touch = &data->touches[i];

The nibble allows up to fifteen fingers, but the array holds the
five touches the chip is documented to report, so a firmware that
counts beyond that makes the driver report coordinates read from
the stack past the register block.

Clamp the finger count to the size of the array.

Fixes: 9f1cd857518f ("Input: cyapa - re-design driver to support multi-trackpad in one driver")
Cc: stable@vger.kernel.org
Signed-off-by: Guanglei Zhu <zhugl3@xiaopeng.com>
---
 drivers/input/mouse/cyapa_gen3.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/input/mouse/cyapa_gen3.c b/drivers/input/mouse/cyapa_gen3.c
index fc3fb954523b..fda807a4c141 100644
--- a/drivers/input/mouse/cyapa_gen3.c
+++ b/drivers/input/mouse/cyapa_gen3.c
@@ -1149,7 +1149,8 @@ static int cyapa_gen3_event_process(struct cyapa *cyapa,
 	int num_fingers;
 	int i;
 
-	num_fingers = (data->finger_btn >> 4) & 0x0f;
+	num_fingers = min((data->finger_btn >> 4) & 0x0f,
+			  ARRAY_SIZE(data->touches));
 	for (i = 0; i < num_fingers; i++) {
 		const struct cyapa_touch *touch = &data->touches[i];
 		/* Note: touch->id range is 1 to 15; slots are 0 to 14. */
-- 
2.50.1

[PATCH v2] Input: joydev - avoid signed overflow in the broken-axis correction

[Guanglei Zhu]

The JS_CORR_BROKEN correction multiplies two user-controlled 32-bit
values before shifting and clamping the result:

	value = value > corr->coef[0] ? (value < corr->coef[1] ? 0 :
			((corr->coef[3] * (value - corr->coef[1])) >> 14)) :
			((corr->coef[2] * (value - corr->coef[0])) >> 14);

The coefficients come from JSIOCSCORR without any range check, so
three things can go wrong: the subtractions can overflow 32 bits
(value = 0 against coef[1] = INT_MIN), the products can overflow the
type, and a product that survives the multiplication in 64 bits can
still wrap when the shifted result is assigned back to the int
variable, which then slips through clamp() as a small value instead
of saturating.

Compute the whole correction in 64 bits, including the subtractions,
and clamp with clamp_t(s64, ...) before narrowing, which is
bit-identical for every in-range input.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Guanglei Zhu <zhugl3@xiaopeng.com>
---
v2: per review, extend the 64-bit computation to cover the subtractions
    ((s64)value - coef, which could overflow 32 bits before the
    promotion in v1) and keep the shifted product in s64 through
    clamp_t(s64, ...) so an out-of-range result saturates instead of
    wrapping during the narrowing to int.

 drivers/input/joydev.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
index 32459fd..18aa9ce 100644
--- a/drivers/input/joydev.c
+++ b/drivers/input/joydev.c
@@ -72,11 +72,15 @@ static int joydev_correct(int value, struct js_corr *corr)
 	case JS_CORR_NONE:
 		break;
 
-	case JS_CORR_BROKEN:
-		value = value > corr->coef[0] ? (value < corr->coef[1] ? 0 :
-			((corr->coef[3] * (value - corr->coef[1])) >> 14)) :
-			((corr->coef[2] * (value - corr->coef[0])) >> 14);
+	case JS_CORR_BROKEN: {
+		s64 tmp = value > corr->coef[0] ?
+			(value < corr->coef[1] ? 0 :
+			 (s64)corr->coef[3] * ((s64)value - corr->coef[1])) :
+			(s64)corr->coef[2] * ((s64)value - corr->coef[0]);
+
+		value = clamp_t(s64, tmp >> 14, -32767, 32767);
 		break;
+	}
 
 	default:
 		return 0;
-- 
2.43.0

[PATCH v2] Input: adi - bound the two-stream packet merge

[Guanglei Zhu]

When a second device shares the wire, adi_move_bits() appends the
second packet to the first one at an offset taken from the
10-bit length field of the ID packet:

	for (i = 1; i <= adi[1].ret; i++)
		adi[0].data[((length - 1) >> 1) + i + 1] = adi[1].data[i];

	adi[0].ret += adi[1].ret;

Nothing compares the merged packet against the size of the buffer:
adi[0].data is the last member of struct adi, and struct adi_port
holds two of them back to back, so an oversized length field makes
the loop write over the second struct adi - its device pointer, its
length and its packet - and the inflated ret later walks the reads
past the buffer as well. The connect path also calls
adi_move_bits() before the length has been validated at all.

Reject a merge that does not fit in the buffer. Use >= for the
combined length so adi_get_bits() can never walk to data[ret] with
ret == ADI_MAX_LENGTH, and invalidate both half-streams on the
rejection so the unmerged packets are not decoded as standalone
reads.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Guanglei Zhu <zhugl3@xiaopeng.com>
---
v2: per review, use >= so a combined ret of ADI_MAX_LENGTH is rejected
    as well (adi_get_bits() allows idx == ret, and data[idx] with
    idx == ADI_MAX_LENGTH reads one byte past the array), and set
    adi[0].ret = adi[1].ret = -1 on the rejection path so the
    unmerged half-streams are not decoded as standalone packets.

 drivers/input/joystick/adi.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/input/joystick/adi.c b/drivers/input/joystick/adi.c
index e07b719..41262b7 100644
--- a/drivers/input/joystick/adi.c
+++ b/drivers/input/joystick/adi.c
@@ -168,6 +168,11 @@ static void adi_move_bits(struct adi_port *port, int length)
 
 	if (adi[0].ret <= 0 || adi[1].ret <= 0) return;
 	if (adi[0].data[0] & 0x20 || ~adi[1].data[0] & 0x20) return;
+	if (adi[0].ret + adi[1].ret >= ADI_MAX_LENGTH ||
+	    ((length - 1) >> 1) + adi[1].ret + 1 >= ADI_MAX_LENGTH) {
+		adi[0].ret = adi[1].ret = -1;
+		return;
+	}
 
 	for (i = 1; i <= adi[1].ret; i++)
 		adi[0].data[((length - 1) >> 1) + i + 1] = adi[1].data[i];
-- 
2.43.0