[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5660b5ea-0806-493e-8364-15b0d519be76@stanley.mountain>
Date: Sat, 24 Aug 2024 00:32:53 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: Charles Wang <charles.goodix@...il.com>, Jiri Kosina <jikos@...nel.org>,
Benjamin Tissoires <bentiss@...nel.org>,
linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
kernel-janitors@...r.kernel.org
Subject: Re: [PATCH next] HID: hid-goodix: Fix a signedness bug in
goodix_hid_get_raw_report()
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@...aro.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
Powered by blists - more mailing lists