2020-06-12 07:39:23

by Jiri Slaby

[permalink] [raw]
Subject: [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: ffffae2c01bfa794
PF: supervisor write access in kernel mode
PF: error_code(0x0003) - permissions violation
PGD 107d51067 P4D 107d51067 PUD 107d52067 PMD 659ad2067 PTE 8000000662298161
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 <c7> 46 20 ff ff ff ff 4b 8d 14 40 48 c1 e2 04 48 8d b4 10 00 05 00
RSP: 0018:ffffae2c00417ce8 EFLAGS: 00010246
RAX: ffff8f0522334018 RBX: ffff8f0522334018 RCX: ffffffffc0fc26c0
RDX: 0000000000000000 RSI: ffffae2c01bfa774 RDI: ffffae2c01bfa774
RBP: 0000000000000000 R08: 0000000000000004 R09: 0000000000000001
R10: 0000000000000034 R11: ffffae2c01bfa77c R12: ffff8f0522334230
R13: 0000000001000009 R14: ffff8f0523fdbc00 R15: ffff8f051f395800
FS: 0000000000000000(0000) GS:ffff8f0527c80000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffffae2c01bfa794 CR3: 0000000389eba000 CR4: 00000000000006e0
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 <[email protected]>
Reported-by: Dieter Nützel <[email protected]>
Tested-by: Dieter Nützel <[email protected]>
Cc: Johannes Berg <[email protected]>
Cc: Emmanuel Grumbach <[email protected]>
Cc: Luca Coelho <[email protected]>
Cc: Intel Linux Wireless <[email protected]>
Cc: Kalle Valo <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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


2020-06-12 07:57:00

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH] iwl: fix crash in iwl_dbg_tlv_alloc_trigger

Jiri Slaby <[email protected]> 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: ffffae2c01bfa794
> PF: supervisor write access in kernel mode
> PF: error_code(0x0003) - permissions violation
> PGD 107d51067 P4D 107d51067 PUD 107d52067 PMD 659ad2067 PTE 8000000662298161
> 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 <c7> 46 20 ff ff ff ff 4b 8d 14 40 48 c1 e2 04 48 8d b4 10 00 05 00
> RSP: 0018:ffffae2c00417ce8 EFLAGS: 00010246
> RAX: ffff8f0522334018 RBX: ffff8f0522334018 RCX: ffffffffc0fc26c0
> RDX: 0000000000000000 RSI: ffffae2c01bfa774 RDI: ffffae2c01bfa774
> RBP: 0000000000000000 R08: 0000000000000004 R09: 0000000000000001
> R10: 0000000000000034 R11: ffffae2c01bfa77c R12: ffff8f0522334230
> R13: 0000000001000009 R14: ffff8f0523fdbc00 R15: ffff8f051f395800
> FS: 0000000000000000(0000) GS:ffff8f0527c80000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: ffffae2c01bfa794 CR3: 0000000389eba000 CR4: 00000000000006e0
> 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 <[email protected]>
> Reported-by: Dieter Nützel <[email protected]>
> Tested-by: Dieter Nützel <[email protected]>
> Cc: Johannes Berg <[email protected]>
> Cc: Emmanuel Grumbach <[email protected]>
> Cc: Luca Coelho <[email protected]>
> Cc: Intel Linux Wireless <[email protected]>
> Cc: Kalle Valo <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: Jakub Kicinski <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> 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

2020-06-12 08:21:19

by Luciano Coelho

[permalink] [raw]
Subject: 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 <[email protected]> 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: ffffae2c01bfa794
> > PF: supervisor write access in kernel mode
> > PF: error_code(0x0003) - permissions violation
> > PGD 107d51067 P4D 107d51067 PUD 107d52067 PMD 659ad2067 PTE 8000000662298161
> > 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 <c7> 46 20 ff ff ff ff 4b 8d 14 40 48 c1 e2 04 48 8d b4 10 00 05 00
> > RSP: 0018:ffffae2c00417ce8 EFLAGS: 00010246
> > RAX: ffff8f0522334018 RBX: ffff8f0522334018 RCX: ffffffffc0fc26c0
> > RDX: 0000000000000000 RSI: ffffae2c01bfa774 RDI: ffffae2c01bfa774
> > RBP: 0000000000000000 R08: 0000000000000004 R09: 0000000000000001
> > R10: 0000000000000034 R11: ffffae2c01bfa77c R12: ffff8f0522334230
> > R13: 0000000001000009 R14: ffff8f0523fdbc00 R15: ffff8f051f395800
> > FS: 0000000000000000(0000) GS:ffff8f0527c80000(0000) knlGS:0000000000000000
> > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: ffffae2c01bfa794 CR3: 0000000389eba000 CR4: 00000000000006e0
> > 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 <[email protected]>
> > Reported-by: Dieter Nützel <[email protected]>
> > Tested-by: Dieter Nützel <[email protected]>
> > Cc: Johannes Berg <[email protected]>
> > Cc: Emmanuel Grumbach <[email protected]>
> > Cc: Luca Coelho <[email protected]>
> > Cc: Intel Linux Wireless <[email protected]>
> > Cc: Kalle Valo <[email protected]>
> > Cc: "David S. Miller" <[email protected]>
> > Cc: Jakub Kicinski <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > ---
> > 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.

2020-06-23 08:28:38

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH] iwlwifi: fix crash in iwl_dbg_tlv_alloc_trigger

Jiri Slaby <[email protected]> wrote:

> 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: ffffae2c01bfa794
> PF: supervisor write access in kernel mode
> PF: error_code(0x0003) - permissions violation
> PGD 107d51067 P4D 107d51067 PUD 107d52067 PMD 659ad2067 PTE 8000000662298161
> 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 <c7> 46 20 ff ff ff ff 4b 8d 14 40 48 c1 e2 04 48 8d b4 10 00 05 00
> RSP: 0018:ffffae2c00417ce8 EFLAGS: 00010246
> RAX: ffff8f0522334018 RBX: ffff8f0522334018 RCX: ffffffffc0fc26c0
> RDX: 0000000000000000 RSI: ffffae2c01bfa774 RDI: ffffae2c01bfa774
> RBP: 0000000000000000 R08: 0000000000000004 R09: 0000000000000001
> R10: 0000000000000034 R11: ffffae2c01bfa77c R12: ffff8f0522334230
> R13: 0000000001000009 R14: ffff8f0523fdbc00 R15: ffff8f051f395800
> FS: 0000000000000000(0000) GS:ffff8f0527c80000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: ffffae2c01bfa794 CR3: 0000000389eba000 CR4: 00000000000006e0
> 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 <[email protected]>
> Reported-by: Dieter Nützel <[email protected]>
> Tested-by: Dieter Nützel <[email protected]>
> Cc: Johannes Berg <[email protected]>
> Cc: Emmanuel Grumbach <[email protected]>
> Cc: Luca Coelho <[email protected]>
> Cc: Intel Linux Wireless <[email protected]>
> Cc: Kalle Valo <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: Jakub Kicinski <[email protected]>
> Cc: [email protected]
> Cc: [email protected]

Patch applied to wireless-drivers.git, thanks.

ea0cca61d628 iwlwifi: fix crash in iwl_dbg_tlv_alloc_trigger

--
https://patchwork.kernel.org/patch/11601365/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches