| Message ID | 20220328122820.1004-1-xiam0nd.tong@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 54a6f29522da3c914da30e50721dedf51046449a |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v2] carl9170: tx: fix an incorrect use of list iterator | expand |
Xiaomeng Tong <xiam0nd.tong@gmail.com> wrote: > If the previous list_for_each_entry_continue_rcu() don't exit early > (no goto hit inside the loop), the iterator 'cvif' after the loop > will be a bogus pointer to an invalid structure object containing > the HEAD (&ar->vif_list). As a result, the use of 'cvif' after that > will lead to a invalid memory access (i.e., 'cvif->id': the invalid > pointer dereference when return back to/after the callsite in the > carl9170_update_beacon()). > > The original intention should have been to return the valid 'cvif' > when found in list, NULL otherwise. So just return NULL when no > entry found, to fix this bug. > > Cc: stable@vger.kernel.org > Fixes: 1f1d9654e183c ("carl9170: refactor carl9170_update_beacon") > Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com> > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> Christian, is this ok to take?
On 05/04/2022 10:10, Kalle Valo wrote: > Xiaomeng Tong <xiam0nd.tong@gmail.com> wrote: > >> If the previous list_for_each_entry_continue_rcu() don't exit early >> (no goto hit inside the loop), the iterator 'cvif' after the loop >> will be a bogus pointer to an invalid structure object containing >> the HEAD (&ar->vif_list). As a result, the use of 'cvif' after that >> will lead to a invalid memory access (i.e., 'cvif->id': the invalid >> pointer dereference when return back to/after the callsite in the >> carl9170_update_beacon()). >> >> The original intention should have been to return the valid 'cvif' >> when found in list, NULL otherwise. So just return NULL when no >> entry found, to fix this bug. >> >> Cc: stable@vger.kernel.org >> Fixes: 1f1d9654e183c ("carl9170: refactor carl9170_update_beacon") >> Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com> >> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> > > Christian, is this ok to take? > First things first: Acked-by: Christian Lamparter <chunkeey@gmail.com> patch is OK as is. In theory, the "return NULL;" could be moved one line down (i.e.:after the closing bracket). This would make it so it would cover the surrounding if (ar->vifs > 0 && cvif) { ... } check too. But I can't tell you whenever this move actually overs extra protection (the function isn't called if there isn't already a virtual interface present). As for the BUG that this patch addresses. It is possible to trigger it: the device needs to have a primary AP/Ad-hoc or Mesh-Interface and the firmware needs to send a rogue PRETBTT-Event before any beaconing is setup. In my test case (changed the firmware to send out PRETBTT events every 100 ms as soon as it starts running). It didn't crash outright after setting up the AP interface. But I managed to see the corruptions, once I reloaded the module: [ 958.763802] BUG: kernel NULL pointer dereference, address: 0000000000000000 [ 958.764560] #PF: supervisor read access in kernel mode [ 958.765246] #PF: error_code(0x0000) - not-present page [ 958.765550] carl9170 3-2:1.0 wlx001f33fcd15b: renamed from wlan0 [ 958.765841] PGD 0 P4D 0 [ 958.766985] Oops: 0000 [#1] PREEMPT SMP NOPTI [ 958.767063] CPU: 3 PID: 716 Comm: kworker/3:2 Tainted: G OE 5.18.0-rc4-wt #50 [ 958.767063] Workqueue: events request_firmware_work_func [ 958.767063] RIP: 0010:strcmp+0xc/0x20 [ 958.767063] Code: 75 f7 31 d2 44 0f b6 04 16 44 88 04 11 48 83 c2 01 45 84 c0 75 ee c3 cc 66 0f 1f 44 00 00 31 c0 eb 08 48 83 c0 01 84 d2 74 > systemd-udevd[5870]: regulatory.0: Process '/lib/crda/crda' failed with exit code 255. [ 958.767063] RSP: 0018:ffffa720026b7d90 EFLAGS: 00010246 [ 958.767063] RAX: 0000000000000000 RBX: ffff980c2e96fd98 RCX: 0000000000000001 [ 958.767063] RDX: 0000000000000074 RSI: ffff980c0fd73e10 RDI: 0000000000000000 [ 958.767063] RBP: ffff980c0fd73d98 R08: ffffa720026b7d50 R09: ffff980c17a0f640 [ 958.767063] R10: ffffffffffffffff R11: ffff980c982e82b7 R12: ffff980c0fd73e10 [ 958.767063] R13: ffff980c128de0a8 R14: ffff980c0fd73788 R15: ffff980c0fd720c0 [ 958.767063] FS: 0000000000000000(0000) GS:ffff980eff8c0000(0000) knlGS:0000000000000000 [ 958.767063] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 958.767063] CR2: 0000000000000000 CR3: 000000012e924000 CR4: 0000000000350ee0 [ 958.767063] Call Trace: [ 958.767063] <TASK> [ 958.767063] hwrng_register+0x5c/0x1a0 [ 958.767063] devm_hwrng_register+0x3e/0x80 [ 958.767063] carl9170_register+0x3ea/0x560 [carl9170] [ 958.767063] carl9170_usb_firmware_step2+0xaf/0xf0 [carl9170] [ 958.767063] request_firmware_work_func+0x48/0x90 [ 958.767063] process_one_work+0x1bd/0x310 [ 958.767063] ? rescuer_thread+0x390/0x390 [ 958.767063] worker_thread+0x4b/0x390 [ 958.767063] ? rescuer_thread+0x390/0x390 ... with the "return NULL;" in place, no crashes happened anymore when reloading the module in the same setup. Cheers, Christian
Xiaomeng Tong <xiam0nd.tong@gmail.com> wrote: > If the previous list_for_each_entry_continue_rcu() don't exit early > (no goto hit inside the loop), the iterator 'cvif' after the loop > will be a bogus pointer to an invalid structure object containing > the HEAD (&ar->vif_list). As a result, the use of 'cvif' after that > will lead to a invalid memory access (i.e., 'cvif->id': the invalid > pointer dereference when return back to/after the callsite in the > carl9170_update_beacon()). > > The original intention should have been to return the valid 'cvif' > when found in list, NULL otherwise. So just return NULL when no > entry found, to fix this bug. > > Cc: stable@vger.kernel.org > Fixes: 1f1d9654e183c ("carl9170: refactor carl9170_update_beacon") > Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com> > Acked-by: Christian Lamparter <chunkeey@gmail.com> > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> Patch applied to ath-next branch of ath.git, thanks. 54a6f29522da carl9170: tx: fix an incorrect use of list iterator
diff --git a/drivers/net/wireless/ath/carl9170/tx.c b/drivers/net/wireless/ath/carl9170/tx.c index 1b76f4434c06..791f9f120af3 100644 --- a/drivers/net/wireless/ath/carl9170/tx.c +++ b/drivers/net/wireless/ath/carl9170/tx.c @@ -1558,6 +1558,9 @@ static struct carl9170_vif_info *carl9170_pick_beaconing_vif(struct ar9170 *ar) goto out; } } while (ar->beacon_enabled && i--); + + /* no entry found in list */ + return NULL; } out:
If the previous list_for_each_entry_continue_rcu() don't exit early (no goto hit inside the loop), the iterator 'cvif' after the loop will be a bogus pointer to an invalid structure object containing the HEAD (&ar->vif_list). As a result, the use of 'cvif' after that will lead to a invalid memory access (i.e., 'cvif->id': the invalid pointer dereference when return back to/after the callsite in the carl9170_update_beacon()). The original intention should have been to return the valid 'cvif' when found in list, NULL otherwise. So just return NULL when no entry found, to fix this bug. Cc: stable@vger.kernel.org Fixes: 1f1d9654e183c ("carl9170: refactor carl9170_update_beacon") Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com> --- changes since v1: - just return NULL when no entry found (Christian Lamparter) v1:https://lore.kernel.org/lkml/20220327072947.10744-1-xiam0nd.tong@gmail.com/ --- drivers/net/wireless/ath/carl9170/tx.c | 3 +++ 1 file changed, 3 insertions(+)