| Message ID | 8e6fe9f1-fcd8-4264-b28d-a1ee99b592b8@stanley.mountain (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | [next] HID: hid-goodix: Fix a signedness bug in goodix_hid_get_raw_report() | expand |
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.
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
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.
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
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);
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(-)