Received-SPF: pass (google.com: domain of linux-kernel+bounces-145846-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240414161223.117654-1-max@enpas.org>
In-Reply-To: <20240414161223.117654-1-max@enpas.org>
From: Roderick Colenbrander <thunderbird2k@gmail.com>
Date: Mon, 15 Apr 2024 12:47:29 -0700
Message-ID: <CAEc3jaBbOfouWJtJWjGvYkqLtd9WnV=8JC94nJuhtPvpp-39AQ@mail.gmail.com>
Subject: Re: [PATCH v1] HID: playstation: DS4: Fix calibration workaround for
 clone devices
To: Max Staudt <max@enpas.org>
Cc: Roderick Colenbrander <roderick.colenbrander@sony.com>, Jiri Kosina <jikos@kernel.org>, 
	Benjamin Tissoires <benjamin.tissoires@redhat.com>, linux-input@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Max,

Thanks for your patch. For readability, I would rather not move a lot
of the ABS_X/ABS_RX related initialization code around. It is not my
preference, but I think keeping the lines were they are, but doing
this within the 'fail safe loops' is nicer:

ds4->gyro_calib_data[i].abs_code =3D ABS_RX + i;

Thanks,
Roderick

On Sun, Apr 14, 2024 at 9:12=E2=80=AFAM Max Staudt <max@enpas.org> wrote:
>
> The logic in dualshock4_get_calibration_data() used uninitialised data
> in case of a failed kzalloc() for the transfer buffer.
>
> The solution is to group all business logic and all sanity checks
> together, and jump only to the latter in case of an error.
> While we're at it, factor out the axes' labelling, since it must happen
> either way for input_report_abs() to succeed later on.
>
> Thanks to Dan Carpenter for the Smatch static checker warning.
>
> Fixes: a48a7cd85f55 ("HID: playstation: DS4: Don't fail on calibration da=
ta request")
> Signed-off-by: Max Staudt <max@enpas.org>
> ---
>  drivers/hid/hid-playstation.c | 63 ++++++++++++++++++-----------------
>  1 file changed, 33 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.=
c
> index edc46fc02e9a..998e79be45ac 100644
> --- a/drivers/hid/hid-playstation.c
> +++ b/drivers/hid/hid-playstation.c
> @@ -1787,7 +1787,7 @@ static int dualshock4_get_calibration_data(struct d=
ualshock4 *ds4)
>                 buf =3D kzalloc(DS4_FEATURE_REPORT_CALIBRATION_SIZE, GFP_=
KERNEL);
>                 if (!buf) {
>                         ret =3D -ENOMEM;
> -                       goto no_buffer_tail_check;
> +                       goto transfer_failed;
>                 }
>
>                 /* We should normally receive the feature report data we =
asked
> @@ -1807,6 +1807,7 @@ static int dualshock4_get_calibration_data(struct d=
ualshock4 *ds4)
>
>                                 hid_warn(hdev, "Failed to retrieve DualSh=
ock4 calibration info: %d\n", ret);
>                                 ret =3D -EILSEQ;
> +                               goto transfer_failed;
>                         } else {
>                                 break;
>                         }
> @@ -1815,17 +1816,19 @@ static int dualshock4_get_calibration_data(struct=
 dualshock4 *ds4)
>                 buf =3D kzalloc(DS4_FEATURE_REPORT_CALIBRATION_BT_SIZE, G=
FP_KERNEL);
>                 if (!buf) {
>                         ret =3D -ENOMEM;
> -                       goto no_buffer_tail_check;
> +                       goto transfer_failed;
>                 }
>
>                 ret =3D ps_get_report(hdev, DS4_FEATURE_REPORT_CALIBRATIO=
N_BT, buf,
>                                 DS4_FEATURE_REPORT_CALIBRATION_BT_SIZE, t=
rue);
>
> -               if (ret)
> +               if (ret) {
>                         hid_warn(hdev, "Failed to retrieve DualShock4 cal=
ibration info: %d\n", ret);
> +                       goto transfer_failed;
> +               }
>         }
>
> -       /* Parse buffer. If the transfer failed, this safely copies zeroe=
s. */
> +       /* Transfer succeeded - parse the calibration data received. */
>         gyro_pitch_bias  =3D get_unaligned_le16(&buf[1]);
>         gyro_yaw_bias    =3D get_unaligned_le16(&buf[3]);
>         gyro_roll_bias   =3D get_unaligned_le16(&buf[5]);
> @@ -1854,71 +1857,71 @@ static int dualshock4_get_calibration_data(struct=
 dualshock4 *ds4)
>         acc_z_plus       =3D get_unaligned_le16(&buf[31]);
>         acc_z_minus      =3D get_unaligned_le16(&buf[33]);
>
> +       /* Done parsing the buffer, so let's free it. */
> +       kfree(buf);
> +
>         /*
>          * Set gyroscope calibration and normalization parameters.
>          * Data values will be normalized to 1/DS4_GYRO_RES_PER_DEG_S deg=
ree/s.
>          */
>         speed_2x =3D (gyro_speed_plus + gyro_speed_minus);
> -       ds4->gyro_calib_data[0].abs_code =3D ABS_RX;
>         ds4->gyro_calib_data[0].bias =3D 0;
>         ds4->gyro_calib_data[0].sens_numer =3D speed_2x*DS4_GYRO_RES_PER_=
DEG_S;
>         ds4->gyro_calib_data[0].sens_denom =3D abs(gyro_pitch_plus - gyro=
_pitch_bias) +
>                         abs(gyro_pitch_minus - gyro_pitch_bias);
>
> -       ds4->gyro_calib_data[1].abs_code =3D ABS_RY;
>         ds4->gyro_calib_data[1].bias =3D 0;
>         ds4->gyro_calib_data[1].sens_numer =3D speed_2x*DS4_GYRO_RES_PER_=
DEG_S;
>         ds4->gyro_calib_data[1].sens_denom =3D abs(gyro_yaw_plus - gyro_y=
aw_bias) +
>                         abs(gyro_yaw_minus - gyro_yaw_bias);
>
> -       ds4->gyro_calib_data[2].abs_code =3D ABS_RZ;
>         ds4->gyro_calib_data[2].bias =3D 0;
>         ds4->gyro_calib_data[2].sens_numer =3D speed_2x*DS4_GYRO_RES_PER_=
DEG_S;
>         ds4->gyro_calib_data[2].sens_denom =3D abs(gyro_roll_plus - gyro_=
roll_bias) +
>                         abs(gyro_roll_minus - gyro_roll_bias);
>
> -       /* Done parsing the buffer, so let's free it. */
> -       kfree(buf);
> -
> -no_buffer_tail_check:
> -
> -       /*
> -        * Sanity check gyro calibration data. This is needed to prevent =
crashes
> -        * during report handling of virtual, clone or broken devices not=
 implementing
> -        * calibration data properly.
> -        */
> -       for (i =3D 0; i < ARRAY_SIZE(ds4->gyro_calib_data); i++) {
> -               if (ds4->gyro_calib_data[i].sens_denom =3D=3D 0) {
> -                       hid_warn(hdev, "Invalid gyro calibration data for=
 axis (%d), disabling calibration.",
> -                                       ds4->gyro_calib_data[i].abs_code)=
;
> -                       ds4->gyro_calib_data[i].bias =3D 0;
> -                       ds4->gyro_calib_data[i].sens_numer =3D DS4_GYRO_R=
ANGE;
> -                       ds4->gyro_calib_data[i].sens_denom =3D S16_MAX;
> -               }
> -       }
> -
>         /*
>          * Set accelerometer calibration and normalization parameters.
>          * Data values will be normalized to 1/DS4_ACC_RES_PER_G g.
>          */
>         range_2g =3D acc_x_plus - acc_x_minus;
> -       ds4->accel_calib_data[0].abs_code =3D ABS_X;
>         ds4->accel_calib_data[0].bias =3D acc_x_plus - range_2g / 2;
>         ds4->accel_calib_data[0].sens_numer =3D 2*DS4_ACC_RES_PER_G;
>         ds4->accel_calib_data[0].sens_denom =3D range_2g;
>
>         range_2g =3D acc_y_plus - acc_y_minus;
> -       ds4->accel_calib_data[1].abs_code =3D ABS_Y;
>         ds4->accel_calib_data[1].bias =3D acc_y_plus - range_2g / 2;
>         ds4->accel_calib_data[1].sens_numer =3D 2*DS4_ACC_RES_PER_G;
>         ds4->accel_calib_data[1].sens_denom =3D range_2g;
>
>         range_2g =3D acc_z_plus - acc_z_minus;
> -       ds4->accel_calib_data[2].abs_code =3D ABS_Z;
>         ds4->accel_calib_data[2].bias =3D acc_z_plus - range_2g / 2;
>         ds4->accel_calib_data[2].sens_numer =3D 2*DS4_ACC_RES_PER_G;
>         ds4->accel_calib_data[2].sens_denom =3D range_2g;
>
> +transfer_failed:
> +       ds4->gyro_calib_data[0].abs_code =3D ABS_RX;
> +       ds4->gyro_calib_data[1].abs_code =3D ABS_RY;
> +       ds4->gyro_calib_data[2].abs_code =3D ABS_RZ;
> +       ds4->accel_calib_data[0].abs_code =3D ABS_X;
> +       ds4->accel_calib_data[1].abs_code =3D ABS_Y;
> +       ds4->accel_calib_data[2].abs_code =3D ABS_Z;
> +
> +       /*
> +        * Sanity check gyro calibration data. This is needed to prevent =
crashes
> +        * during report handling of virtual, clone or broken devices not=
 implementing
> +        * calibration data properly.
> +        */
> +       for (i =3D 0; i < ARRAY_SIZE(ds4->gyro_calib_data); i++) {
> +               if (ds4->gyro_calib_data[i].sens_denom =3D=3D 0) {
> +                       hid_warn(hdev, "Invalid gyro calibration data for=
 axis (%d), disabling calibration.",
> +                                       ds4->gyro_calib_data[i].abs_code)=
;
> +                       ds4->gyro_calib_data[i].bias =3D 0;
> +                       ds4->gyro_calib_data[i].sens_numer =3D DS4_GYRO_R=
ANGE;
> +                       ds4->gyro_calib_data[i].sens_denom =3D S16_MAX;
> +               }
> +       }
> +
>         /*
>          * Sanity check accelerometer calibration data. This is needed to=
 prevent crashes
>          * during report handling of virtual, clone or broken devices not=
 implementing calibration
> --
> 2.39.2
>
>