| Message ID | 5c1cc882-b2b8-45cb-b8f7-4b35d1800e5d@moroto.mountain (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: steelseries: Fix signedness bug in steelseries_headset_arctis_1_fetch_battery() | expand |
Hey Dan, On Thu, 2023-09-07 at 12:55 +0300, Dan Carpenter wrote: > The hid_hw_raw_request() function returns negative error codes or the > number bytes transferred. If it returns negative error codes, then > the > problem is that when we check if "ret < > sizeof(arctis_1_battery_request)", > negative values are type promoted to high unsigned values and treated > as > success. Add an explicit check for negatives to address this issue. I would be grateful if you could either add the compiler's error message or explain in greater details the integer type promotion (or demotion from signed to unsigned in this case) in the commit message. > Fixes: a0c76896c3fb ("HID: steelseries: Add support for Arctis 1 > XBox") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > drivers/hid/hid-steelseries.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid- > steelseries.c > index 43d2cf7153d7..485d2287d58a 100644 > --- a/drivers/hid/hid-steelseries.c > +++ b/drivers/hid/hid-steelseries.c > @@ -390,7 +390,7 @@ static int > steelseries_headset_arctis_1_fetch_battery(struct hid_device *hdev) > ret = hid_hw_raw_request(hdev, arctis_1_battery_request[0], > write_buf, > sizeof(arctis_1_battery_request), > HID_OUTPUT_REPORT, > HID_REQ_SET_REPORT); > - if (ret < sizeof(arctis_1_battery_request)) { > + if (ret < 0 || ret < sizeof(arctis_1_battery_request)) { I prefer: - if (ret < sizeof(arctis_1_battery_request)) { + if (ret < (int) sizeof(arctis_1_battery_request)) { although I'm not sure that's the kernel-style for this sort of checks. > hid_err(hdev, "hid_hw_raw_request() failed with > %d\n", ret); > ret = -ENODATA; > }
On Wed, Sep 13, 2023 at 04:43:29PM +0200, Bastien Nocera wrote: > Hey Dan, > > On Thu, 2023-09-07 at 12:55 +0300, Dan Carpenter wrote: > > The hid_hw_raw_request() function returns negative error codes or the > > number bytes transferred. If it returns negative error codes, then > > the > > problem is that when we check if "ret < > > sizeof(arctis_1_battery_request)", > > negative values are type promoted to high unsigned values and treated > > as > > success. Add an explicit check for negatives to address this issue. > > I would be grateful if you could either add the compiler's error > message or explain in greater details the integer type promotion (or > demotion from signed to unsigned in this case) in the commit message. > This is a Smatch warning but you have to have built the cross function database and I only know one other person who does that for sure... drivers/hid/hid-steelseries.c:393 steelseries_headset_arctis_1_fetch_battery() warn: error code type promoted to positive: 'ret' I can add that and resend. > > Fixes: a0c76896c3fb ("HID: steelseries: Add support for Arctis 1 > > XBox") > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > > > > --- > > drivers/hid/hid-steelseries.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid- > > steelseries.c > > index 43d2cf7153d7..485d2287d58a 100644 > > --- a/drivers/hid/hid-steelseries.c > > +++ b/drivers/hid/hid-steelseries.c > > @@ -390,7 +390,7 @@ static int > > steelseries_headset_arctis_1_fetch_battery(struct hid_device *hdev) > > ret = hid_hw_raw_request(hdev, arctis_1_battery_request[0], > > write_buf, > > sizeof(arctis_1_battery_request), > > HID_OUTPUT_REPORT, > > HID_REQ_SET_REPORT); > > - if (ret < sizeof(arctis_1_battery_request)) { > > + if (ret < 0 || ret < sizeof(arctis_1_battery_request)) { > > I prefer: > - if (ret < sizeof(arctis_1_battery_request)) { > + if (ret < (int) sizeof(arctis_1_battery_request)) { > > although I'm not sure that's the kernel-style for this sort of checks. > I grepped to see which format is more popular and it's 14 vs 13 in favor of casting. I prefer to avoid casts, but I can resend. regards, dan carpenter
On Wed, 2023-09-13 at 20:38 +0300, Dan Carpenter wrote: > On Wed, Sep 13, 2023 at 04:43:29PM +0200, Bastien Nocera wrote: > > Hey Dan, > > > > On Thu, 2023-09-07 at 12:55 +0300, Dan Carpenter wrote: > > > The hid_hw_raw_request() function returns negative error codes or > > > the > > > number bytes transferred. If it returns negative error codes, > > > then > > > the > > > problem is that when we check if "ret < > > > sizeof(arctis_1_battery_request)", > > > negative values are type promoted to high unsigned values and > > > treated > > > as > > > success. Add an explicit check for negatives to address this > > > issue. > > > > I would be grateful if you could either add the compiler's error > > message or explain in greater details the integer type promotion > > (or > > demotion from signed to unsigned in this case) in the commit > > message. > > > > This is a Smatch warning but you have to have built the cross > function > database and I only know one other person who does that for sure... > > drivers/hid/hid-steelseries.c:393 > steelseries_headset_arctis_1_fetch_battery() > warn: error code type promoted to positive: 'ret' > > I can add that and resend. That would be great, thanks! > > > > Fixes: a0c76896c3fb ("HID: steelseries: Add support for Arctis 1 > > > XBox") > > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > > > > > > > > --- > > > drivers/hid/hid-steelseries.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid- > > > steelseries.c > > > index 43d2cf7153d7..485d2287d58a 100644 > > > --- a/drivers/hid/hid-steelseries.c > > > +++ b/drivers/hid/hid-steelseries.c > > > @@ -390,7 +390,7 @@ static int > > > steelseries_headset_arctis_1_fetch_battery(struct hid_device > > > *hdev) > > > ret = hid_hw_raw_request(hdev, > > > arctis_1_battery_request[0], > > > write_buf, > > > sizeof(arctis_1_battery_request), > > > HID_OUTPUT_REPORT, > > > HID_REQ_SET_REPORT); > > > - if (ret < sizeof(arctis_1_battery_request)) { > > > + if (ret < 0 || ret < sizeof(arctis_1_battery_request)) { > > > > I prefer: > > - if (ret < sizeof(arctis_1_battery_request)) { > > + if (ret < (int) sizeof(arctis_1_battery_request)) { > > > > although I'm not sure that's the kernel-style for this sort of > > checks. > > > > I grepped to see which format is more popular and it's 14 vs 13 in > favor > of casting. I prefer to avoid casts, but I can resend. > > regards, > dan carpenter > >
diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-steelseries.c index 43d2cf7153d7..485d2287d58a 100644 --- a/drivers/hid/hid-steelseries.c +++ b/drivers/hid/hid-steelseries.c @@ -390,7 +390,7 @@ static int steelseries_headset_arctis_1_fetch_battery(struct hid_device *hdev) ret = hid_hw_raw_request(hdev, arctis_1_battery_request[0], write_buf, sizeof(arctis_1_battery_request), HID_OUTPUT_REPORT, HID_REQ_SET_REPORT); - if (ret < sizeof(arctis_1_battery_request)) { + if (ret < 0 || ret < sizeof(arctis_1_battery_request)) { hid_err(hdev, "hid_hw_raw_request() failed with %d\n", ret); ret = -ENODATA; }
The hid_hw_raw_request() function returns negative error codes or the number bytes transferred. If it returns negative error codes, then the problem is that when we check if "ret < sizeof(arctis_1_battery_request)", negative values are type promoted to high unsigned values and treated as success. Add an explicit check for negatives to address this issue. Fixes: a0c76896c3fb ("HID: steelseries: Add support for Arctis 1 XBox") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- drivers/hid/hid-steelseries.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)