Re: [PATCH] ath9k: fix use-after-free read at ath9k_hif_usb_rx_cb()

[Pavel Skripkin <paskripkin@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2378 bytes --]

Hi Tetsuo,

On 4/29/22 14:18, Tetsuo Handa wrote:
> Although hif_dev->htc_handle is allocated by ath9k_htc_hw_alloc() from
> ath9k_hif_usb_firmware_cb(), hif_dev->htc_handle->drv_priv is not assigned
> until ieee80211_alloc_hw() from ath9k_htc_probe_device() from
> ath9k_htc_hw_init() from ath9k_hif_usb_firmware_cb() returns. However, as
> soon as ath9k_hif_usb_alloc_rx_urbs() from ath9k_hif_usb_alloc_urbs() from
> ath9k_hif_usb_dev_init() from ath9k_hif_usb_firmware_cb() returns, a timer
> interrupt can access hif_dev->htc_handle->drv_priv via RX_STAT_INC() from
> ath9k_hif_usb_rx_stream() from ath9k_hif_usb_rx_cb() from
> usb_hcd_giveback_urb(), which results in NULL pointer deference problem.
> 
> Also, even after htc_handle->drv_priv is assigned, when
> ath9k_htc_wait_for_target() from ath9k_htc_probe_device() from
> ath9k_htc_hw_init() from ath9k_hif_usb_firmware_cb() fails,
> ieee80211_free_hw() (which does not reset hif_dev->htc_handle->drv_priv)
> is immediately called due to "goto err_free;". As a result, a timer
> interrupt which happens after ieee80211_free_hw() triggers use-after-free
> problem at the abovementioned location.
> 
> We can flush in-flight requests by calling ath9k_hif_usb_dealloc_urbs()
> before calling ieee80211_free_hw(). But we need to take from two choices
> for not yet assigned case. One is to change e.g. RX_STAT_INC() to check
> for NULL because it depends on CONFIG_ATH9K_HTC_DEBUGFS=y. The other is to
> assign a dummy object which is used until initialization. This patch took
> the latter.
> 
> Link: https://syzkaller.appspot.com/bug?extid=03110230a11411024147
> Reported-by: syzbot <syzbot+03110230a11411024147@syzkaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Tested-by: syzbot <syzbot+03110230a11411024147@syzkaller.appspotmail.com>
> ---
> Pavel Skripkin has tested "check for NULL" approach, but not yet accepted.
> What was wrong with Pavel's approach?
> 

I don't know. IIRC the problem is that nobody has tested my patch on 
real hw, so they can't accept it as-is. And maybe it just got lost

You can check out [1] thread. It's the latest version I have posted



[1] 
https://lore.kernel.org/all/80962aae265995d1cdb724f5362c556d494c7566.1644265120.git.paskripkin@gmail.com/



With regards,
Pavel Skripkin




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]