Arend van Spriel <[email protected]> 于2023年11月13日周一 17:18写道：
>
> On November 8, 2023 4:03:26 AM Zheng Hacker <[email protected]>
> wrote:
>
> > Arend Van Spriel <[email protected]> 于2023年11月6日周一 23:48写道：
> >>
> >> On November 6, 2023 3:44:53 PM Zheng Hacker <[email protected]> wrote:
> >>
> >>> Thanks! I didn't test it for I don't have a device. Very appreciated
> >>> if anyone could help with that.
> >>
> >> I would volunteer, but it made me dig deep and not sure if there is a
> >> problem to solve here.
> >>
> >> brcmf_cfg80211_detach() calls wl_deinit_priv() -> brcmf_abort_scanning() ->
> >> brcmf_notify_escan_complete() which does delete the timer.
> >>
> >> What am I missing here?
> >
> > Thanks four your detailed review. I did see the code and not sure if
> > brcmf_notify_escan_complete
> > would be triggered for sure. So in the first version I want to delete
> > the pending timer ahead of time.
>
> Why requesting a CVE when you are not sure? Seems a bit hasty to put it
> mildly.
I'm sure the issue exists because there's only cancler of timer but not woker.
As there's similar CVEs before like : https://github.com/V4bel/CVE-2022-41218,
I submit it as soon as I found it.
>
> > As I'm not very familiar with the logic here. I'm still not sure if we
> > should delete the timer_shutdown_sync.
> > Looking forward to your reply :)
>
> Reading the kerneldoc of timer_shutdown_sync() has the advantage that
> the timer can not be rearmed by another thread. However, that will only
> happen when a new scan is initiated in firmware, but the bus is already
> down so that can not happen. The only improvement (no bug fix!) I see
> here is to replace timer handling code in brcmf_notify_escan_complete():
>
> - if (timer_pending(&cfg->escan_timeout))
> - del_timer_sync(&cfg->escan_timeout);
> + timer_delete_sync(&cfg->escan_timeout);
>
Very thanks for your reviews and suggestions! I thinks it's a good
idea. I'll make
another patch sooner or later.
Best regards,
Zheng
> Regards,
> Arend
On November 15, 2023 4:00:46 PM Zheng Hacker <[email protected]> wrote:
> Arend van Spriel <[email protected]> 于2023年11月13日周一 17:18写道：
>>
>> On November 8, 2023 4:03:26 AM Zheng Hacker <[email protected]>
>> wrote:
>>
>>> Arend Van Spriel <[email protected]> 于2023年11月6日周一 23:48写道：
>>>>
>>>> On November 6, 2023 3:44:53 PM Zheng Hacker <[email protected]> wrote:
>>>>
>>>>> Thanks! I didn't test it for I don't have a device. Very appreciated
>>>>> if anyone could help with that.
>>>>
>>>> I would volunteer, but it made me dig deep and not sure if there is a
>>>> problem to solve here.
>>>>
>>>> brcmf_cfg80211_detach() calls wl_deinit_priv() -> brcmf_abort_scanning() ->
>>>> brcmf_notify_escan_complete() which does delete the timer.
>>>>
>>>> What am I missing here?
>>>
>>> Thanks four your detailed review. I did see the code and not sure if
>>> brcmf_notify_escan_complete
>>> would be triggered for sure. So in the first version I want to delete
>>> the pending timer ahead of time.
>>
>> Why requesting a CVE when you are not sure? Seems a bit hasty to put it
>> mildly.
>
> I'm sure the issue exists because there's only cancler of timer but not woker.
> As there's similar CVEs before like : https://github.com/V4bel/CVE-2022-41218,
> I submit it as soon as I found it.
Ah, yes. The cancel_work_sync() can also be done in
brcmf_notify_escan_complete().
Regards,
Arend
On Thu, 16 Nov 2023 19:20:06 +0100,
Arend Van Spriel wrote:
>
> On November 15, 2023 4:00:46 PM Zheng Hacker <[email protected]> wrote:
>
> > Arend van Spriel <[email protected]> $BP2(B2023$BG/(B11$B7n(B13$BF|<~0l(B 17:18$B<LF;!'(B
> >>
> >> On November 8, 2023 4:03:26 AM Zheng Hacker <[email protected]>
> >> wrote:
> >>
> >>> Arend Van Spriel <[email protected]> $BP2(B2023$BG/(B11$B7n(B6$BF|<~0l(B 23:48$B<LF;!'(B
> >>>>
> >>>> On November 6, 2023 3:44:53 PM Zheng Hacker <[email protected]> wrote:
> >>>>
> >>>>> Thanks! I didn't test it for I don't have a device. Very appreciated
> >>>>> if anyone could help with that.
> >>>>
> >>>> I would volunteer, but it made me dig deep and not sure if there is a
> >>>> problem to solve here.
> >>>>
> >>>> brcmf_cfg80211_detach() calls wl_deinit_priv() -> brcmf_abort_scanning() ->
> >>>> brcmf_notify_escan_complete() which does delete the timer.
> >>>>
> >>>> What am I missing here?
> >>>
> >>> Thanks four your detailed review. I did see the code and not sure if
> >>> brcmf_notify_escan_complete
> >>> would be triggered for sure. So in the first version I want to delete
> >>> the pending timer ahead of time.
> >>
> >> Why requesting a CVE when you are not sure? Seems a bit hasty to put it
> >> mildly.
> >
> > I'm sure the issue exists because there's only cancler of timer but not woker.
> > As there's similar CVEs before like : https://github.com/V4bel/CVE-2022-41218,
> > I submit it as soon as I found it.
>
> Ah, yes. The cancel_work_sync() can also be done in
> brcmf_notify_escan_complete().
AFAIUC, brcmf_notify_scan_complete() is called from the work itself,
too, hence you can't issue cancel_work_sync() there (unless you make
it conditional).
Takashi
On November 16, 2023 7:25:16 PM Takashi Iwai <[email protected]> wrote:
> On Thu, 16 Nov 2023 19:20:06 +0100,
> Arend Van Spriel wrote:
>>
>> On November 15, 2023 4:00:46 PM Zheng Hacker <[email protected]> wrote:
>>
>>> Arend van Spriel <[email protected]> 于2023年11月13日周一 17:18写道：
>>>>
>>>> On November 8, 2023 4:03:26 AM Zheng Hacker <[email protected]>
>>>> wrote:
>>>>
>>>>> Arend Van Spriel <[email protected]> 于2023年11月6日周一 23:48写道：
>>>>>>
>>>>>> On November 6, 2023 3:44:53 PM Zheng Hacker <[email protected]> wrote:
>>>>>>
>>>>>>> Thanks! I didn't test it for I don't have a device. Very appreciated
>>>>>>> if anyone could help with that.
>>>>>>
>>>>>> I would volunteer, but it made me dig deep and not sure if there is a
>>>>>> problem to solve here.
>>>>>>
>>>>>> brcmf_cfg80211_detach() calls wl_deinit_priv() -> brcmf_abort_scanning() ->
>>>>>> brcmf_notify_escan_complete() which does delete the timer.
>>>>>>
>>>>>> What am I missing here?
>>>>>
>>>>> Thanks four your detailed review. I did see the code and not sure if
>>>>> brcmf_notify_escan_complete
>>>>> would be triggered for sure. So in the first version I want to delete
>>>>> the pending timer ahead of time.
>>>>
>>>> Why requesting a CVE when you are not sure? Seems a bit hasty to put it
>>>> mildly.
>>>
>>> I'm sure the issue exists because there's only cancler of timer but not woker.
>>> As there's similar CVEs before like : https://github.com/V4bel/CVE-2022-41218,
>>> I submit it as soon as I found it.
>>
>> Ah, yes. The cancel_work_sync() can also be done in
>> brcmf_notify_escan_complete().
>
> AFAIUC, brcmf_notify_scan_complete() is called from the work itself,
> too, hence you can't issue cancel_work_sync() there (unless you make
> it conditional).
Hi Takashi,
You are obviously right. Let's wait and see what v6 will look like ;-)
Regards,
Arend
Yes, that makes this issue hard to fix. I was wondering why it binds the
worker with the timer rather than using just one of them.
Takashi Iwai <[email protected]> 于2023年11月17日周五 02:25写道：
>
> On Thu, 16 Nov 2023 19:20:06 +0100,
> Arend Van Spriel wrote:
> >
> > On November 15, 2023 4:00:46 PM Zheng Hacker <[email protected]> wrote:
> >
> > > Arend van Spriel <[email protected]> 于2023年11月13日周一 17:18写道：
> > >>
> > >> On November 8, 2023 4:03:26 AM Zheng Hacker <[email protected]>
> > >> wrote:
> > >>
> > >>> Arend Van Spriel <[email protected]> 于2023年11月6日周一 23:48写道：
> > >>>>
> > >>>> On November 6, 2023 3:44:53 PM Zheng Hacker <[email protected]> wrote:
> > >>>>
> > >>>>> Thanks! I didn't test it for I don't have a device. Very appreciated
> > >>>>> if anyone could help with that.
> > >>>>
> > >>>> I would volunteer, but it made me dig deep and not sure if there is a
> > >>>> problem to solve here.
> > >>>>
> > >>>> brcmf_cfg80211_detach() calls wl_deinit_priv() -> brcmf_abort_scanning() ->
> > >>>> brcmf_notify_escan_complete() which does delete the timer.
> > >>>>
> > >>>> What am I missing here?
> > >>>
> > >>> Thanks four your detailed review. I did see the code and not sure if
> > >>> brcmf_notify_escan_complete
> > >>> would be triggered for sure. So in the first version I want to delete
> > >>> the pending timer ahead of time.
> > >>
> > >> Why requesting a CVE when you are not sure? Seems a bit hasty to put it
> > >> mildly.
> > >
> > > I'm sure the issue exists because there's only cancler of timer but not woker.
> > > As there's similar CVEs before like : https://github.com/V4bel/CVE-2022-41218,
> > > I submit it as soon as I found it.
> >
> > Ah, yes. The cancel_work_sync() can also be done in
> > brcmf_notify_escan_complete().
>
> AFAIUC, brcmf_notify_scan_complete() is called from the work itself,
> too, hence you can't issue cancel_work_sync() there (unless you make
> it conditional).
>
>
> Takashi
On November 17, 2023 3:31:40 AM Zheng Hacker <[email protected]> wrote:
> Yes, that makes this issue hard to fix. I was wondering why it binds the
> worker with the timer rather than using just one of them.
No top posting please!
The timer context is softirq and worker is thread context. The ability to
sleep is the big difference between the two or at least the reason for
using them here.
Regards,
Arend
>
> Takashi Iwai <[email protected]> 于2023年11月17日周五 02:25写道：
>>
>> On Thu, 16 Nov 2023 19:20:06 +0100,
>> Arend Van Spriel wrote:
>>>
>>> On November 15, 2023 4:00:46 PM Zheng Hacker <[email protected]> wrote:
>>>
>>>> Arend van Spriel <[email protected]> 于2023年11月13日周一 17:18写道：
>>>>>
>>>>> On November 8, 2023 4:03:26 AM Zheng Hacker <[email protected]>
>>>>> wrote:
>>>>>
>>>>>> Arend Van Spriel <[email protected]> 于2023年11月6日周一 23:48写道：
>>>>>>>
>>>>>>> On November 6, 2023 3:44:53 PM Zheng Hacker <[email protected]> wrote:
>>>>>>>
>>>>>>>> Thanks! I didn't test it for I don't have a device. Very appreciated
>>>>>>>> if anyone could help with that.
>>>>>>>
>>>>>>> I would volunteer, but it made me dig deep and not sure if there is a
>>>>>>> problem to solve here.
>>>>>>>
>>>>>>> brcmf_cfg80211_detach() calls wl_deinit_priv() -> brcmf_abort_scanning() ->
>>>>>>> brcmf_notify_escan_complete() which does delete the timer.
>>>>>>>
>>>>>>> What am I missing here?
>>>>>>
>>>>>> Thanks four your detailed review. I did see the code and not sure if
>>>>>> brcmf_notify_escan_complete
>>>>>> would be triggered for sure. So in the first version I want to delete
>>>>>> the pending timer ahead of time.
>>>>>
>>>>> Why requesting a CVE when you are not sure? Seems a bit hasty to put it
>>>>> mildly.
>>>>
>>>> I'm sure the issue exists because there's only cancler of timer but not woker.
>>>> As there's similar CVEs before like : https://github.com/V4bel/CVE-2022-41218,
>>>> I submit it as soon as I found it.
>>>
>>> Ah, yes. The cancel_work_sync() can also be done in
>>> brcmf_notify_escan_complete().
>>
>> AFAIUC, brcmf_notify_scan_complete() is called from the work itself,
>> too, hence you can't issue cancel_work_sync() there (unless you make
>> it conditional).
>>
>>
>> Takashi