Linux-Wireless Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] ath9k: release allocated buffer if timed out
@ 2019-09-06 18:59 Navid Emamdoost
  2019-09-10 13:32 ` Kalle Valo
  2020-05-12 16:56 ` Brian Norris
  0 siblings, 2 replies; 9+ messages in thread
From: Navid Emamdoost @ 2019-09-06 18:59 UTC (permalink / raw)
  Cc: emamd001, smccaman, kjlu, Navid Emamdoost, QCA ath9k Development,
	Kalle Valo, David S. Miller, linux-wireless, netdev,
	linux-kernel

In ath9k_wmi_cmd, the allocated network buffer needs to be released
if timeout happens. Otherwise memory will be leaked.

Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
 drivers/net/wireless/ath/ath9k/wmi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/ath/ath9k/wmi.c b/drivers/net/wireless/ath/ath9k/wmi.c
index d1f6710ca63b..cdc146091194 100644
--- a/drivers/net/wireless/ath/ath9k/wmi.c
+++ b/drivers/net/wireless/ath/ath9k/wmi.c
@@ -336,6 +336,7 @@ int ath9k_wmi_cmd(struct wmi *wmi, enum wmi_cmd_id cmd_id,
 		ath_dbg(common, WMI, "Timeout waiting for WMI command: %s\n",
 			wmi_cmd_to_name(cmd_id));
 		mutex_unlock(&wmi->op_mutex);
+		kfree_skb(skb);
 		return -ETIMEDOUT;
 	}
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ath9k: release allocated buffer if timed out
  2019-09-06 18:59 [PATCH] ath9k: release allocated buffer if timed out Navid Emamdoost
@ 2019-09-10 13:32 ` Kalle Valo
  2020-05-12 16:56 ` Brian Norris
  1 sibling, 0 replies; 9+ messages in thread
From: Kalle Valo @ 2019-09-10 13:32 UTC (permalink / raw)
  To: Navid Emamdoost

Navid Emamdoost <navid.emamdoost@gmail.com> wrote:

> In ath9k_wmi_cmd, the allocated network buffer needs to be released
> if timeout happens. Otherwise memory will be leaked.
> 
> Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>

Patch applied to ath-next branch of ath.git, thanks.

728c1e2a05e4 ath9k: release allocated buffer if timed out

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

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ath9k: release allocated buffer if timed out
  2019-09-06 18:59 [PATCH] ath9k: release allocated buffer if timed out Navid Emamdoost
  2019-09-10 13:32 ` Kalle Valo
@ 2020-05-12 16:56 ` Brian Norris
  2020-05-13  3:24   ` Navid Emamdoost
  2020-05-13  7:05   ` Kalle Valo
  1 sibling, 2 replies; 9+ messages in thread
From: Brian Norris @ 2020-05-12 16:56 UTC (permalink / raw)
  To: Navid Emamdoost
  Cc: emamd001, smccaman, Kangjie Lu, QCA ath9k Development,
	Kalle Valo, David S. Miller, linux-wireless,
	<netdev@vger.kernel.org>,
	Linux Kernel

On Fri, Sep 6, 2019 at 11:59 AM Navid Emamdoost
<navid.emamdoost@gmail.com> wrote:
>
> In ath9k_wmi_cmd, the allocated network buffer needs to be released
> if timeout happens. Otherwise memory will be leaked.
>
> Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>

I wonder, did you actually test your patches? I ask, because it seems
that all your patches are of the same mechanical variety (produced by
some sort of research project?), and if I look around a bit, I see
several mistakes and regressions noted on your other patches. And
recently, I see someone reporting a 5.4 kernel regression, which looks
a lot like it was caused by this patch:

https://bugzilla.kernel.org/show_bug.cgi?id=207703#c1

I'll propose a revert, if there's no evidence this was actually tested
or otherwise confirmed to fix a real bug.

Brian

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ath9k: release allocated buffer if timed out
  2020-05-12 16:56 ` Brian Norris
@ 2020-05-13  3:24   ` Navid Emamdoost
  2020-05-13 19:12     ` Brian Norris
  2020-05-13  7:05   ` Kalle Valo
  1 sibling, 1 reply; 9+ messages in thread
From: Navid Emamdoost @ 2020-05-13  3:24 UTC (permalink / raw)
  To: Brian Norris
  Cc: Navid Emamdoost, Stephen McCamant, Kangjie Lu,
	QCA ath9k Development, Kalle Valo, David S. Miller,
	linux-wireless, <netdev@vger.kernel.org>,
	Linux Kernel

Hi Brian,

