[PATCH next] HID: hid-goodix: Fix a signedness bug in goodix_hid_get_raw_report()

[PATCH next] HID: hid-goodix: Fix a signedness bug in goodix_hid_get_raw_report()

[Dan Carpenter]

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@linaro.org>
---
 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;
 
 	len = min(len, response_data_len - GOODIX_HID_PKG_LEN_SIZE);
-- 
2.43.0

Re: [PATCH next] HID: hid-goodix: Fix a signedness bug in goodix_hid_get_raw_report()

[Dmitry Torokhov]

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@linaro.org>
> ---
>  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

Re: [PATCH next] HID: hid-goodix: Fix a signedness bug in goodix_hid_get_raw_report()

[Dan Carpenter]

On Fri, Aug 23, 2024 at 01:10:49PM -0700, Dmitry Torokhov wrote:
> 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@linaro.org>
> > ---
> >  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?

I'm fine with this.

I bet that you already wrote the patch that your describing.  If you want to
just merge that and give me reported-by credit then that's fine by me.

I can also resend.  I don't mind doing that.  I've already written the patch
that you're describing and I just have to write the commit message and hit send.
I can send it on Monday.  Whatever is easiest.

regards,
dan carpenter

Re: [PATCH next] HID: hid-goodix: Fix a signedness bug in goodix_hid_get_raw_report()

[Dmitry Torokhov]

On Sat, Aug 24, 2024 at 12:32:53AM +0300, Dan Carpenter wrote:
> On Fri, Aug 23, 2024 at 01:10:49PM -0700, Dmitry Torokhov wrote:
> > 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@linaro.org>
> > > ---
> > >  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?
> 
> I'm fine with this.
> 
> I bet that you already wrote the patch that your describing.  If you want to

No I haven't yet.

> just merge that and give me reported-by credit then that's fine by me.

This is HID so it has to go through Jiri/Benjamin anyway.

> 
> I can also resend.  I don't mind doing that.  I've already written the patch
> that you're describing and I just have to write the commit message and hit send.
> I can send it on Monday.  Whatever is easiest.

Yes, please send it unless you hear from Jiri/Benjamin that they like
current version more.

Thanks.

-- 
Dmitry

Re: [PATCH next] HID: hid-goodix: Fix a signedness bug in goodix_hid_get_raw_report()

[Dan Carpenter]

On Fri, Aug 23, 2024 at 02:58:13PM -0700, Dmitry Torokhov wrote:
> On Sat, Aug 24, 2024 at 12:32:53AM +0300, Dan Carpenter wrote:
> > 
> > I can also resend.  I don't mind doing that.  I've already written the patch
> > that you're describing and I just have to write the commit message and hit send.
> > I can send it on Monday.  Whatever is easiest.
> 
> Yes, please send it unless you hear from Jiri/Benjamin that they like
> current version more.

Sure.  Will do.

regards,
dan carpenter