| Message ID | d57bbedc857950659bfacac0ab48790c1eda00c8.1655145743.git.paskripkin@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0ac4827f78c7ffe8eef074bc010e7e34bc22f533 |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v6,1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb | expand |
Pavel Skripkin <paskripkin@gmail.com> 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: syzbot+03110230a11411024147@syzkaller.appspotmail.com > Reported-and-tested-by: syzbot+c6dde1f690b60e0b9fbe@syzkaller.appspotmail.com > Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> 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 <toke@toke.dk>
Toke Høiland-Jørgensen <toke@toke.dk> writes: > Pavel Skripkin <paskripkin@gmail.com> 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: syzbot+03110230a11411024147@syzkaller.appspotmail.com >> Reported-and-tested-by: syzbot+c6dde1f690b60e0b9fbe@syzkaller.appspotmail.com >> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> > > 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 <toke@toke.dk> 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.
Kalle Valo <kvalo@kernel.org> writes: > Toke Høiland-Jørgensen <toke@toke.dk> writes: > >> Pavel Skripkin <paskripkin@gmail.com> 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: syzbot+03110230a11411024147@syzkaller.appspotmail.com >>> Reported-and-tested-by: syzbot+c6dde1f690b60e0b9fbe@syzkaller.appspotmail.com >>> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> >> >> 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 <toke@toke.dk> > > 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 <kvalo@kernel.org> writes: > > > Toke Høiland-Jørgensen <toke@toke.dk> writes: > > > >> Pavel Skripkin <paskripkin@gmail.com> 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: syzbot+03110230a11411024147@syzkaller.appspotmail.com > >>> Reported-and-tested-by: syzbot+c6dde1f690b60e0b9fbe@syzkaller.appspotmail.com > >>> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> > >> > >> 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 <toke@toke.dk> > > > > 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 <tiwai@suse.de> writes: > On Wed, 15 Jun 2022 11:05:20 +0200, > Toke Høiland-Jørgensen wrote: > >> >> Kalle Valo <kvalo@kernel.org> writes: >> >> > Toke Høiland-Jørgensen <toke@toke.dk> writes: >> > >> >> Pavel Skripkin <paskripkin@gmail.com> 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: syzbot+03110230a11411024147@syzkaller.appspotmail.com >> >>> Reported-and-tested-by: syzbot+c6dde1f690b60e0b9fbe@syzkaller.appspotmail.com >> >>> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> >> >> >> >> 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 <toke@toke.dk> >> > >> > 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).
Pavel Skripkin <paskripkin@gmail.com> wrote: > 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: syzbot+03110230a11411024147@syzkaller.appspotmail.com > Reported-and-tested-by: syzbot+c6dde1f690b60e0b9fbe@syzkaller.appspotmail.com > Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> > Acked-by: Toke Høiland-Jørgensen <toke@toke.dk> > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> 2 patches applied to ath-next branch of ath.git, thanks. 0ac4827f78c7 ath9k: fix use-after-free in ath9k_hif_usb_rx_cb d7fc76039b74 ath9k: htc: clean up statistics macros
diff --git a/drivers/net/wireless/ath/ath9k/htc.h b/drivers/net/wireless/ath/ath9k/htc.h index 6b45e63fa..e3d546ef7 100644 --- a/drivers/net/wireless/ath/ath9k/htc.h +++ b/drivers/net/wireless/ath/ath9k/htc.h @@ -327,11 +327,11 @@ static inline struct ath9k_htc_tx_ctl *HTC_SKB_CB(struct sk_buff *skb) } #ifdef CONFIG_ATH9K_HTC_DEBUGFS - -#define TX_STAT_INC(c) (hif_dev->htc_handle->drv_priv->debug.tx_stats.c++) -#define TX_STAT_ADD(c, a) (hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a) -#define RX_STAT_INC(c) (hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++) -#define RX_STAT_ADD(c, a) (hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c += a) +#define __STAT_SAFE(expr) (hif_dev->htc_handle->drv_priv ? (expr) : 0) +#define TX_STAT_INC(c) __STAT_SAFE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c++) +#define TX_STAT_ADD(c, a) __STAT_SAFE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a) +#define RX_STAT_INC(c) __STAT_SAFE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++) +#define RX_STAT_ADD(c, a) __STAT_SAFE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c += a) #define CAB_STAT_INC priv->debug.tx_stats.cab_queued++ #define TX_QSTAT_INC(q) (priv->debug.tx_stats.queue_stats[q]++) diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_init.c b/drivers/net/wireless/ath/ath9k/htc_drv_init.c index ff61ae34e..07ac88fb1 100644 --- a/drivers/net/wireless/ath/ath9k/htc_drv_init.c +++ b/drivers/net/wireless/ath/ath9k/htc_drv_init.c @@ -944,7 +944,6 @@ int ath9k_htc_probe_device(struct htc_target *htc_handle, struct device *dev, priv->hw = hw; priv->htc = htc_handle; priv->dev = dev; - htc_handle->drv_priv = priv; SET_IEEE80211_DEV(hw, priv->dev); ret = ath9k_htc_wait_for_target(priv); @@ -965,6 +964,8 @@ int ath9k_htc_probe_device(struct htc_target *htc_handle, struct device *dev, if (ret) goto err_init; + htc_handle->drv_priv = priv; + return 0; err_init:
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: syzbot+03110230a11411024147@syzkaller.appspotmail.com Reported-and-tested-by: syzbot+c6dde1f690b60e0b9fbe@syzkaller.appspotmail.com Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> --- Changes since v5: - No changes Changes since v4: s/save/safe/ in commit message Changes since v3: - s/SAVE/SAFE/ - Added links to syzkaller reports Changes since v2: - My send-email script forgot, that mailing lists exist. Added back all related lists Changes since v1: - Removed clean-ups and moved them to 2/2 --- drivers/net/wireless/ath/ath9k/htc.h | 10 +++++----- drivers/net/wireless/ath/ath9k/htc_drv_init.c | 3 ++- 2 files changed, 7 insertions(+), 6 deletions(-)