netdev - Re: [PATCH 1/1] wifi: ath9k: hif_usb: fix memory leak of remain

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5d67552f-88dd-7bbe-ebeb-888d1efad985@ispras.ru>
Date:   Thu, 16 Feb 2023 20:50:51 +0300
From:   Fedor Pchelkin <pchelkin@...ras.ru>
To:     Toke Høiland-Jørgensen <toke@...e.dk>
Cc:     Kalle Valo <kvalo@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        "John W. Linville" <linville@...driver.com>,
        Vasanthakumar Thiagarajan <vasanth@...eros.com>,
        Senthil Balasubramanian <senthilkumar@...eros.com>,
        Sujith <Sujith.Manoharan@...eros.com>,
        linux-wireless@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Alexey Khoroshilov <khoroshilov@...ras.ru>,
        lvc-project@...uxtesting.org
Subject: Re: [PATCH 1/1] wifi: ath9k: hif_usb: fix memory leak of remain_skbs

On 16.02.2023 19:15, Toke Høiland-Jørgensen wrote:
 > Erm, does this actually fix the leak? AFAICT, ath9k_hif_usb_dev_deinit()
 > is only called on the error path of ath9k_hif_usb_firmware_cb(), not
 > when the device is subsequently torn down in
 > ath9k_htc_disconnect_device()?

ath9k_hif_usb_dev_deinit() is also called inside
ath9k_hif_usb_disconnect(). I see it to be the only place wherehif_dev is
freed (apart from an early error path), so the current patchimplementation
actually fixes the leak. However, as you have noticed, itis not probably
the best place to put the deallocation: we need to clearthe cached skb
not only when freeing the device but in urbs deallocationcase, too - in
order to avoid its irrelevant processing later.

 > I think the right place to put this is probably inside
 > ath9k_hif_usb_dealloc_urbs()? That gets called on USB suspend as well,
 > but it seems to me that if we're suspending the device to an extent that
 > we're deallocating the urbs, we should be clearing out the cached skb in
 > remain_skb anyway?
 >
 > -Toke

Thank you for the advice! As I can see, remain_skb makes sense when
receiving two consecutive urbs which are logically linked together, i.e.
a specific data field from the first skb indicates a cached skb to be
allocated, memcpy'd with some data and subsequently processed in the
next call to rx callback (see 6ce708f54cc8 ("ath9k: Fix out-of-bound
memcpy in ath9k_hif_usb_rx_stream")). Urbs deallocation, I suppose,
makes that link irrelevant.

So I agree with you that remain_skb freeing should be done when
deallocating the urbs. I would just place that specifically into
ath9k_hif_usb_dealloc_rx_urbs() as remain_skb is associated with rx
urbs.

RX_STAT_INC(hif_dev, skb_dropped), I think, should be also called when
freeing afilled remain_skb?

---
Regards,

Fedor