| Message ID | 20231205211628.993129-1-gpiccoli@igalia.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: nintendo: Prevent divide-by-zero on code | expand |
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,
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
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
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
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
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
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,
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
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,
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
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",
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(-)