[PATCH] ath9k: release allocated buffer if timed out

[PATCH] ath9k: release allocated buffer if timed out

[Navid Emamdoost]

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

Re: [PATCH] ath9k: release allocated buffer if timed out

[Kalle Valo]

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

Re: [PATCH] ath9k: release allocated buffer if timed out

[Brian Norris]

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

Re: [PATCH] ath9k: release allocated buffer if timed out

[Navid Emamdoost]

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.

Re: [PATCH] ath9k: release allocated buffer if timed out

[Kalle Valo]

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

Re: [PATCH] ath9k: release allocated buffer if timed out

[Brian Norris]

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

Re: [PATCH] ath9k: release allocated buffer if timed out

[Brian Norris]

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

Re: [PATCH] ath9k: release allocated buffer if timed out

[Brian Norris]

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

Re: [PATCH] ath9k: release allocated buffer if timed out

[Kalle Valo]

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