[PATCH] HID: nintendo: Prevent divide-by-zero on code

[PATCH] HID: nintendo: Prevent divide-by-zero on code

[Guilherme G. Piccoli]

It was reported [0] that adding a generic joycon to the system caused
a kernel crash on Steam Deck, with the below panic spew:

divide error: 0000 [#1] PREEMPT SMP NOPTI
[...]
Hardware name: Valve Jupiter/Jupiter, BIOS F7A0119 10/24/2023
RIP: 0010:nintendo_hid_event+0x340/0xcc1 [hid_nintendo]
[...]
Call Trace:
 [...]
 ? exc_divide_error+0x38/0x50
 ? nintendo_hid_event+0x340/0xcc1 [hid_nintendo]
 ? asm_exc_divide_error+0x1a/0x20
 ? nintendo_hid_event+0x307/0xcc1 [hid_nintendo]
 hid_input_report+0x143/0x160
 hidp_session_run+0x1ce/0x700 [hidp]

Since it's a divide-by-0 error, by tracking the code for potential
denominator issues, we've spotted 2 places in which this could happen;
so let's guard against the possibility and log in the kernel if the
condition happens. This is specially useful since some data that
fills some denominators are read from the joycon HW in some cases,
increasing the potential for flaws.

[0] https://github.com/ValveSoftware/SteamOS/issues/1070

Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---
 drivers/hid/hid-nintendo.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
index 138f154fecef..23f3f96c8c85 100644
--- a/drivers/hid/hid-nintendo.c
+++ b/drivers/hid/hid-nintendo.c
@@ -927,14 +927,27 @@ static int joycon_request_calibration(struct joycon_ctlr *ctlr)
  */
 static void joycon_calc_imu_cal_divisors(struct joycon_ctlr *ctlr)
 {
-	int i;
+	int i, divz = 0;
 
 	for (i = 0; i < 3; i++) {
 		ctlr->imu_cal_accel_divisor[i] = ctlr->accel_cal.scale[i] -
 						ctlr->accel_cal.offset[i];
 		ctlr->imu_cal_gyro_divisor[i] = ctlr->gyro_cal.scale[i] -
 						ctlr->gyro_cal.offset[i];
+
+		if (ctlr->imu_cal_accel_divisor[i] == 0) {
+			ctlr->imu_cal_accel_divisor[i] = 1;
+			divz++;
+		}
+
+		if (ctlr->imu_cal_gyro_divisor[i] == 0) {
+			ctlr->imu_cal_gyro_divisor[i] = 1;
+			divz++;
+		}
 	}
+
+	if (divz)
+		hid_warn(ctlr->hdev, "inaccurate IMU divisors (%d)\n", divz);
 }
 
 static const s16 DFLT_ACCEL_OFFSET /*= 0*/;
@@ -1163,16 +1176,16 @@ static void joycon_parse_imu_report(struct joycon_ctlr *ctlr,
 		    JC_IMU_SAMPLES_PER_DELTA_AVG) {
 			ctlr->imu_avg_delta_ms = ctlr->imu_delta_samples_sum /
 						 ctlr->imu_delta_samples_count;
-			/* don't ever want divide by zero shenanigans */
-			if (ctlr->imu_avg_delta_ms == 0) {
-				ctlr->imu_avg_delta_ms = 1;
-				hid_warn(ctlr->hdev,
-					 "calculated avg imu delta of 0\n");
-			}
 			ctlr->imu_delta_samples_count = 0;
 			ctlr->imu_delta_samples_sum = 0;
 		}
 
+		/* don't ever want divide by zero shenanigans */
+		if (ctlr->imu_avg_delta_ms == 0) {
+			ctlr->imu_avg_delta_ms = 1;
+			hid_warn(ctlr->hdev, "calculated avg imu delta of 0\n");
+		}
+
 		/* useful for debugging IMU sample rate */
 		hid_dbg(ctlr->hdev,
 			"imu_report: ms=%u last_ms=%u delta=%u avg_delta=%u\n",
-- 
2.42.0

Re: [PATCH] HID: nintendo: Prevent divide-by-zero on code

[Jiri Kosina]

On Tue, 5 Dec 2023, Guilherme G. Piccoli wrote:

> It was reported [0] that adding a generic joycon to the system caused
> a kernel crash on Steam Deck, with the below panic spew:
> 
> divide error: 0000 [#1] PREEMPT SMP NOPTI
> [...]
> Hardware name: Valve Jupiter/Jupiter, BIOS F7A0119 10/24/2023
> RIP: 0010:nintendo_hid_event+0x340/0xcc1 [hid_nintendo]
> [...]
> Call Trace:
>  [...]
>  ? exc_divide_error+0x38/0x50
>  ? nintendo_hid_event+0x340/0xcc1 [hid_nintendo]
>  ? asm_exc_divide_error+0x1a/0x20
>  ? nintendo_hid_event+0x307/0xcc1 [hid_nintendo]
>  hid_input_report+0x143/0x160
>  hidp_session_run+0x1ce/0x700 [hidp]
> 
> Since it's a divide-by-0 error, by tracking the code for potential
> denominator issues, we've spotted 2 places in which this could happen;
> so let's guard against the possibility and log in the kernel if the
> condition happens. This is specially useful since some data that
> fills some denominators are read from the joycon HW in some cases,
> increasing the potential for flaws.
> 
> [0] https://github.com/ValveSoftware/SteamOS/issues/1070
> 
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>

Thanks a lot for the fix. Is it confirmed to fix the issue by either of 
the reporters? (that's not clear to me from the github issue).

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] HID: nintendo: Prevent divide-by-zero on code

[Guilherme G. Piccoli]

On 18/12/2023 12:50, Jiri Kosina wrote:
> [...]
> 
> Thanks a lot for the fix. Is it confirmed to fix the issue by either of 
> the reporters? (that's not clear to me from the github issue).
> 
> Thanks,
> 

Hi Jiri, thanks for the message!

Yes, not only in github but internally - Slouken got the same issue, and
this is fixed to him, so I'm CCing his email so we can have a Tested tag.

Cheers,


Guilherme

Re: [PATCH] HID: nintendo: Prevent divide-by-zero on code

[Jiri Kosina]

On Mon, 18 Dec 2023, Sam Lantinga wrote:

> Tested-by: Sam Lantinga <slouken@libsdl.org>

Thanks. Applied to hid.git#for-6.7/upstream-fixes

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] HID: nintendo: Prevent divide-by-zero on code

[Rahul Rameshbabu]

On Tue, 05 Dec, 2023 18:15:51 -0300 "Guilherme G. Piccoli" <gpiccoli@igalia.com> wrote:
> It was reported [0] that adding a generic joycon to the system caused
> a kernel crash on Steam Deck, with the below panic spew:
>
> divide error: 0000 [#1] PREEMPT SMP NOPTI
> [...]
> Hardware name: Valve Jupiter/Jupiter, BIOS F7A0119 10/24/2023
> RIP: 0010:nintendo_hid_event+0x340/0xcc1 [hid_nintendo]
> [...]
> Call Trace:
>  [...]
>  ? exc_divide_error+0x38/0x50
>  ? nintendo_hid_event+0x340/0xcc1 [hid_nintendo]
>  ? asm_exc_divide_error+0x1a/0x20
>  ? nintendo_hid_event+0x307/0xcc1 [hid_nintendo]
>  hid_input_report+0x143/0x160
>  hidp_session_run+0x1ce/0x700 [hidp]
>
> Since it's a divide-by-0 error, by tracking the code for potential
> denominator issues, we've spotted 2 places in which this could happen;
> so let's guard against the possibility and log in the kernel if the
> condition happens. This is specially useful since some data that
> fills some denominators are read from the joycon HW in some cases,
> increasing the potential for flaws.
>
> [0] https://github.com/ValveSoftware/SteamOS/issues/1070
>
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
> ---
>  drivers/hid/hid-nintendo.c | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index 138f154fecef..23f3f96c8c85 100644
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c

[...]

> @@ -1163,16 +1176,16 @@ static void joycon_parse_imu_report(struct joycon_ctlr *ctlr,
>  		    JC_IMU_SAMPLES_PER_DELTA_AVG) {
>  			ctlr->imu_avg_delta_ms = ctlr->imu_delta_samples_sum /
>  						 ctlr->imu_delta_samples_count;
> -			/* don't ever want divide by zero shenanigans */
> -			if (ctlr->imu_avg_delta_ms == 0) {
> -				ctlr->imu_avg_delta_ms = 1;
> -				hid_warn(ctlr->hdev,
> -					 "calculated avg imu delta of 0\n");
> -			}
>  			ctlr->imu_delta_samples_count = 0;
>  			ctlr->imu_delta_samples_sum = 0;
>  		}
>
> +		/* don't ever want divide by zero shenanigans */
> +		if (ctlr->imu_avg_delta_ms == 0) {
> +			ctlr->imu_avg_delta_ms = 1;
> +			hid_warn(ctlr->hdev, "calculated avg imu delta of 0\n");
> +		}
> +

Hi Guilherme,

I agree with the previous hunks you added and can see how those could
trigger the divide-by-zero issue you were seeing. However, I am not sure
if this hunk that I have left makes sense.

Reason being, is that the hid-nintendo driver has a special conditional
to prevent divide-by-zero in this case without this change.

1. If the first packet has not been received by the IMU, set
   imu_avg_delta_ms to JC_IMU_DFLT_AVG_DELTA_MS.
2. Only change imu_avg_delta_ms when imu_delta_samples_count >=
   JC_IMU_SAMPLES_PER_DELTA_AVG.
3. If that change leads to imu_avg_delta_ms being 0, set it to 1.

With this logic as-is, I do not see how a divide by zero could have
occurred in this specific path without your change. I might be missing
something, but I wanted to make sure to avoid integrating a hunk that
does not help.

Would it be possible to retest without this hunk?

>  		/* useful for debugging IMU sample rate */
>  		hid_dbg(ctlr->hdev,
>  			"imu_report: ms=%u last_ms=%u delta=%u avg_delta=%u\n",

--
Thanks,

Rahul Rameshbabu

Re: [PATCH] HID: nintendo: Prevent divide-by-zero on code

[Guilherme G. Piccoli]

On 18/12/2023 17:39, Rahul Rameshbabu wrote:
> [...]
> 
> Hi Guilherme,
> 
> I agree with the previous hunks you added and can see how those could
> trigger the divide-by-zero issue you were seeing. However, I am not sure
> if this hunk that I have left makes sense.
> 
> Reason being, is that the hid-nintendo driver has a special conditional
> to prevent divide-by-zero in this case without this change.
> 
> 1. If the first packet has not been received by the IMU, set
>    imu_avg_delta_ms to JC_IMU_DFLT_AVG_DELTA_MS.
> 2. Only change imu_avg_delta_ms when imu_delta_samples_count >=
>    JC_IMU_SAMPLES_PER_DELTA_AVG.
> 3. If that change leads to imu_avg_delta_ms being 0, set it to 1.
> 
> With this logic as-is, I do not see how a divide by zero could have
> occurred in this specific path without your change. I might be missing
> something, but I wanted to make sure to avoid integrating a hunk that
> does not help.
> 
> Would it be possible to retest without this hunk?
> 

Hi Rahul, thanks for your review.

I think I see ... I covered both bases in this patch, but IIUC after
your points above and better looking the code:

(a) imu_avg_delta_ms is set to JC_IMU_DFLT_AVG_DELTA_MS and it can only
change iff imu_delta_samples_count >= JC_IMU_SAMPLES_PER_DELTA_AVG.

(b) But the if path related with the imu_delta_samples_count is the one
that also guards the divide-by-zero, so effectively that error condition
cannot happen outside that if path, i.e., my hunk changed nothing.
Ugh...my bad.

At the same time, the hunk is harmless - it's up to Jiri to decide, we
can drop it (either directly by git rebasing or I can send a V2 if Jiri
prefers), or we can keep it.

I can try to test internally, github testing may be a bit uncertain in
the timeframe (specially during holidays season). If Jiri thinks the
hunk is harmless and no change is necessary, I'd rather not bother
people for testing now (I don't have the HW).

Cheers,


Guilherme

Re: [PATCH] HID: nintendo: Prevent divide-by-zero on code

[Rahul Rameshbabu]

On Mon, 18 Dec, 2023 18:46:09 -0300 "Guilherme G. Piccoli" <gpiccoli@igalia.com> wrote:
>
> Hi Rahul, thanks for your review.
>
> I think I see ... I covered both bases in this patch, but IIUC after
> your points above and better looking the code:
>
> (a) imu_avg_delta_ms is set to JC_IMU_DFLT_AVG_DELTA_MS and it can only
> change iff imu_delta_samples_count >= JC_IMU_SAMPLES_PER_DELTA_AVG.
>
> (b) But the if path related with the imu_delta_samples_count is the one
> that also guards the divide-by-zero, so effectively that error condition
> cannot happen outside that if path, i.e., my hunk changed nothing.
> Ugh...my bad.

Right, no worries. I really appreciate this patch though for covering
the cases involving the gyro and the accelerometer.

>
> At the same time, the hunk is harmless - it's up to Jiri to decide, we
> can drop it (either directly by git rebasing or I can send a V2 if Jiri
> prefers), or we can keep it.

Agreed.

>
> I can try to test internally, github testing may be a bit uncertain in
> the timeframe (specially during holidays season). If Jiri thinks the
> hunk is harmless and no change is necessary, I'd rather not bother
> people for testing now (I don't have the HW).
>

Makes sense. Like discussed above, the change here is harmless in nature
but has no functional change at the same time.

--
Thanks,

Rahul Rameshbabu

Re: [PATCH] HID: nintendo: Prevent divide-by-zero on code

[Jiri Kosina]

On Mon, 18 Dec 2023, Guilherme G. Piccoli wrote:

> I think I see ... I covered both bases in this patch, but IIUC after 
> your points above and better looking the code:
> 
> (a) imu_avg_delta_ms is set to JC_IMU_DFLT_AVG_DELTA_MS and it can only
> change iff imu_delta_samples_count >= JC_IMU_SAMPLES_PER_DELTA_AVG.
> 
> (b) But the if path related with the imu_delta_samples_count is the one
> that also guards the divide-by-zero, so effectively that error condition
> cannot happen outside that if path, i.e., my hunk changed nothing.
> Ugh...my bad.
> 
> At the same time, the hunk is harmless - it's up to Jiri to decide, we
> can drop it (either directly by git rebasing or I can send a V2 if Jiri
> prefers), or we can keep it.
> 
> I can try to test internally, github testing may be a bit uncertain in
> the timeframe (specially during holidays season). If Jiri thinks the
> hunk is harmless and no change is necessary, I'd rather not bother
> people for testing now (I don't have the HW).

My plan is to send this to Linus in a day or two, to have this fixed for 
good in 6.7 final to be on the safe side.

We can always remove the potentially superfluous check (thanks Rahul for 
spotting that) later once we get the testing results.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] HID: nintendo: Prevent divide-by-zero on code

[Guilherme G. Piccoli]

On 18/12/2023 19:27, Jiri Kosina wrote:
> On Mon, 18 Dec 2023, Guilherme G. Piccoli wrote:
> [...]
> My plan is to send this to Linus in a day or two, to have this fixed for 
> good in 6.7 final to be on the safe side.
> 
> We can always remove the potentially superfluous check (thanks Rahul for 
> spotting that) later once we get the testing results.
> 
> Thanks,
> 

Thanks Jiri and Rahul! I agree with the approach, better to have it
fixed ASAP indeed.

I understand no action is necessary from my side now.
Cheers,


Guilherme

Re: [PATCH] HID: nintendo: Prevent divide-by-zero on code

[Jiri Kosina]

On Mon, 18 Dec 2023, Guilherme G. Piccoli wrote:

> > My plan is to send this to Linus in a day or two, to have this fixed for 
> > good in 6.7 final to be on the safe side.
> > 
> > We can always remove the potentially superfluous check (thanks Rahul for 
> > spotting that) later once we get the testing results.
> > 
> > Thanks,
> > 
> 
> Thanks Jiri and Rahul! I agree with the approach, better to have it
> fixed ASAP indeed.
> 
> I understand no action is necessary from my side now.

Not immediately, but if you are able to eventually remove that 
likely-superfluous hunk with a Tested-by: tag, I'll happily merge that 
patch.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] HID: nintendo: Prevent divide-by-zero on code

[Guilherme G. Piccoli]

On 18/12/2023 20:49, Jiri Kosina wrote:
> [...]
> 
> Not immediately, but if you are able to eventually remove that 
> likely-superfluous hunk with a Tested-by: tag, I'll happily merge that 
> patch.
> 
> Thanks,
> 

Of course, I'll do that and mention that it was a suggestion from Rahul!
Thanks,


Guilherme