Re: [PATCH] iwl: fix crash in iwl_dbg_tlv_alloc_trigger
On Fri, 2020-06-12 at 10:55 +0300, Kalle Valo wrote: > Jiri Slaby writes: > > > The tlv passed to iwl_dbg_tlv_alloc_trigger comes from a loaded firmware > > file. The memory can be marked as read-only as firmware could be > > shared. In anyway, writing to this memory is not expected. So, > > iwl_dbg_tlv_alloc_trigger can crash now: > > > > BUG: unable to handle page fault for address: ae2c01bfa794 > > PF: supervisor write access in kernel mode > > PF: error_code(0x0003) - permissions violation > > PGD 107d51067 P4D 107d51067 PUD 107d52067 PMD 659ad2067 PTE > > 800662298161 > > CPU: 2 PID: 161 Comm: kworker/2:1 Not tainted 5.7.0-3.gad96a07-default #1 > > openSUSE Tumbleweed (unreleased) > > RIP: 0010:iwl_dbg_tlv_alloc_trigger+0x25/0x60 [iwlwifi] > > Code: eb f2 0f 1f 00 66 66 66 66 90 83 7e 04 33 48 89 f8 44 8b 46 10 48 > > 89 f7 76 40 41 8d 50 ff 83 fa 19 77 23 8b 56 20 85 d2 75 07 46 20 ff > > ff ff ff 4b 8d 14 40 48 c1 e2 04 48 8d b4 10 00 05 00 > > RSP: 0018:ae2c00417ce8 EFLAGS: 00010246 > > RAX: 8f0522334018 RBX: 8f0522334018 RCX: c0fc26c0 > > RDX: RSI: ae2c01bfa774 RDI: ae2c01bfa774 > > RBP: R08: 0004 R09: 0001 > > R10: 0034 R11: ae2c01bfa77c R12: 8f0522334230 > > R13: 0109 R14: 8f0523fdbc00 R15: 8f051f395800 > > FS: () GS:8f0527c8() > > knlGS: > > CS: 0010 DS: ES: CR0: 80050033 > > CR2: ae2c01bfa794 CR3: 000389eba000 CR4: 06e0 > > Call Trace: > >iwl_dbg_tlv_alloc+0x79/0x120 [iwlwifi] > >iwl_parse_tlv_firmware.isra.0+0x57d/0x1550 [iwlwifi] > >iwl_req_fw_callback+0x3f8/0x6a0 [iwlwifi] > >request_firmware_work_func+0x47/0x90 > >process_one_work+0x1e3/0x3b0 > >worker_thread+0x46/0x340 > >kthread+0x115/0x140 > >ret_from_fork+0x1f/0x40 > > > > As can be seen, write bit is not set in the PTE. Read of > > trig->occurrences succeeds in iwl_dbg_tlv_alloc_trigger, but > > trig->occurrences = cpu_to_le32(-1); fails there, obviously. > > > > This is likely because we (at SUSE) use compressed firmware and that is > > marked as RO after decompression (see fw_map_paged_buf). > > > > Fix it by creating a temporary buffer in case we need to change the > > memory. > > > > Signed-off-by: Jiri Slaby > > Reported-by: Dieter Nützel > > Tested-by: Dieter Nützel > > Cc: Johannes Berg > > Cc: Emmanuel Grumbach > > Cc: Luca Coelho > > Cc: Intel Linux Wireless > > Cc: Kalle Valo > > Cc: "David S. Miller" > > Cc: Jakub Kicinski > > Cc: linux-wirel...@vger.kernel.org > > Cc: net...@vger.kernel.org > > --- > > drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c | 16 ++-- > > The prefix should be "iwlwifi: ", I can fix that. > > Luca, should I take this to wireless-drivers? Yeah, this looks good. Thanks, Jiri! And yes, Kalle, please apply it directly to w-d. Thank you! -- Cheers, Luca.
Re: [PATCH] iwl: fix crash in iwl_dbg_tlv_alloc_trigger
Jiri Slaby writes: > The tlv passed to iwl_dbg_tlv_alloc_trigger comes from a loaded firmware > file. The memory can be marked as read-only as firmware could be > shared. In anyway, writing to this memory is not expected. So, > iwl_dbg_tlv_alloc_trigger can crash now: > > BUG: unable to handle page fault for address: ae2c01bfa794 > PF: supervisor write access in kernel mode > PF: error_code(0x0003) - permissions violation > PGD 107d51067 P4D 107d51067 PUD 107d52067 PMD 659ad2067 PTE 800662298161 > CPU: 2 PID: 161 Comm: kworker/2:1 Not tainted 5.7.0-3.gad96a07-default #1 > openSUSE Tumbleweed (unreleased) > RIP: 0010:iwl_dbg_tlv_alloc_trigger+0x25/0x60 [iwlwifi] > Code: eb f2 0f 1f 00 66 66 66 66 90 83 7e 04 33 48 89 f8 44 8b 46 10 48 89 > f7 76 40 41 8d 50 ff 83 fa 19 77 23 8b 56 20 85 d2 75 07 46 20 ff ff ff > ff 4b 8d 14 40 48 c1 e2 04 48 8d b4 10 00 05 00 > RSP: 0018:ae2c00417ce8 EFLAGS: 00010246 > RAX: 8f0522334018 RBX: 8f0522334018 RCX: c0fc26c0 > RDX: RSI: ae2c01bfa774 RDI: ae2c01bfa774 > RBP: R08: 0004 R09: 0001 > R10: 0034 R11: ae2c01bfa77c R12: 8f0522334230 > R13: 0109 R14: 8f0523fdbc00 R15: 8f051f395800 > FS: () GS:8f0527c8() knlGS: > CS: 0010 DS: ES: CR0: 80050033 > CR2: ae2c01bfa794 CR3: 000389eba000 CR4: 06e0 > Call Trace: >iwl_dbg_tlv_alloc+0x79/0x120 [iwlwifi] >iwl_parse_tlv_firmware.isra.0+0x57d/0x1550 [iwlwifi] >iwl_req_fw_callback+0x3f8/0x6a0 [iwlwifi] >request_firmware_work_func+0x47/0x90 >process_one_work+0x1e3/0x3b0 >worker_thread+0x46/0x340 >kthread+0x115/0x140 >ret_from_fork+0x1f/0x40 > > As can be seen, write bit is not set in the PTE. Read of > trig->occurrences succeeds in iwl_dbg_tlv_alloc_trigger, but > trig->occurrences = cpu_to_le32(-1); fails there, obviously. > > This is likely because we (at SUSE) use compressed firmware and that is > marked as RO after decompression (see fw_map_paged_buf). > > Fix it by creating a temporary buffer in case we need to change the > memory. > > Signed-off-by: Jiri Slaby > Reported-by: Dieter Nützel > Tested-by: Dieter Nützel > Cc: Johannes Berg > Cc: Emmanuel Grumbach > Cc: Luca Coelho > Cc: Intel Linux Wireless > Cc: Kalle Valo > Cc: "David S. Miller" > Cc: Jakub Kicinski > Cc: linux-wirel...@vger.kernel.org > Cc: net...@vger.kernel.org > --- > drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c | 16 ++-- The prefix should be "iwlwifi: ", I can fix that. Luca, should I take this to wireless-drivers? -- https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
[PATCH] iwl: fix crash in iwl_dbg_tlv_alloc_trigger
The tlv passed to iwl_dbg_tlv_alloc_trigger comes from a loaded firmware
file. The memory can be marked as read-only as firmware could be
shared. In anyway, writing to this memory is not expected. So,
iwl_dbg_tlv_alloc_trigger can crash now:
BUG: unable to handle page fault for address: ae2c01bfa794
PF: supervisor write access in kernel mode
PF: error_code(0x0003) - permissions violation
PGD 107d51067 P4D 107d51067 PUD 107d52067 PMD 659ad2067 PTE 800662298161
CPU: 2 PID: 161 Comm: kworker/2:1 Not tainted 5.7.0-3.gad96a07-default #1
openSUSE Tumbleweed (unreleased)
RIP: 0010:iwl_dbg_tlv_alloc_trigger+0x25/0x60 [iwlwifi]
Code: eb f2 0f 1f 00 66 66 66 66 90 83 7e 04 33 48 89 f8 44 8b 46 10 48 89 f7
76 40 41 8d 50 ff 83 fa 19 77 23 8b 56 20 85 d2 75 07 46 20 ff ff ff ff 4b
8d 14 40 48 c1 e2 04 48 8d b4 10 00 05 00
RSP: 0018:ae2c00417ce8 EFLAGS: 00010246
RAX: 8f0522334018 RBX: 8f0522334018 RCX: c0fc26c0
RDX: RSI: ae2c01bfa774 RDI: ae2c01bfa774
RBP: R08: 0004 R09: 0001
R10: 0034 R11: ae2c01bfa77c R12: 8f0522334230
R13: 0109 R14: 8f0523fdbc00 R15: 8f051f395800
FS: () GS:8f0527c8() knlGS:
CS: 0010 DS: ES: CR0: 80050033
CR2: ae2c01bfa794 CR3: 000389eba000 CR4: 06e0
Call Trace:
iwl_dbg_tlv_alloc+0x79/0x120 [iwlwifi]
iwl_parse_tlv_firmware.isra.0+0x57d/0x1550 [iwlwifi]
iwl_req_fw_callback+0x3f8/0x6a0 [iwlwifi]
request_firmware_work_func+0x47/0x90
process_one_work+0x1e3/0x3b0
worker_thread+0x46/0x340
kthread+0x115/0x140
ret_from_fork+0x1f/0x40
As can be seen, write bit is not set in the PTE. Read of
trig->occurrences succeeds in iwl_dbg_tlv_alloc_trigger, but
trig->occurrences = cpu_to_le32(-1); fails there, obviously.
This is likely because we (at SUSE) use compressed firmware and that is
marked as RO after decompression (see fw_map_paged_buf).
Fix it by creating a temporary buffer in case we need to change the
memory.
Signed-off-by: Jiri Slaby
Reported-by: Dieter Nützel
Tested-by: Dieter Nützel
Cc: Johannes Berg
Cc: Emmanuel Grumbach
Cc: Luca Coelho
Cc: Intel Linux Wireless
Cc: Kalle Valo
Cc: "David S. Miller"
Cc: Jakub Kicinski
Cc: linux-wirel...@vger.kernel.org
Cc: net...@vger.kernel.org
---
drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c | 16 ++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c
b/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c
index 7987a288917b..27116c7d3f4f 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c
@@ -271,6 +271,8 @@ static int iwl_dbg_tlv_alloc_trigger(struct iwl_trans
*trans,
{
struct iwl_fw_ini_trigger_tlv *trig = (void *)tlv->data;
u32 tp = le32_to_cpu(trig->time_point);
+ struct iwl_ucode_tlv *dup = NULL;
+ int ret;
if (le32_to_cpu(tlv->length) < sizeof(*trig))
return -EINVAL;
@@ -283,10 +285,20 @@ static int iwl_dbg_tlv_alloc_trigger(struct iwl_trans
*trans,
return -EINVAL;
}
- if (!le32_to_cpu(trig->occurrences))
+ if (!le32_to_cpu(trig->occurrences)) {
+ dup = kmemdup(tlv, sizeof(*tlv) + le32_to_cpu(tlv->length),
+ GFP_KERNEL);
+ if (!dup)
+ return -ENOMEM;
+ trig = (void *)dup->data;
trig->occurrences = cpu_to_le32(-1);
+ tlv = dup;
+ }
+
+ ret = iwl_dbg_tlv_add(tlv, &trans->dbg.time_point[tp].trig_list);
+ kfree(dup);
- return iwl_dbg_tlv_add(tlv, &trans->dbg.time_point[tp].trig_list);
+ return ret;
}
static int (*dbg_tlv_alloc[])(struct iwl_trans *trans,
--
2.27.0
