[PATCH] ath10k: fix use-after-free in ath10k_wmi_cmd_send_nowait

[PATCH] ath10k: fix use-after-free in ath10k_wmi_cmd_send_nowait

[Carl Huang]

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 <cjhuang@codeaurora.org>
---
 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


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: fix use-after-free in ath10k_wmi_cmd_send_nowait

[Felix Fietkau]

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 <cjhuang@codeaurora.org>
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

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

RE: [PATCH] ath10k: fix use-after-free in ath10k_wmi_cmd_send_nowait

[Carl Huang]

> -----Original Message-----
> From: ath10k [mailto:ath10k-bounces@lists.infradead.org] On Behalf Of Felix
> Fietkau
> Sent: Sunday, February 11, 2018 5:59 PM
> To: Carl Huang <cjhuang@codeaurora.org>; ath10k@lists.infradead.org
> Cc: linux-wireless@vger.kernel.org
> Subject: Re: [PATCH] ath10k: fix use-after-free in
> ath10k_wmi_cmd_send_nowait
> 
> 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 <cjhuang@codeaurora.org>
> 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.
> 
Agree that Moving the trace function before ath10k_htc_send is more simple, but then the parameter ret 
has no meaning and can't trace the return value of ath10k_htc_send.  Besides that, skb->data and skb->len 
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. 

> 
> _______________________________________________
> ath10k mailing list
> ath10k@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/ath10k

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: fix use-after-free in ath10k_wmi_cmd_send_nowait

[Brian Norris]

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 <cjhuang@codeaurora.org>
> ---
>  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 <briannorris@chromium.org>

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;

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k