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

[Bastien Nocera <hadess@hadess.net>]

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