Pavel Skripkin <[email protected]> writes:
> Syzbot reported use-after-free Read in ath9k_hif_usb_rx_cb() [0]. The
> problem was in incorrect htc_handle->drv_priv initialization.
>
> Probable call trace which can trigger use-after-free:
>
> ath9k_htc_probe_device()
> /* htc_handle->drv_priv = priv; */
> ath9k_htc_wait_for_target() <--- Failed
> ieee80211_free_hw() <--- priv pointer is freed
>
> <IRQ>
> ...
> ath9k_hif_usb_rx_cb()
> ath9k_hif_usb_rx_stream()
> RX_STAT_INC() <--- htc_handle->drv_priv access
>
> In order to not add fancy protection for drv_priv we can move
> htc_handle->drv_priv initialization at the end of the
> ath9k_htc_probe_device() and add helper macro to make
> all *_STAT_* macros NULL safe, since syzbot has reported related NULL
> deref in that macros [1]
>
> Link: https://syzkaller.appspot.com/bug?id=6ead44e37afb6866ac0c7dd121b4ce07cb665f60 [0]
> Link: https://syzkaller.appspot.com/bug?id=b8101ffcec107c0567a0cd8acbbacec91e9ee8de [1]
> Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
> Reported-and-tested-by: [email protected]
> Reported-and-tested-by: [email protected]
> Signed-off-by: Pavel Skripkin <[email protected]>
Alright, since we've heard no more objections and the status quo is
definitely broken, let's get this merged and we can follow up with any
other fixes as necessary...
Acked-by: Toke Høiland-Jørgensen <[email protected]>
Toke Høiland-Jørgensen <[email protected]> writes:
> Pavel Skripkin <[email protected]> writes:
>
>> Syzbot reported use-after-free Read in ath9k_hif_usb_rx_cb() [0]. The
>> problem was in incorrect htc_handle->drv_priv initialization.
>>
>> Probable call trace which can trigger use-after-free:
>>
>> ath9k_htc_probe_device()
>> /* htc_handle->drv_priv = priv; */
>> ath9k_htc_wait_for_target() <--- Failed
>> ieee80211_free_hw() <--- priv pointer is freed
>>
>> <IRQ>
>> ...
>> ath9k_hif_usb_rx_cb()
>> ath9k_hif_usb_rx_stream()
>> RX_STAT_INC() <--- htc_handle->drv_priv access
>>
>> In order to not add fancy protection for drv_priv we can move
>> htc_handle->drv_priv initialization at the end of the
>> ath9k_htc_probe_device() and add helper macro to make
>> all *_STAT_* macros NULL safe, since syzbot has reported related NULL
>> deref in that macros [1]
>>
>> Link: https://syzkaller.appspot.com/bug?id=6ead44e37afb6866ac0c7dd121b4ce07cb665f60 [0]
>> Link: https://syzkaller.appspot.com/bug?id=b8101ffcec107c0567a0cd8acbbacec91e9ee8de [1]
>> Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
>> Reported-and-tested-by: [email protected]
>> Reported-and-tested-by: [email protected]
>> Signed-off-by: Pavel Skripkin <[email protected]>
>
> Alright, since we've heard no more objections and the status quo is
> definitely broken, let's get this merged and we can follow up with any
> other fixes as necessary...
>
> Acked-by: Toke Høiland-Jørgensen <[email protected]>
I'm wondering should these go to -rc or -next? Has anyone actually
tested these with real hardware? (syzbot testing does not count) With
the past bad experience with syzbot fixes I'm leaning towards -next to
have more time to fix any regressions.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Kalle Valo <[email protected]> writes:
> Toke Høiland-Jørgensen <[email protected]> writes:
>
>> Pavel Skripkin <[email protected]> writes:
>>
>>> Syzbot reported use-after-free Read in ath9k_hif_usb_rx_cb() [0]. The
>>> problem was in incorrect htc_handle->drv_priv initialization.
>>>
>>> Probable call trace which can trigger use-after-free:
>>>
>>> ath9k_htc_probe_device()
>>> /* htc_handle->drv_priv = priv; */
>>> ath9k_htc_wait_for_target() <--- Failed
>>> ieee80211_free_hw() <--- priv pointer is freed
>>>
>>> <IRQ>
>>> ...
>>> ath9k_hif_usb_rx_cb()
>>> ath9k_hif_usb_rx_stream()
>>> RX_STAT_INC() <--- htc_handle->drv_priv access
>>>
>>> In order to not add fancy protection for drv_priv we can move
>>> htc_handle->drv_priv initialization at the end of the
>>> ath9k_htc_probe_device() and add helper macro to make
>>> all *_STAT_* macros NULL safe, since syzbot has reported related NULL
>>> deref in that macros [1]
>>>
>>> Link: https://syzkaller.appspot.com/bug?id=6ead44e37afb6866ac0c7dd121b4ce07cb665f60 [0]
>>> Link: https://syzkaller.appspot.com/bug?id=b8101ffcec107c0567a0cd8acbbacec91e9ee8de [1]
>>> Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
>>> Reported-and-tested-by: [email protected]
>>> Reported-and-tested-by: [email protected]
>>> Signed-off-by: Pavel Skripkin <[email protected]>
>>
>> Alright, since we've heard no more objections and the status quo is
>> definitely broken, let's get this merged and we can follow up with any
>> other fixes as necessary...
>>
>> Acked-by: Toke Høiland-Jørgensen <[email protected]>
>
> I'm wondering should these go to -rc or -next? Has anyone actually
> tested these with real hardware? (syzbot testing does not count) With
> the past bad experience with syzbot fixes I'm leaning towards -next to
> have more time to fix any regressions.
Hmm, good question. From Takashi's comment on v5, it seems like distros
are going to backport it anyway, so in that sense it probably doesn't
matter that much?
In any case I think it has a fairly low probability of breaking real
users' setup (how often is that error path on setup even hit?), but I'm
OK with it going to -next to be doubleplus-sure :)
-Toke
On Wed, 15 Jun 2022 11:05:20 +0200,
Toke H?iland-J?rgensen wrote:
>
> Kalle Valo <[email protected]> writes:
>
> > Toke H?iland-J?rgensen <[email protected]> writes:
> >
> >> Pavel Skripkin <[email protected]> writes:
> >>
> >>> Syzbot reported use-after-free Read in ath9k_hif_usb_rx_cb() [0]. The
> >>> problem was in incorrect htc_handle->drv_priv initialization.
> >>>
> >>> Probable call trace which can trigger use-after-free:
> >>>
> >>> ath9k_htc_probe_device()
> >>> /* htc_handle->drv_priv = priv; */
> >>> ath9k_htc_wait_for_target() <--- Failed
> >>> ieee80211_free_hw() <--- priv pointer is freed
> >>>
> >>> <IRQ>
> >>> ...
> >>> ath9k_hif_usb_rx_cb()
> >>> ath9k_hif_usb_rx_stream()
> >>> RX_STAT_INC() <--- htc_handle->drv_priv access
> >>>
> >>> In order to not add fancy protection for drv_priv we can move
> >>> htc_handle->drv_priv initialization at the end of the
> >>> ath9k_htc_probe_device() and add helper macro to make
> >>> all *_STAT_* macros NULL safe, since syzbot has reported related NULL
> >>> deref in that macros [1]
> >>>
> >>> Link: https://syzkaller.appspot.com/bug?id=6ead44e37afb6866ac0c7dd121b4ce07cb665f60 [0]
> >>> Link: https://syzkaller.appspot.com/bug?id=b8101ffcec107c0567a0cd8acbbacec91e9ee8de [1]
> >>> Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
> >>> Reported-and-tested-by: [email protected]
> >>> Reported-and-tested-by: [email protected]
> >>> Signed-off-by: Pavel Skripkin <[email protected]>
> >>
> >> Alright, since we've heard no more objections and the status quo is
> >> definitely broken, let's get this merged and we can follow up with any
> >> other fixes as necessary...
> >>
> >> Acked-by: Toke H?iland-J?rgensen <[email protected]>
> >
> > I'm wondering should these go to -rc or -next? Has anyone actually
> > tested these with real hardware? (syzbot testing does not count) With
> > the past bad experience with syzbot fixes I'm leaning towards -next to
> > have more time to fix any regressions.
>
> Hmm, good question. From Takashi's comment on v5, it seems like distros
> are going to backport it anyway, so in that sense it probably doesn't
> matter that much?
Well, it does matter if it really breaks things, of course ;)
> In any case I think it has a fairly low probability of breaking real
> users' setup (how often is that error path on setup even hit?), but I'm
> OK with it going to -next to be doubleplus-sure :)
Queuing to for-next is fine for us. Backporting immediately or not
will be a decision by each distro, then.
OTOH, if anyone has tested it beforehand on a real hardware and
confirmed, at least, that it works for normal cases (no error path),
that should suffice for -rc inclusion, too, IMO.
thanks,
Takashi
Takashi Iwai <[email protected]> writes:
> On Wed, 15 Jun 2022 11:05:20 +0200,
> Toke Høiland-Jørgensen wrote:
>
>>
>> Kalle Valo <[email protected]> writes:
>>
>> > Toke Høiland-Jørgensen <[email protected]> writes:
>> >
>> >> Pavel Skripkin <[email protected]> writes:
>> >>
>> >>> Syzbot reported use-after-free Read in ath9k_hif_usb_rx_cb() [0]. The
>> >>> problem was in incorrect htc_handle->drv_priv initialization.
>> >>>
>> >>> Probable call trace which can trigger use-after-free:
>> >>>
>> >>> ath9k_htc_probe_device()
>> >>> /* htc_handle->drv_priv = priv; */
>> >>> ath9k_htc_wait_for_target() <--- Failed
>> >>> ieee80211_free_hw() <--- priv pointer is freed
>> >>>
>> >>> <IRQ>
>> >>> ...
>> >>> ath9k_hif_usb_rx_cb()
>> >>> ath9k_hif_usb_rx_stream()
>> >>> RX_STAT_INC() <--- htc_handle->drv_priv access
>> >>>
>> >>> In order to not add fancy protection for drv_priv we can move
>> >>> htc_handle->drv_priv initialization at the end of the
>> >>> ath9k_htc_probe_device() and add helper macro to make
>> >>> all *_STAT_* macros NULL safe, since syzbot has reported related NULL
>> >>> deref in that macros [1]
>> >>>
>> >>> Link: https://syzkaller.appspot.com/bug?id=6ead44e37afb6866ac0c7dd121b4ce07cb665f60 [0]
>> >>> Link: https://syzkaller.appspot.com/bug?id=b8101ffcec107c0567a0cd8acbbacec91e9ee8de [1]
>> >>> Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
>> >>> Reported-and-tested-by: [email protected]
>> >>> Reported-and-tested-by: [email protected]
>> >>> Signed-off-by: Pavel Skripkin <[email protected]>
>> >>
>> >> Alright, since we've heard no more objections and the status quo is
>> >> definitely broken, let's get this merged and we can follow up with any
>> >> other fixes as necessary...
>> >>
>> >> Acked-by: Toke Høiland-Jørgensen <[email protected]>
>> >
>> > I'm wondering should these go to -rc or -next? Has anyone actually
>> > tested these with real hardware? (syzbot testing does not count) With
>> > the past bad experience with syzbot fixes I'm leaning towards -next to
>> > have more time to fix any regressions.
>>
>> Hmm, good question. From Takashi's comment on v5, it seems like distros
>> are going to backport it anyway, so in that sense it probably doesn't
>> matter that much?
>
> Well, it does matter if it really breaks things, of course ;)
>
>> In any case I think it has a fairly low probability of breaking real
>> users' setup (how often is that error path on setup even hit?), but I'm
>> OK with it going to -next to be doubleplus-sure :)
>
> Queuing to for-next is fine for us. Backporting immediately or not
> will be a decision by each distro, then.
>
> OTOH, if anyone has tested it beforehand on a real hardware and
> confirmed, at least, that it works for normal cases (no error path),
> that should suffice for -rc inclusion, too, IMO.
Ok, I'll take these to -next then. I just don't like taking untested
patches, having them -next gives us more time to fix any issues (or
revert the patches).
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches