Re: [PATCH] HID: sony: Fix division by zero

[Alain <alain.carlucci@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 690 bytes --]

Hi Roderick,
Hi Daniel,

Thank you for all the suggestions, Roderick.
I fixed the typo in your patch, backported to hid-sony and tested both of them.
They fix the issue and the DS4 can be used even without calibration data.
I cannot test if everything works well with a DualSense because I do
not own one.

Hi Daniel, I've seen that in the hid-nintendo driver, the calibration
denominator is not sanitized if it's zero.
If I'm not mistaken, a device that disguises itself as a joycon with
denominator zero can crash the kernel
when this is used in the mult_frac(). I had this behaviour with the
hid-sony driver.
You can find attached the patch that should solve the problem.

Thanks,
Alain

[-- Attachment #2: 0003-HID-nintendo-sanity-check-for-denominators.patch --]
[-- Type: text/x-patch, Size: 1309 bytes --]

From 5f7aa1612a10123b9630701d29e39f8e55a74b5e Mon Sep 17 00:00:00 2001
From: Alain Carlucci <carpikemail@gmail.com>
Date: Fri, 30 Dec 2022 18:53:53 +0100
Subject: [PATCH 3/3] HID: nintendo: sanity check for denominators

This patch prevents a crash if the device reports denominator equal to
zero. In that case, it is substituted with one, so that the driver does
not do a division by zero.
---
 drivers/hid/hid-nintendo.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
index 5bfc0c450..f050754e2 100644
--- a/drivers/hid/hid-nintendo.c
+++ b/drivers/hid/hid-nintendo.c
@@ -866,6 +866,19 @@ static void joycon_calc_imu_cal_divisors(struct joycon_ctlr *ctlr)
 						ctlr->accel_cal.offset[i];
 		ctlr->imu_cal_gyro_divisor[i] = ctlr->gyro_cal.scale[i] -
 						ctlr->gyro_cal.offset[i];
+
+
+		/* Sanity check accelerometer calibration data. */
+		if (ctlr->imu_cal_accel_divisor[i] == 0) {
+			hid_warn(ctlr->hdev, "Invalid divisor for accelerometer %d.", i);
+			ctlr->imu_cal_accel_divisor[i] = 1;
+		}
+
+		/* Sanity check gyroscope calibration data. */
+		if (ctlr->imu_cal_gyro_divisor[i] == 0) {
+			hid_warn(ctlr->hdev, "Invalid divisor for gyroscope %d.", i);
+			ctlr->imu_cal_gyro_divisor[i] = 1;
+		}
 	}
 }
 
-- 
2.39.0


[-- Attachment #3: 0001-HID-playstation-sanity-check-calibration-data.patch --]
[-- Type: text/x-patch, Size: 4975 bytes --]

From d751911f23aca413b2bf831ea301219c533081e6 Mon Sep 17 00:00:00 2001
From: Alain Carlucci <carpikemail@gmail.com>
Date: Fri, 30 Dec 2022 18:47:54 +0100
Subject: [PATCH 1/3] HID: playstation: sanity check calibration data.

This patch prevents a division by zero if the the device reports invalid
calibration parameters. In that case, fallback to default values.
---
 drivers/hid/hid-playstation.c | 61 +++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index f399bf0d3..a249f3bb0 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -954,6 +954,7 @@ static int dualsense_get_calibration_data(struct dualsense *ds)
 	int speed_2x;
 	int range_2g;
 	int ret = 0;
+	int i;
 	uint8_t *buf;
 
 	buf = kzalloc(DS_FEATURE_REPORT_CALIBRATION_SIZE, GFP_KERNEL);
@@ -1005,6 +1006,23 @@ static int dualsense_get_calibration_data(struct dualsense *ds)
 	ds->gyro_calib_data[2].sens_numer = speed_2x*DS_GYRO_RES_PER_DEG_S;
 	ds->gyro_calib_data[2].sens_denom = gyro_roll_plus - gyro_roll_minus;
 
+	/* Sanity check gyro calibration data. This is needed for clone devices,
+	 * which poorly implement support. Some devices appear to return zeros
+	 * for all data, while some hardcode all the same values and for some
+	 * use the wrong sign (the delta is then 0).
+	 * When we detect something fishy, just disable the calibration logic
+	 * altogether as nothing can be trusted for that axis.
+	 */
+	for (i = 0; i < ARRAY_SIZE(ds->gyro_calib_data); i++) {
+		if (ds->gyro_calib_data[i].sens_denom == 0) {
+			hid_warn(ds->base.hdev, "Invalid gyro calibration data for axis (%d), disabling calibration.",
+					ds->gyro_calib_data[i].abs_code);
+			ds->gyro_calib_data[i].bias = 0;
+			ds->gyro_calib_data[i].sens_numer = DS_GYRO_RES_PER_DEG_S;
+			ds->gyro_calib_data[i].sens_denom = 1;
+		}
+	}
+
 	/*
 	 * Set accelerometer calibration and normalization parameters.
 	 * Data values will be normalized to 1/DS_ACC_RES_PER_G g.
@@ -1027,6 +1045,18 @@ static int dualsense_get_calibration_data(struct dualsense *ds)
 	ds->accel_calib_data[2].sens_numer = 2*DS_ACC_RES_PER_G;
 	ds->accel_calib_data[2].sens_denom = range_2g;
 
+	/* Sanity check accelerometer calibration data. This is needed for clone devices,
+	 * which poorly implement support.
+	 */
+	for (i = 0; i < ARRAY_SIZE(ds->accel_calib_data); i++) {
+		if (ds->accel_calib_data[i].sens_denom == 0) {
+			hid_warn(ds->base.hdev, "Invalid accelerometer calibration data for axis (%d), disabling calibration.",
+					ds->accel_calib_data[i].abs_code);
+			ds->accel_calib_data[i].bias = 0;
+			ds->accel_calib_data[i].sens_numer = DS_ACC_RES_PER_G;
+			ds->accel_calib_data[i].sens_denom = 1;
+		}
+	}
 err_free:
 	kfree(buf);
 	return ret;
@@ -1737,6 +1767,7 @@ static int dualshock4_get_calibration_data(struct dualshock4 *ds4)
 	int speed_2x;
 	int range_2g;
 	int ret = 0;
+	int i;
 	uint8_t *buf;
 
 	if (ds4->base.hdev->bus == BUS_USB) {
@@ -1830,6 +1861,23 @@ static int dualshock4_get_calibration_data(struct dualshock4 *ds4)
 	ds4->gyro_calib_data[2].sens_numer = speed_2x*DS4_GYRO_RES_PER_DEG_S;
 	ds4->gyro_calib_data[2].sens_denom = gyro_roll_plus - gyro_roll_minus;
 
+	/* Sanity check gyro calibration data. This is needed for clone devices,
+	 * which poorly implement support. Some devices appear to return zeros
+	 * for all data, while some hardcode all the same values and for some
+	 * use the wrong sign (the delta is then 0).
+	 * When we detect something fishy, just disable the calibration logic
+	 * altogether as nothing can be trusted for that axis.
+	 */
+	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_RES_PER_DEG_S;
+			ds4->gyro_calib_data[i].sens_denom = 1;
+		}
+	}
+
 	/*
 	 * Set accelerometer calibration and normalization parameters.
 	 * Data values will be normalized to 1/DS4_ACC_RES_PER_G g.
@@ -1852,6 +1900,19 @@ static int dualshock4_get_calibration_data(struct dualshock4 *ds4)
 	ds4->accel_calib_data[2].sens_numer = 2*DS4_ACC_RES_PER_G;
 	ds4->accel_calib_data[2].sens_denom = range_2g;
 
+	/* Sanity check accelerometer calibration data. This is needed for clone devices,
+	 * which poorly implement support.
+	 */
+	for (i = 0; i < ARRAY_SIZE(ds4->accel_calib_data); i++) {
+		if (ds4->accel_calib_data[i].sens_denom == 0) {
+			hid_warn(hdev, "Invalid accelerometer calibration data for axis (%d), disabling calibration.",
+					ds4->accel_calib_data[i].abs_code);
+			ds4->accel_calib_data[i].bias = 0;
+			ds4->accel_calib_data[i].sens_numer = DS4_ACC_RES_PER_G;
+			ds4->accel_calib_data[i].sens_denom = 1;
+		}
+	}
+
 err_free:
 	kfree(buf);
 	return ret;