On Tue, May 12, 2020 at 11:57 AM Brian Norris <briannorris@chromium.org> wrote:
>
> On Fri, Sep 6, 2019 at 11:59 AM Navid Emamdoost
> <navid.emamdoost@gmail.com> wrote:
> >
> > In ath9k_wmi_cmd, the allocated network buffer needs to be released
> > if timeout happens. Otherwise memory will be leaked.
> >
> > Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
>
> I wonder, did you actually test your patches? I ask, because it seems
> that all your patches are of the same mechanical variety (produced by
> some sort of research project?), and if I look around a bit, I see
I found this via static analysis and as a result, did had the inputs
to test it with (like the way fuzzing works).
It may be beneficial if you could point me to any testing
infrastructure that you use or are aware of for future cases.

> several mistakes and regressions noted on your other patches. And
> recently, I see someone reporting a 5.4 kernel regression, which looks
> a lot like it was caused by this patch:
>
> https://bugzilla.kernel.org/show_bug.cgi?id=207703#c1
>
> I'll propose a revert, if there's no evidence this was actually tested
> or otherwise confirmed to fix a real bug.
>
> Brian



-- 
Navid.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ath9k: release allocated buffer if timed out
  2020-05-12 16:56 ` Brian Norris
  2020-05-13  3:24   ` Navid Emamdoost
@ 2020-05-13  7:05   ` Kalle Valo
  2020-05-13 19:02     ` Brian Norris
  1 sibling, 1 reply; 9+ messages in thread
From: Kalle Valo @ 2020-05-13  7:05 UTC (permalink / raw)
  To: Brian Norris
  Cc: Navid Emamdoost, emamd001, smccaman, Kangjie Lu,
	QCA ath9k Development, David S. Miller, linux-wireless,
	<netdev@vger.kernel.org>,
	Linux Kernel

Brian Norris <briannorris@chromium.org> writes:

> On Fri, Sep 6, 2019 at 11:59 AM Navid Emamdoost
> <navid.emamdoost@gmail.com> wrote:
>>
>> In ath9k_wmi_cmd, the allocated network buffer needs to be released
>> if timeout happens. Otherwise memory will be leaked.
>>
>> Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
>
> I wonder, did you actually test your patches? I ask, because it seems
> that all your patches are of the same mechanical variety (produced by
> some sort of research project?), and if I look around a bit, I see
> several mistakes and regressions noted on your other patches. And
> recently, I see someone reporting a 5.4 kernel regression, which looks
> a lot like it was caused by this patch:
>
> https://bugzilla.kernel.org/show_bug.cgi?id=207703#c1
>
> I'll propose a revert, if there's no evidence this was actually tested
> or otherwise confirmed to fix a real bug.

Actually it's already reverted in -next, nobody just realised that it's
a regression from commit 728c1e2a05e4:

ced21a4c726b ath9k: Fix use-after-free Read in htc_connect_service

v5.8-rc1 should be the first release having the fix.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ath9k: release allocated buffer if timed out
  2020-05-13  7:05   ` Kalle Valo
@ 2020-05-13 19:02     ` Brian Norris
  2020-05-20 20:59       ` Brian Norris
  0 siblings, 1 reply; 9+ messages in thread
From: Brian Norris @ 2020-05-13 19:02 UTC (permalink / raw)
  To: Kalle Valo
  Cc: Navid Emamdoost, Navid Emamdoost, Stephen McCamant, Kangjie Lu,
	QCA ath9k Development, David S. Miller, linux-wireless,
	<netdev@vger.kernel.org>,
	Linux Kernel

On Wed, May 13, 2020 at 12:05 AM Kalle Valo <kvalo@codeaurora.org> wrote:
> Actually it's already reverted in -next, nobody just realised that it's
> a regression from commit 728c1e2a05e4:
>
> ced21a4c726b ath9k: Fix use-after-free Read in htc_connect_service

Nice.

> v5.8-rc1 should be the first release having the fix.

So I guess we have to wait until 5.8-rc1 (when this lands in mainline)
to send this manually to stable@vger.kernel.org?

Brian

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ath9k: release allocated buffer if timed out
  2020-05-13  3:24   ` Navid Emamdoost
