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

[Rahul Rameshbabu <sergeantsagara@protonmail.com>]

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