[PATCH] HID: steelseries: Fix signedness bug in steelseries_headset_arctis_1_fetch_battery()

[PATCH] HID: steelseries: Fix signedness bug in steelseries_headset_arctis_1_fetch_battery()

[Dan Carpenter]

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

Re: [PATCH] HID: steelseries: Fix signedness bug in steelseries_headset_arctis_1_fetch_battery()

[Bastien Nocera]

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;
>         }

Re: [PATCH] HID: steelseries: Fix signedness bug in steelseries_headset_arctis_1_fetch_battery()

[Dan Carpenter]

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

Re: [PATCH] HID: steelseries: Fix signedness bug in steelseries_headset_arctis_1_fetch_battery()

[Bastien Nocera]

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
> 
>