-- 
2.39.0


[-- Attachment #4: 0002-HID-sony-sanity-check-DS4-calibration-data.patch --]
[-- Type: text/x-patch, Size: 2086 bytes --]

From dc92f0be1352ba5a27239f451e5f0161e48f14e6 Mon Sep 17 00:00:00 2001
From: Alain Carlucci <carpikemail@gmail.com>
Date: Fri, 30 Dec 2022 18:52:06 +0100
Subject: [PATCH 2/3] HID: sony: sanity check DS4 calibration data

This patch prevents a crash when the DS4 reports invalid calibration
data for the IMUs. It checks if the denominator is zero, and in that
case fallback to a default value.
---
 drivers/hid/hid-sony.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 13125997a..d25e84e35 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1714,6 +1714,7 @@ static int dualshock4_get_calibration_data(struct sony_sc *sc)
 	short acc_z_plus, acc_z_minus;
 	int speed_2x;
 	int range_2g;
+	int i;
 
 	/* For Bluetooth we use a different request, which supports CRC.
 	 * Note: in Bluetooth mode feature report 0x02 also changes the state
@@ -1858,6 +1859,30 @@ static int dualshock4_get_calibration_data(struct sony_sc *sc)
 	sc->ds4_calib_data[5].sens_numer = 2*DS4_ACC_RES_PER_G;
 	sc->ds4_calib_data[5].sens_denom = range_2g;
 
+	/* Sanity check gyro calibration data. This is needed for clone devices,
+	 * which poorly implement support. Some devices appear to return zeros
+	 * for all data, while some hardcode all the same values and for some
+	 * use the wrong sign (the delta is then 0).
+	 * When we detect something fishy, just disable the calibration logic
+	 * altogether as nothing can be trusted for that axis.
+	 */
+	for (i = 0; i < 6; i++) {
+		if (sc->ds4_calib_data[i].sens_denom == 0) {
+			hid_warn(sc->hdev, "Invalid calibration data for axis (%d), disabling calibration.",
+					sc->ds4_calib_data[i].abs_code);
+
+			if (i < 3) {
+				/* Gyroscope */
+				sc->ds4_calib_data[i].sens_numer = DS4_GYRO_RES_PER_DEG_S;
+			} else {
+				/* Accelerometer */
+				sc->ds4_calib_data[i].sens_numer = DS4_ACC_RES_PER_G;
+			}
+			sc->ds4_calib_data[i].bias = 0;
+			sc->ds4_calib_data[i].sens_denom = 1;
+		}
+	}
+
 err_stop:
 	kfree(buf);
 	return ret;
-- 
2.39.0