[PATCH v2 next] HID: hid-goodix: Fix type promotion bug in goodix_hid_get_raw_report()

[PATCH v2 next] HID: hid-goodix: Fix type promotion bug in goodix_hid_get_raw_report()

[Dan Carpenter]

The issue is GOODIX_HID_PKG_LEN_SIZE is defined as sizeof(u16) which is
type size_t.  However, goodix_hid_check_ack_status() returns negative
error codes or potentially a positive but invalid length which is too
small.  So when we compare "if ((response_data_len <=
GOODIX_HID_PKG_LEN_SIZE)" then negative error codes are type promoted to
size_t and counted as a positive large value and treated as valid.

It would have been easy enough to add some casting to avoid the type
promotion, however this patch takes a more thourough approach and moves
the length check into goodix_hid_check_ack_status().  Now the function
only return negative error codes or zero on success and the length
pointer is never set to an invalid length.

Fixes: 75e16c8ce283 ("HID: hid-goodix: Add Goodix HID-over-SPI driver")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
v2: take a different approach

 drivers/hid/hid-goodix-spi.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-goodix-spi.c b/drivers/hid/hid-goodix-spi.c
index 5103bf0aada4..de655f745d3f 100644
--- a/drivers/hid/hid-goodix-spi.c
+++ b/drivers/hid/hid-goodix-spi.c
@@ -335,11 +335,12 @@ static void goodix_hid_close(struct hid_device *hid)
 }
 
 /* Return date length of response data */
-static int goodix_hid_check_ack_status(struct goodix_ts_data *ts)
+static int goodix_hid_check_ack_status(struct goodix_ts_data *ts, u32 *resp_len)
 {
 	struct goodix_hid_report_header hdr;
 	int retry = 20;
 	int error;
+	int len;
 
 	while (retry--) {
 		/*
@@ -349,8 +350,15 @@ static int goodix_hid_check_ack_status(struct goodix_ts_data *ts)
 		 */
 		error = goodix_spi_read(ts, ts->hid_report_addr,
 					&hdr, sizeof(hdr));
-		if (!error && (hdr.flag & GOODIX_HID_ACK_READY_FLAG))
-			return le16_to_cpu(hdr.size);
+		if (!error && (hdr.flag & GOODIX_HID_ACK_READY_FLAG)) {
+			len = le16_to_cpu(hdr.size);
+			if (len < GOODIX_HID_PKG_LEN_SIZE) {
+				dev_err(ts->dev, "hrd.size too short: %d", len);
+				return -EINVAL;
+			}
+			*resp_len = len;
+			return 0;
+		}
 
 		/* Wait 10ms for another try */
 		usleep_range(10000, 11000);
@@ -383,7 +391,7 @@ static int goodix_hid_get_raw_report(struct hid_device *hid,
 	u16 cmd_register = le16_to_cpu(ts->hid_desc.cmd_register);
 	u8 tmp_buf[GOODIX_HID_MAX_INBUF_SIZE];
 	int tx_len = 0, args_len = 0;
-	int response_data_len;
+	u32 response_data_len;
 	u8 args[3];
 	int error;
 
@@ -434,9 +442,9 @@ static int goodix_hid_get_raw_report(struct hid_device *hid,
 		return 0;
 
 	/* Step2: check response data status */
-	response_data_len = goodix_hid_check_ack_status(ts);
-	if (response_data_len <= GOODIX_HID_PKG_LEN_SIZE)
-		return -EINVAL;
+	error = goodix_hid_check_ack_status(ts, &response_data_len);
+	if (error)
+		return error;
 
 	len = min(len, response_data_len - GOODIX_HID_PKG_LEN_SIZE);
 	/* Step3: read response data(skip 2bytes of hid pkg length) */
-- 
2.43.0

Re: [PATCH v2 next] HID: hid-goodix: Fix type promotion bug in goodix_hid_get_raw_report()

[Dmitry Torokhov]

On Thu, Aug 29, 2024 at 10:30:39PM +0300, Dan Carpenter wrote:
> The issue is GOODIX_HID_PKG_LEN_SIZE is defined as sizeof(u16) which is
> type size_t.  However, goodix_hid_check_ack_status() returns negative
> error codes or potentially a positive but invalid length which is too
> small.  So when we compare "if ((response_data_len <=
> GOODIX_HID_PKG_LEN_SIZE)" then negative error codes are type promoted to
> size_t and counted as a positive large value and treated as valid.
> 
> It would have been easy enough to add some casting to avoid the type
> promotion, however this patch takes a more thourough approach and moves
> the length check into goodix_hid_check_ack_status().  Now the function
> only return negative error codes or zero on success and the length
> pointer is never set to an invalid length.
> 
> Fixes: 75e16c8ce283 ("HID: hid-goodix: Add Goodix HID-over-SPI driver")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>

Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Thanks Dan!

-- 
Dmitry

Re: [PATCH v2 next] HID: hid-goodix: Fix type promotion bug in goodix_hid_get_raw_report()

[Jiri Kosina]

On Thu, 29 Aug 2024, Dan Carpenter wrote:

> The issue is GOODIX_HID_PKG_LEN_SIZE is defined as sizeof(u16) which is
> type size_t.  However, goodix_hid_check_ack_status() returns negative
> error codes or potentially a positive but invalid length which is too
> small.  So when we compare "if ((response_data_len <=
> GOODIX_HID_PKG_LEN_SIZE)" then negative error codes are type promoted to
> size_t and counted as a positive large value and treated as valid.
> 
> It would have been easy enough to add some casting to avoid the type
> promotion, however this patch takes a more thourough approach and moves
> the length check into goodix_hid_check_ack_status().  Now the function
> only return negative error codes or zero on success and the length
> pointer is never set to an invalid length.
> 
> Fixes: 75e16c8ce283 ("HID: hid-goodix: Add Goodix HID-over-SPI driver")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>

Good catch Dan, applied, thanks!

-- 
Jiri Kosina
SUSE Labs