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 add a reference count to the skb and release it after
trace_ath10k_wmi_cmd is called.
Signed-off-by: Carl Huang <[email protected]>
---
drivers/net/wireless/ath/ath10k/wmi.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index 58dc218..e63aedb 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -1,6 +1,7 @@
/*
* Copyright (c) 2005-2011 Atheros Communications Inc.
* Copyright (c) 2011-2017 Qualcomm Atheros, Inc.
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
*
* Permission to use, copy, modify, and/or distribute this software for any
* purpose with or without fee is hereby granted, provided that the above
@@ -1742,8 +1743,10 @@ 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));
+ skb_get(skb);
ret = ath10k_htc_send(&ar->htc, ar->wmi.eid, skb);
trace_ath10k_wmi_cmd(ar, cmd_id, skb->data, skb->len, ret);
+ dev_kfree_skb(skb);
if (ret)
goto err_pull;
--
2.7.4
> -----Original Message-----
> From: ath10k [mailto:[email protected]] On Behalf Of Fel=
ix
> Fietkau
> Sent: Sunday, February 11, 2018 5:59 PM
> To: Carl Huang <[email protected]>; [email protected]
> Cc: [email protected]
> Subject: Re: [PATCH] ath10k: fix use-after-free in
> ath10k_wmi_cmd_send_nowait
>=20
> On 2018-02-11 03:56, 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 add a
> > reference count to the skb and release it after trace_ath10k_wmi_cmd
> > is called.
> >
> > Signed-off-by: Carl Huang <[email protected]>
> I think it makes more sense to simply call the trace function before
> ath10k_htc_send. Also, for a trivial change like this it probably does no=
t make
> sense to add a Copyright line either.
>=20
Agree that Moving the trace function before ath10k_htc_send is more simple,=
but then the parameter ret=20
has no meaning and can't trace the return value of ath10k_htc_send. Beside=
s that, skb->data and skb->len=20
have different value if putting trace function before ath10k_htc_send, and =
this may be fine since the main
purpose is to trace the wmi information and not htc header information.
For the Copyright line, need Kalle Valo to comment.=20
>=20
> _______________________________________________
> ath10k mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/ath10k
On 2018-02-11 03:56, 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 add a reference count to the skb and release it after
> trace_ath10k_wmi_cmd is called.
>
> Signed-off-by: Carl Huang <[email protected]>
I think it makes more sense to simply call the trace function before
ath10k_htc_send. Also, for a trivial change like this it probably does
not make sense to add a Copyright line either.
- Felix
On Sun, Feb 11, 2018 at 10:56:45AM +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 add a reference count to the skb and release it after
> trace_ath10k_wmi_cmd is called.
>
> Signed-off-by: Carl Huang <[email protected]>
> ---
> drivers/net/wireless/ath/ath10k/wmi.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
> index 58dc218..e63aedb 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.c
> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> @@ -1,6 +1,7 @@
> /*
> * Copyright (c) 2005-2011 Atheros Communications Inc.
> * Copyright (c) 2011-2017 Qualcomm Atheros, Inc.
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
I agree with Felix that this seems excessive for a 2-line bugfix. But
I'm not a lawyer or a maintainer, and I have no power here :)
(On the practical side: it's annoying that the copyright line has to be
the one to provide conflicts when backporting.)
> *
> * Permission to use, copy, modify, and/or distribute this software for any
> * purpose with or without fee is hereby granted, provided that the above
> @@ -1742,8 +1743,10 @@ 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));
> + skb_get(skb);
> ret = ath10k_htc_send(&ar->htc, ar->wmi.eid, skb);
> trace_ath10k_wmi_cmd(ar, cmd_id, skb->data, skb->len, ret);
> + dev_kfree_skb(skb);
Tested-by: Brian Norris <[email protected]>
It does feel like capturing before the command is sent might make more
sense. Otherwise, you may be concurrently dumping the buffer and DMA'ing
to a device. If you really need to trace the return code, you could do
that separately.
Brian
>
> if (ret)
> goto err_pull;