| Message ID | 20190906185931.19288-1-navid.emamdoost@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 728c1e2a05e4b5fc52fab3421dce772a806612a2 |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | ath9k: release allocated buffer if timed out | expand |
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
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
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
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.
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
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
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
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 :)
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; }
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(+)