Re: [PATCH v2] carl9170: tx: fix an incorrect use of list iterator

From: Christian Lamparter <chunkeey@gmail.com>
To: Kalle Valo <kvalo@kernel.org>, Xiaomeng Tong <xiam0nd.tong@gmail.com>
Cc: davem@davemloft.net, kuba@kernel.org, pabeni@redhat.com,
	linville@tuxdriver.com, linux-wireless@vger.kernel.org,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	stable@vger.kernel.org
Subject: Re: [PATCH v2] carl9170: tx: fix an incorrect use of list iterator
Date: Sun, 1 May 2022 11:37:03 +0200	[thread overview]
Message-ID: <21e8a296-d756-78c7-8dc9-80fc416302c7@gmail.com> (raw)
In-Reply-To: <164914623778.12306.14074908465775082444.kvalo@kernel.org>

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