Re: [PATCH] ath9k: release allocated buffer if timed out
Brian Norris writes: > On Wed, May 13, 2020 at 12:02 PM Brian Norris > wrote: >> >> On Wed, May 13, 2020 at 12:05 AM Kalle Valo 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 sta...@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
Re: [PATCH] ath9k: release allocated buffer if timed out
On Wed, May 13, 2020 at 12:02 PM Brian Norris wrote: > > On Wed, May 13, 2020 at 12:05 AM Kalle Valo 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 sta...@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
On Tue, May 12, 2020 at 8:25 PM Navid Emamdoost 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
On Wed, May 13, 2020 at 12:05 AM Kalle Valo 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 sta...@vger.kernel.org? Brian
Re: [PATCH] ath9k: release allocated buffer if timed out
Brian Norris writes: > On Fri, Sep 6, 2019 at 11:59 AM Navid Emamdoost > 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 > > 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
Hi Brian, On Tue, May 12, 2020 at 11:57 AM Brian Norris wrote: > > On Fri, Sep 6, 2019 at 11:59 AM Navid Emamdoost > 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 > > 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
On Fri, Sep 6, 2019 at 11:59 AM Navid Emamdoost 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 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 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 > Signed-off-by: Kalle Valo 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
