| Message ID | 1518317805-5796-1-git-send-email-cjhuang@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
> -----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
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;
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;
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(+)