@ 2020-05-13 19:12     ` Brian Norris
  0 siblings, 0 replies; 9+ messages in thread
From: Brian Norris @ 2020-05-13 19:12 UTC (permalink / raw)
  To: Navid Emamdoost
  Cc: Navid Emamdoost, Stephen McCamant, Kangjie Lu,
	QCA ath9k Development, Kalle Valo, David S. Miller,
	linux-wireless, <netdev@vger.kernel.org>,
	Linux Kernel

On Tue, May 12, 2020 at 8:25 PM Navid Emamdoost
<navid.emamdoost@gmail.com> wrote:
> I found this via static analysis and as a result, did had the inputs
> to test it with (like the way fuzzing works).

Fuzzing is dynamic analysis, so I'm not sure how that fits.

> It may be beneficial if you could point me to any testing
> infrastructure that you use or are aware of for future cases.

syzbot (a real fuzzer -- I believe it uses fake USB devices [1])
caught the error, apparently:
https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git/commit/?id=ced21a4c726bdc60b1680c050a284b08803bc64c
so you might look at using that too.

Traditionally, "testing your patches" means having hardware that runs
the driver in question when patching said driver. That likely won't
scale for researchers, but then, perhaps it just means you need to be
more clear on how you caught the issue and how you did (or didn't)
test it, so it's easier to reconcile your claims with the testing done
by real users.

If you only did static analysis, then we can be more confident in
reverting. The fuzz-tested revert is an even nicer bonus.

Brian

[1] https://github.com/google/syzkaller/blob/master/docs/syzbot.md#usb-bugs
https://github.com/google/syzkaller/blob/master/docs/linux/external_fuzzing_usb.md

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ath9k: release allocated buffer if timed out
  2020-05-13 19:02     ` Brian Norris
@ 2020-05-20 20:59       ` Brian Norris
  2020-05-22 10:34         ` Kalle Valo
  0 siblings, 1 reply; 9+ messages in thread
From: Brian Norris @ 2020-05-20 20:59 UTC (permalink / raw)
  To: Kalle Valo
  Cc: Navid Emamdoost, Navid Emamdoost, Stephen McCamant, Kangjie Lu,
	QCA ath9k Development, David S. Miller, linux-wireless,
	<netdev@vger.kernel.org>,
	Linux Kernel

On Wed, May 13, 2020 at 12:02 PM Brian Norris <briannorris@chromium.org> wrote:
>
> On Wed, May 13, 2020 at 12:05 AM Kalle Valo <kvalo@codeaurora.org> wrote:
> > Actually it's already reverted in -next, nobody just realised that it's
> > a regression from commit 728c1e2a05e4:
> >
> > ced21a4c726b ath9k: Fix use-after-free Read in htc_connect_service
>
> Nice.
>
> > v5.8-rc1 should be the first release having the fix.
>
> So I guess we have to wait until 5.8-rc1 (when this lands in mainline)
> to send this manually to stable@vger.kernel.org?

For the record, there are more reports of this, if I'm reading them right:

https://bugzilla.kernel.org/show_bug.cgi?id=207797

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ath9k: release allocated buffer if timed out
  2020-05-20 20:59       ` Brian Norris
@ 2020-05-22 10:34         ` Kalle Valo
  0 siblings, 0 replies; 9+ messages in thread
From: Kalle Valo @ 2020-05-22 10:34 UTC (permalink / raw)
  To: Brian Norris
  Cc: Navid Emamdoost, Navid Emamdoost, Stephen McCamant, Kangjie Lu,
	QCA ath9k Development, David S. Miller, linux-wireless,
	<netdev@vger.kernel.org>,
	Linux Kernel

Brian Norris <briannorris@chromium.org> writes:

> On Wed, May 13, 2020 at 12:02 PM Brian Norris <briannorris@chromium.org> wrote:
>>
>> On Wed, May 13, 2020 at 12:05 AM Kalle Valo <kvalo@codeaurora.org> wrote:
>> > Actually it's already reverted in -next, nobody just realised that it's
>> > a regression from commit 728c1e2a05e4:
>> >
>> > ced21a4c726b ath9k: Fix use-after-free Read in htc_connect_service
>>
>> Nice.
>>
>> > v5.8-rc1 should be the first release having the fix.
>>
>> So I guess we have to wait until 5.8-rc1 (when this lands in mainline)
>> to send this manually to stable@vger.kernel.org?

Yeah, following Option 2:

https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

> For the record, there are more reports of this, if I'm reading them right:
>
> https://bugzilla.kernel.org/show_bug.cgi?id=207797

Thanks for the followup, this case is a good example why small cleanup
patches are not always that simple and easy as some people claim :)

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-05-22 10:34 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-06 18:59 [PATCH] ath9k: release allocated buffer if timed out Navid Emamdoost
2019-09-10 13:32 ` Kalle Valo
2020-05-12 16:56 ` Brian Norris
2020-05-13  3:24   ` Navid Emamdoost
2020-05-13 19:12     ` Brian Norris
2020-05-13  7:05   ` Kalle Valo
2020-05-13 19:02     ` Brian Norris
2020-05-20 20:59       ` Brian Norris
2020-05-22 10:34         ` Kalle Valo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).