Re: [PATCH v1] HID: playstation: DS4: Fix calibration workaround for clone devices

From: Roderick Colenbrander
Date: Mon Apr 15 2024 - 15:56:00 EST

Next message: Kuppuswamy Sathyanarayanan: "Re: [PATCH v3 13/14] x86/sev: Hide SVSM attestation entries if not running under an SVSM"
Previous message: Jarkko Sakkinen: "Re: [PATCH v4 13/14] selftests/digest_cache: Add selftests for digest_cache LSM"
In reply to: Max Staudt: "[PATCH v1] HID: playstation: DS4: Fix calibration workaround for clone devices"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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 = ABS_RX + i;

Thanks,
Roderick

On Sun, Apr 14, 2024 at 9:12 AM Max Staudt <max@xxxxxxxxx> 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 data request")
> Signed-off-by: Max Staudt <max@xxxxxxxxx>
> ---
> 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 dualshock4 *ds4)
> buf = kzalloc(DS4_FEATURE_REPORT_CALIBRATION_SIZE, GFP_KERNEL);
> if (!buf) {
> ret = -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 dualshock4 *ds4)
>
> hid_warn(hdev, "Failed to retrieve DualShock4 calibration info: %d\n", ret);
> ret = -EILSEQ;
> + goto transfer_failed;
> } else {
> break;
> }
> @@ -1815,17 +1816,19 @@ static int dualshock4_get_calibration_data(struct dualshock4 *ds4)
> buf = kzalloc(DS4_FEATURE_REPORT_CALIBRATION_BT_SIZE, GFP_KERNEL);
> if (!buf) {
> ret = -ENOMEM;
> - goto no_buffer_tail_check;
> + goto transfer_failed;
> }
>
> ret = ps_get_report(hdev, DS4_FEATURE_REPORT_CALIBRATION_BT, buf,
> DS4_FEATURE_REPORT_CALIBRATION_BT_SIZE, true);
>
> - if (ret)
> + if (ret) {
> hid_warn(hdev, "Failed to retrieve DualShock4 calibration info: %d\n", ret);
> + goto transfer_failed;
> + }
> }
>
> - /* Parse buffer. If the transfer failed, this safely copies zeroes. */
> + /* Transfer succeeded - parse the calibration data received. */
> gyro_pitch_bias = get_unaligned_le16(&buf[1]);
> gyro_yaw_bias = get_unaligned_le16(&buf[3]);
> gyro_roll_bias = get_unaligned_le16(&buf[5]);
> @@ -1854,71 +1857,71 @@ static int dualshock4_get_calibration_data(struct dualshock4 *ds4)
> acc_z_plus = get_unaligned_le16(&buf[31]);
> acc_z_minus = 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 degree/s.
> */
> speed_2x = (gyro_speed_plus + gyro_speed_minus);
> - ds4->gyro_calib_data[0].abs_code = ABS_RX;
> ds4->gyro_calib_data[0].bias = 0;
> ds4->gyro_calib_data[0].sens_numer = speed_2x*DS4_GYRO_RES_PER_DEG_S;
> ds4->gyro_calib_data[0].sens_denom = abs(gyro_pitch_plus - gyro_pitch_bias) +
> abs(gyro_pitch_minus - gyro_pitch_bias);
>
> - ds4->gyro_calib_data[1].abs_code = ABS_RY;
> ds4->gyro_calib_data[1].bias = 0;
> ds4->gyro_calib_data[1].sens_numer = speed_2x*DS4_GYRO_RES_PER_DEG_S;
> ds4->gyro_calib_data[1].sens_denom = abs(gyro_yaw_plus - gyro_yaw_bias) +
> abs(gyro_yaw_minus - gyro_yaw_bias);
>
> - ds4->gyro_calib_data[2].abs_code = ABS_RZ;
> ds4->gyro_calib_data[2].bias = 0;
> ds4->gyro_calib_data[2].sens_numer = speed_2x*DS4_GYRO_RES_PER_DEG_S;
> ds4->gyro_calib_data[2].sens_denom = 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 = 0; i < ARRAY_SIZE(ds4->gyro_calib_data); i++) {
> - if (ds4->gyro_calib_data[i].sens_denom == 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 = 0;
> - ds4->gyro_calib_data[i].sens_numer = DS4_GYRO_RANGE;
> - ds4->gyro_calib_data[i].sens_denom = S16_MAX;
> - }
> - }
> -
> /*
> * Set accelerometer calibration and normalization parameters.
> * Data values will be normalized to 1/DS4_ACC_RES_PER_G g.
> */
> range_2g = acc_x_plus - acc_x_minus;
> - ds4->accel_calib_data[0].abs_code = ABS_X;
> ds4->accel_calib_data[0].bias = acc_x_plus - range_2g / 2;
> ds4->accel_calib_data[0].sens_numer = 2*DS4_ACC_RES_PER_G;
> ds4->accel_calib_data[0].sens_denom = range_2g;
>
> range_2g = acc_y_plus - acc_y_minus;
> - ds4->accel_calib_data[1].abs_code = ABS_Y;
> ds4->accel_calib_data[1].bias = acc_y_plus - range_2g / 2;
> ds4->accel_calib_data[1].sens_numer = 2*DS4_ACC_RES_PER_G;
> ds4->accel_calib_data[1].sens_denom = range_2g;
>
> range_2g = acc_z_plus - acc_z_minus;
> - ds4->accel_calib_data[2].abs_code = ABS_Z;
> ds4->accel_calib_data[2].bias = acc_z_plus - range_2g / 2;
> ds4->accel_calib_data[2].sens_numer = 2*DS4_ACC_RES_PER_G;
> ds4->accel_calib_data[2].sens_denom = range_2g;
>
> +transfer_failed:
> + ds4->gyro_calib_data[0].abs_code = ABS_RX;
> + ds4->gyro_calib_data[1].abs_code = ABS_RY;
> + ds4->gyro_calib_data[2].abs_code = ABS_RZ;
> + ds4->accel_calib_data[0].abs_code = ABS_X;
> + ds4->accel_calib_data[1].abs_code = ABS_Y;
> + ds4->accel_calib_data[2].abs_code = 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 = 0; i < ARRAY_SIZE(ds4->gyro_calib_data); i++) {
> + if (ds4->gyro_calib_data[i].sens_denom == 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 = 0;
> + ds4->gyro_calib_data[i].sens_numer = DS4_GYRO_RANGE;
> + ds4->gyro_calib_data[i].sens_denom = 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
>
>

Next message: Kuppuswamy Sathyanarayanan: "Re: [PATCH v3 13/14] x86/sev: Hide SVSM attestation entries if not running under an SVSM"
Previous message: Jarkko Sakkinen: "Re: [PATCH v4 13/14] selftests/digest_cache: Add selftests for digest_cache LSM"
In reply to: Max Staudt: "[PATCH v1] HID: playstation: DS4: Fix calibration workaround for clone devices"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]