Re: [PATCH] drivers/hid: avoid invalid denominator

[Roderick Colenbrander]

Hm, I'm a little confused then. The Shanwan gamepad is a Dualshock 3
clone (nickname 'sixaxis'), so should not go through the DualShock 4
paths. Can you double check what the vendor / device id for this
device are? Are you seeing specific errors or warnings from the driver
in the kernel logs?

I have been considering how to better handle some of these clones
devices, while also not polluting the code too much. If this device is
indeed a clone DualShock 4. I am more leaning towards finding out
sooner during device creation. Perhaps should not even create a motion
sensor device if the data will be none anyway... Hm, not sure what I
prefer yet.

Thanks,
Roderick

On Sun, Apr 25, 2021 at 9:39 AM Henry Castro <hcvcastro@xxxxxxxxx> wrote:
>
> Hi,
>
> I was trying to play some old games from Retroarch, and I plugged
> my "SHANWAN PS3 GamePad", but it begin to vibrate and I could
> not play the games, So I started to investigate and read the source
> code, and get the code to bypass the check, and suddenly I got
> a panic crash.  So I found the issue, and I added the patch, finally
> got working, and I played my favorite old games.
>
> So it happens in PS3 clone device I think, but I have to tweak the code
> probably is safe under the condition you mention in PS4 DualShock,
> but it is good as a prevention, if someone is testing and debugging
> the driver.
>
> Regards
> Henry
>
> El sáb, 24 abr 2021 a las 17:11, Roderick Colenbrander (<thunderbird2k@xxxxxxxxx>) escribió:
>>
>> Hi Henry,
>>
>> Thanks for your patch. In what case has this been an issue? Or was it
>> more theoretical.
>>
>> During normal operation this condition should never be triggered for a
>> DualShock 4 when calibration succeeds. If it doesn't succeed the
>> device is not registered. We had an issue recently with the DS4 dongle
>> where the calibration data was 0, which was due to a race condition
>> with Steam, but that was resolved recently.
>>
>> Thanks,
>> Roderick
>>
>> On Sun, Apr 11, 2021 at 10:19 AM Henry Castro <hcvcastro@xxxxxxxxx> wrote:
>> >
>> > Avoid a potential panic in case wrong denominator
>> > is given.
>> >
>> > Signed-off-by: Henry Castro <hcvcastro@xxxxxxxxx>
>> > ---
>> >  drivers/hid/hid-sony.c | 13 +++++++++----
>> >  1 file changed, 9 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
>> > index 8319b0ce385a..67b45d82cc3b 100644
>> > --- a/drivers/hid/hid-sony.c
>> > +++ b/drivers/hid/hid-sony.c
>> > @@ -1134,11 +1134,16 @@ static void dualshock4_parse_report(struct sony_sc *sc, u8 *rd, int size)
>> >                  * Note: we swap numerator 'x' and 'numer' in mult_frac for
>> >                  *       precision reasons so we don't need 64-bit.
>> >                  */
>> > -               int calib_data = mult_frac(calib->sens_numer,
>> > -                                          raw_data - calib->bias,
>> > -                                          calib->sens_denom);
>> > +               if (calib->sens_denom != 0) {
>> > +                       int calib_data = mult_frac(calib->sens_numer,
>> > +                                                  raw_data - calib->bias,
>> > +                                                  calib->sens_denom);
>> > +
>> > +                       input_report_abs(sc->sensor_dev, calib->abs_code, calib_data);
>> > +               } else {
>> > +                       hid_warn(sc->hdev, "DualShock 4 parse report, avoid invalid denominator");
>> > +               }
>> >
>> > -               input_report_abs(sc->sensor_dev, calib->abs_code, calib_data);
>> >                 offset += 2;
>> >         }
>> >         input_sync(sc->sensor_dev);
>> > --
>> > 2.20.1
>> >