The skb may be freed in tx completion context before
trace_ath10k_wmi_cmd is called. This can be easily captured when
KASAN(Kernel Address Sanitizer) is enabled. The fix is to move
trace_ath10k_wmi_cmd before the send operation. As the ret has no
meaning in trace_ath10k_wmi_cmd then, so remove this parameter too.
Signed-off-by: Carl Huang <[email protected]>
---
drivers/net/wireless/ath/ath10k/trace.h | 12 ++++--------
drivers/net/wireless/ath/ath10k/wmi.c | 2 +-
2 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/trace.h b/drivers/net/wireless/ath/ath10k/trace.h
index e40edce..7d2fac3 100644
--- a/drivers/net/wireless/ath/ath10k/trace.h
+++ b/drivers/net/wireless/ath/ath10k/trace.h
@@ -152,10 +152,9 @@ TRACE_EVENT(ath10k_log_dbg_dump,
);
TRACE_EVENT(ath10k_wmi_cmd,
- TP_PROTO(struct ath10k *ar, int id, const void *buf, size_t buf_len,
- int ret),
+ TP_PROTO(struct ath10k *ar, int id, const void *buf, size_t buf_len),
- TP_ARGS(ar, id, buf, buf_len, ret),
+ TP_ARGS(ar, id, buf, buf_len),
TP_STRUCT__entry(
__string(device, dev_name(ar->dev))
@@ -163,7 +162,6 @@ TRACE_EVENT(ath10k_wmi_cmd,
__field(unsigned int, id)
__field(size_t, buf_len)
__dynamic_array(u8, buf, buf_len)
- __field(int, ret)
),
TP_fast_assign(
@@ -171,17 +169,15 @@ TRACE_EVENT(ath10k_wmi_cmd,
__assign_str(driver, dev_driver_string(ar->dev));
__entry->id = id;
__entry->buf_len = buf_len;
- __entry->ret = ret;
memcpy(__get_dynamic_array(buf), buf, buf_len);
),
TP_printk(
- "%s %s id %d len %zu ret %d",
+ "%s %s id %d len %zu",
__get_str(driver),
__get_str(device),
__entry->id,
- __entry->buf_len,
- __entry->ret
+ __entry->buf_len
)
);
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index 58dc218..fc9f50d 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -1742,8 +1742,8 @@ int ath10k_wmi_cmd_send_nowait(struct ath10k *ar, struct sk_buff *skb,
cmd_hdr->cmd_id = __cpu_to_le32(cmd);
memset(skb_cb, 0, sizeof(*skb_cb));
+ trace_ath10k_wmi_cmd(ar, cmd_id, skb->data, skb->len);
ret = ath10k_htc_send(&ar->htc, ar->wmi.eid, skb);
- trace_ath10k_wmi_cmd(ar, cmd_id, skb->data, skb->len, ret);
if (ret)
goto err_pull;
--
2.7.4
Carl Huang <[email protected]> wrote:
> The skb may be freed in tx completion context before
> trace_ath10k_wmi_cmd is called. This can be easily captured when
> KASAN(Kernel Address Sanitizer) is enabled. The fix is to move
> trace_ath10k_wmi_cmd before the send operation. As the ret has no
> meaning in trace_ath10k_wmi_cmd then, so remove this parameter too.
>
> Signed-off-by: Carl Huang <[email protected]>
> Tested-by: Brian Norris <[email protected]>
> Reviewed-by: Brian Norris <[email protected]>
> Signed-off-by: Kalle Valo <[email protected]>
Patch applied to ath-next branch of ath.git, thanks.
9ef0f58ed7b4 ath10k: fix use-after-free in ath10k_wmi_cmd_send_nowait
--
https://patchwork.kernel.org/patch/10258179/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
+ Felix, who had feedback on the last version
On Mon, Mar 05, 2018 at 02:44:02PM +0800, Carl Huang wrote:
> The skb may be freed in tx completion context before
> trace_ath10k_wmi_cmd is called. This can be easily captured when
> KASAN(Kernel Address Sanitizer) is enabled. The fix is to move
> trace_ath10k_wmi_cmd before the send operation. As the ret has no
> meaning in trace_ath10k_wmi_cmd then, so remove this parameter too.
>
> Signed-off-by: Carl Huang <[email protected]>
> ---
> drivers/net/wireless/ath/ath10k/trace.h | 12 ++++--------
> drivers/net/wireless/ath/ath10k/wmi.c | 2 +-
> 2 files changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/trace.h b/drivers/net/wireless/ath/ath10k/trace.h
> index e40edce..7d2fac3 100644
> --- a/drivers/net/wireless/ath/ath10k/trace.h
> +++ b/drivers/net/wireless/ath/ath10k/trace.h
> @@ -152,10 +152,9 @@ TRACE_EVENT(ath10k_log_dbg_dump,
> );
>
> TRACE_EVENT(ath10k_wmi_cmd,
> - TP_PROTO(struct ath10k *ar, int id, const void *buf, size_t buf_len,
> - int ret),
> + TP_PROTO(struct ath10k *ar, int id, const void *buf, size_t buf_len),
>
> - TP_ARGS(ar, id, buf, buf_len, ret),
> + TP_ARGS(ar, id, buf, buf_len),
>
> TP_STRUCT__entry(
> __string(device, dev_name(ar->dev))
> @@ -163,7 +162,6 @@ TRACE_EVENT(ath10k_wmi_cmd,
> __field(unsigned int, id)
> __field(size_t, buf_len)
> __dynamic_array(u8, buf, buf_len)
> - __field(int, ret)
> ),
>
> TP_fast_assign(
> @@ -171,17 +169,15 @@ TRACE_EVENT(ath10k_wmi_cmd,
> __assign_str(driver, dev_driver_string(ar->dev));
> __entry->id = id;
> __entry->buf_len = buf_len;
> - __entry->ret = ret;
> memcpy(__get_dynamic_array(buf), buf, buf_len);
> ),
>
> TP_printk(
> - "%s %s id %d len %zu ret %d",
> + "%s %s id %d len %zu",
> __get_str(driver),
> __get_str(device),
> __entry->id,
> - __entry->buf_len,
> - __entry->ret
> + __entry->buf_len
> )
> );
>
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
> index 58dc218..fc9f50d 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.c
> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> @@ -1742,8 +1742,8 @@ int ath10k_wmi_cmd_send_nowait(struct ath10k *ar, struct sk_buff *skb,
> cmd_hdr->cmd_id = __cpu_to_le32(cmd);
>
> memset(skb_cb, 0, sizeof(*skb_cb));
> + trace_ath10k_wmi_cmd(ar, cmd_id, skb->data, skb->len);
> ret = ath10k_htc_send(&ar->htc, ar->wmi.eid, skb);
> - trace_ath10k_wmi_cmd(ar, cmd_id, skb->data, skb->len, ret);
>
> if (ret)
> goto err_pull;
Tested-by: Brian Norris <[email protected]>
Reviewed-by: Brian Norris <[email protected]>