| Message ID | a04cfa63-de06-4d09-af80-a567f2db8f12@stanley.mountain (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | [v2,next] HID: hid-goodix: Fix type promotion bug in goodix_hid_get_raw_report() | expand |
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!
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!
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) */
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(-)