Hi Dan,
On Fri, Aug 23, 2024 at 03:51:27PM +0300, Dan Carpenter wrote:
> GOODIX_HID_PKG_LEN_SIZE defined as sizeof(u16) (type size_t). If the
> goodix_hid_check_ack_status() function times out and return -EINVAL then,
> because of type promotion, the negative error code is treated as a high
> positive value which is success.
>
> Fix this by adding an explicit check for negative error codes.
>
> Fixes: 75e16c8ce283 ("HID: hid-goodix: Add Goodix HID-over-SPI driver")
> Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> ---
> drivers/hid/hid-goodix-spi.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-goodix-spi.c b/drivers/hid/hid-goodix-spi.c
> index 5103bf0aada4..59415f95c675 100644
> --- a/drivers/hid/hid-goodix-spi.c
> +++ b/drivers/hid/hid-goodix-spi.c
> @@ -435,7 +435,8 @@ static int goodix_hid_get_raw_report(struct hid_device *hid,
>
> /* Step2: check response data status */
> response_data_len = goodix_hid_check_ack_status(ts);
> - if (response_data_len <= GOODIX_HID_PKG_LEN_SIZE)
> + if (response_data_len < 0 ||
> + response_data_len <= GOODIX_HID_PKG_LEN_SIZE)
> return -EINVAL;
I think this is too subtle and we may lose your fix again in
restructuring/refactoring. Could you change goodix_hid_check_ack_status()
to take length as an argument to be filled in? And then we'd do:
error = goodix_hid_check_ack_status(ts, &response_data_len);
if (error)
return error;
The check for the correct length of the response could go into
goodix_hid_check_ack_status() as well.
What do you think?
Thanks.
--
Dmitry
