* [PATCH] HID: steelseries: Fix signedness bug in steelseries_headset_arctis_1_fetch_battery()
@ 2023-09-07 9:55 Dan Carpenter
2023-09-13 14:43 ` Bastien Nocera
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2023-09-07 9:55 UTC (permalink / raw)
To: Bastien Nocera
Cc: Jiri Kosina, Benjamin Tissoires, linux-input, kernel-janitors
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(-)
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;
}
--
2.39.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] HID: steelseries: Fix signedness bug in steelseries_headset_arctis_1_fetch_battery()
2023-09-07 9:55 [PATCH] HID: steelseries: Fix signedness bug in steelseries_headset_arctis_1_fetch_battery() Dan Carpenter
@ 2023-09-13 14:43 ` Bastien Nocera
2023-09-13 17:38 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Bastien Nocera @ 2023-09-13 14:43 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jiri Kosina, Benjamin Tissoires, linux-input, kernel-janitors
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;
> }
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] HID: steelseries: Fix signedness bug in steelseries_headset_arctis_1_fetch_battery()
2023-09-13 14:43 ` Bastien Nocera
@ 2023-09-13 17:38 ` Dan Carpenter
2023-09-13 19:06 ` Bastien Nocera
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2023-09-13 17:38 UTC (permalink / raw)
To: Bastien Nocera
Cc: Jiri Kosina, Benjamin Tissoires, linux-input, kernel-janitors
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] HID: steelseries: Fix signedness bug in steelseries_headset_arctis_1_fetch_battery()
2023-09-13 17:38 ` Dan Carpenter
@ 2023-09-13 19:06 ` Bastien Nocera
0 siblings, 0 replies; 4+ messages in thread
From: Bastien Nocera @ 2023-09-13 19:06 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jiri Kosina, Benjamin Tissoires, linux-input, kernel-janitors
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
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-09-13 19:06 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-07 9:55 [PATCH] HID: steelseries: Fix signedness bug in steelseries_headset_arctis_1_fetch_battery() Dan Carpenter
2023-09-13 14:43 ` Bastien Nocera
2023-09-13 17:38 ` Dan Carpenter
2023-09-13 19:06 ` Bastien Nocera
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox