* [PATCH 0/1] wifi: ath9k: hif_usb: fix memory leak of remain_skbs @ 2023-02-12 14:52 Fedor Pchelkin 2023-02-12 14:52 ` [PATCH 1/1] " Fedor Pchelkin 0 siblings, 1 reply; 9+ messages in thread From: Fedor Pchelkin @ 2023-02-12 14:52 UTC (permalink / raw) To: Toke Høiland-Jørgensen Cc: Fedor Pchelkin, Kalle Valo, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, John W. Linville, Vasanthakumar Thiagarajan, Senthil Balasubramanian, Sujith, linux-wireless, netdev, linux-kernel, Alexey Khoroshilov, lvc-project Hello, the problem is described in the following patch. Some remarks: I'm not quite sure if the function proposed in the patch should be placed somewhere else in the code... The patch now calls it during device deinitialization after urbs deallocation, and I suppose the interrupt callback ath9k_hif_usb_rx_stream() not to be executed during/after that period (in this case the leak would still remain, but I couldn't reproduce this particular case). Maybe the function freeing remain_skbs should be placed right before calling 'kfree(hif_dev)'? The patch is made on top of [1] master branch. [1] https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/ --- Regards, Fedor ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/1] wifi: ath9k: hif_usb: fix memory leak of remain_skbs 2023-02-12 14:52 [PATCH 0/1] wifi: ath9k: hif_usb: fix memory leak of remain_skbs Fedor Pchelkin @ 2023-02-12 14:52 ` Fedor Pchelkin 2023-02-16 16:15 ` Toke Høiland-Jørgensen 0 siblings, 1 reply; 9+ messages in thread From: Fedor Pchelkin @ 2023-02-12 14:52 UTC (permalink / raw) To: Toke Høiland-Jørgensen Cc: Fedor Pchelkin, Kalle Valo, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, John W. Linville, Vasanthakumar Thiagarajan, Senthil Balasubramanian, Sujith, linux-wireless, netdev, linux-kernel, Alexey Khoroshilov, lvc-project hif_dev->remain_skb is allocated and used exclusively in ath9k_hif_usb_rx_stream(). It is implied that an allocated remain_skb is processed and subsequently freed (in error paths) only during the next call of ath9k_hif_usb_rx_stream(). So, if the device is deinitialized between those two calls or if the skb contents are incorrect, it is possible that ath9k_hif_usb_rx_stream() is not called next time and the allocated remain_skb is leaked. Our local Syzkaller instance was able to trigger that. Fix the leak by introducing a function to explicitly free remain_skb (if it is not NULL) when the device is being deinitialized. remain_skb is NULL when it has not been allocated at all (hif_dev struct is kzalloced) or when it has been proccesed in next call to ath9k_hif_usb_rx_stream(). Proper spinlocks are held to prevent possible concurrent access to remain_skb from the interrupt context ath9k_hif_usb_rx_stream(). These accesses should not happen as rx_urbs have been deallocated before but it prevents a dangerous race condition in these cases. Found by Linux Verification Center (linuxtesting.org) with Syzkaller. Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.") Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> --- drivers/net/wireless/ath/ath9k/hif_usb.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c index f521dfa2f194..e03ab972edf7 100644 --- a/drivers/net/wireless/ath/ath9k/hif_usb.c +++ b/drivers/net/wireless/ath/ath9k/hif_usb.c @@ -534,6 +534,23 @@ static struct ath9k_htc_hif hif_usb = { .send = hif_usb_send, }; +/* Need to free remain_skb allocated in ath9k_hif_usb_rx_stream + * in case ath9k_hif_usb_rx_stream wasn't called next time to + * process the buffer and subsequently free it. + */ +static void ath9k_hif_usb_free_rx_remain_skb(struct hif_device_usb *hif_dev) +{ + unsigned long flags; + + spin_lock_irqsave(&hif_dev->rx_lock, flags); + if (hif_dev->remain_skb) { + dev_kfree_skb_any(hif_dev->remain_skb); + hif_dev->remain_skb = NULL; + hif_dev->rx_remain_len = 0; + } + spin_unlock_irqrestore(&hif_dev->rx_lock, flags); +} + static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev, struct sk_buff *skb) { @@ -1129,6 +1146,7 @@ static int ath9k_hif_usb_dev_init(struct hif_device_usb *hif_dev) static void ath9k_hif_usb_dev_deinit(struct hif_device_usb *hif_dev) { ath9k_hif_usb_dealloc_urbs(hif_dev); + ath9k_hif_usb_free_rx_remain_skb(hif_dev); } /* -- 2.34.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] wifi: ath9k: hif_usb: fix memory leak of remain_skbs 2023-02-12 14:52 ` [PATCH 1/1] " Fedor Pchelkin @ 2023-02-16 16:15 ` Toke Høiland-Jørgensen 2023-02-16 17:50 ` Fedor Pchelkin 0 siblings, 1 reply; 9+ messages in thread From: Toke Høiland-Jørgensen @ 2023-02-16 16:15 UTC (permalink / raw) To: Fedor Pchelkin Cc: Fedor Pchelkin, Kalle Valo, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, John W. Linville, Vasanthakumar Thiagarajan, Senthil Balasubramanian, Sujith, linux-wireless, netdev, linux-kernel, Alexey Khoroshilov, lvc-project Fedor Pchelkin <pchelkin@ispras.ru> writes: > hif_dev->remain_skb is allocated and used exclusively in > ath9k_hif_usb_rx_stream(). It is implied that an allocated remain_skb is > processed and subsequently freed (in error paths) only during the next > call of ath9k_hif_usb_rx_stream(). > > So, if the device is deinitialized between those two calls or if the skb > contents are incorrect, it is possible that ath9k_hif_usb_rx_stream() is > not called next time and the allocated remain_skb is leaked. Our local > Syzkaller instance was able to trigger that. > > Fix the leak by introducing a function to explicitly free remain_skb (if > it is not NULL) when the device is being deinitialized. remain_skb is NULL > when it has not been allocated at all (hif_dev struct is kzalloced) or > when it has been proccesed in next call to ath9k_hif_usb_rx_stream(). > > Proper spinlocks are held to prevent possible concurrent access to > remain_skb from the interrupt context ath9k_hif_usb_rx_stream(). These > accesses should not happen as rx_urbs have been deallocated before but > it prevents a dangerous race condition in these cases. > > Found by Linux Verification Center (linuxtesting.org) with Syzkaller. > > Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.") > Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru> > Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> > --- > drivers/net/wireless/ath/ath9k/hif_usb.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c > index f521dfa2f194..e03ab972edf7 100644 > --- a/drivers/net/wireless/ath/ath9k/hif_usb.c > +++ b/drivers/net/wireless/ath/ath9k/hif_usb.c > @@ -534,6 +534,23 @@ static struct ath9k_htc_hif hif_usb = { > .send = hif_usb_send, > }; > > +/* Need to free remain_skb allocated in ath9k_hif_usb_rx_stream > + * in case ath9k_hif_usb_rx_stream wasn't called next time to > + * process the buffer and subsequently free it. > + */ > +static void ath9k_hif_usb_free_rx_remain_skb(struct hif_device_usb *hif_dev) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&hif_dev->rx_lock, flags); > + if (hif_dev->remain_skb) { > + dev_kfree_skb_any(hif_dev->remain_skb); > + hif_dev->remain_skb = NULL; > + hif_dev->rx_remain_len = 0; > + } > + spin_unlock_irqrestore(&hif_dev->rx_lock, flags); > +} > + > static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev, > struct sk_buff *skb) > { > @@ -1129,6 +1146,7 @@ static int ath9k_hif_usb_dev_init(struct hif_device_usb *hif_dev) > static void ath9k_hif_usb_dev_deinit(struct hif_device_usb *hif_dev) > { > ath9k_hif_usb_dealloc_urbs(hif_dev); > + ath9k_hif_usb_free_rx_remain_skb(hif_dev); > } 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()? 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] wifi: ath9k: hif_usb: fix memory leak of remain_skbs 2023-02-16 16:15 ` Toke Høiland-Jørgensen @ 2023-02-16 17:50 ` Fedor Pchelkin 2023-02-16 18:05 ` Toke Høiland-Jørgensen 0 siblings, 1 reply; 9+ messages in thread From: Fedor Pchelkin @ 2023-02-16 17:50 UTC (permalink / raw) To: Toke Høiland-Jørgensen Cc: Kalle Valo, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, John W. Linville, Vasanthakumar Thiagarajan, Senthil Balasubramanian, Sujith, linux-wireless, netdev, linux-kernel, Alexey Khoroshilov, lvc-project 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] wifi: ath9k: hif_usb: fix memory leak of remain_skbs 2023-02-16 17:50 ` Fedor Pchelkin @ 2023-02-16 18:05 ` Toke Høiland-Jørgensen 2023-02-16 18:44 ` Fedor Pchelkin 2023-02-16 19:23 ` [PATCH v2] " Fedor Pchelkin 0 siblings, 2 replies; 9+ messages in thread From: Toke Høiland-Jørgensen @ 2023-02-16 18:05 UTC (permalink / raw) To: Fedor Pchelkin Cc: Kalle Valo, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, John W. Linville, Vasanthakumar Thiagarajan, Senthil Balasubramanian, Sujith, linux-wireless, netdev, linux-kernel, Alexey Khoroshilov, lvc-project Fedor Pchelkin <pchelkin@ispras.ru> writes: > 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(). No it's not, as of: f099c5c9e2ba ("wifi: ath9k: Fix use-after-free in ath9k_hif_usb_disconnect()") I guess you're looking at an older tree? Please base your patches on an up-to-date ath-next tree. > 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. SGTM. > RX_STAT_INC(hif_dev, skb_dropped), I think, should be also called when > freeing afilled remain_skb? Well, if this is mostly something that happens if the device is going away I'm not sure that anyone will actually see that; but I suppose if it happens on suspend, the stat increase may be useful, and it shouldn't hurt otherwise, so sure, let's add that :) -Toke ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] wifi: ath9k: hif_usb: fix memory leak of remain_skbs 2023-02-16 18:05 ` Toke Høiland-Jørgensen @ 2023-02-16 18:44 ` Fedor Pchelkin 2023-02-16 19:23 ` [PATCH v2] " Fedor Pchelkin 1 sibling, 0 replies; 9+ messages in thread From: Fedor Pchelkin @ 2023-02-16 18:44 UTC (permalink / raw) To: Toke Høiland-Jørgensen Cc: Kalle Valo, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, John W. Linville, Vasanthakumar Thiagarajan, Senthil Balasubramanian, Sujith, linux-wireless, netdev, linux-kernel, Alexey Khoroshilov, lvc-project On 16.02.2023 21:05, Toke Høiland-Jørgensen wrote: > Fedor Pchelkin <pchelkin@ispras.ru> writes: > >> 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(). > No it's not, as of: > > f099c5c9e2ba ("wifi: ath9k: Fix use-after-free in ath9k_hif_usb_disconnect()") > > I guess you're looking at an older tree? Please base your patches on an > up-to-date ath-next tree. > Oops, that's my fault, I indeed patched the wrong tree. Thanks for clarifying! ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] wifi: ath9k: hif_usb: fix memory leak of remain_skbs 2023-02-16 18:05 ` Toke Høiland-Jørgensen 2023-02-16 18:44 ` Fedor Pchelkin @ 2023-02-16 19:23 ` Fedor Pchelkin 2023-02-16 20:54 ` Toke Høiland-Jørgensen 2023-02-20 8:37 ` Kalle Valo 1 sibling, 2 replies; 9+ messages in thread From: Fedor Pchelkin @ 2023-02-16 19:23 UTC (permalink / raw) To: Toke Høiland-Jørgensen Cc: Fedor Pchelkin, Kalle Valo, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, John W. Linville, Vasanthakumar Thiagarajan, Senthil Balasubramanian, Sujith, linux-wireless, netdev, linux-kernel, Alexey Khoroshilov, lvc-project hif_dev->remain_skb is allocated and used exclusively in ath9k_hif_usb_rx_stream(). It is implied that an allocated remain_skb is processed and subsequently freed (in error paths) only during the next call of ath9k_hif_usb_rx_stream(). So, if the urbs are deallocated between those two calls due to the device deinitialization or suspend, it is possible that ath9k_hif_usb_rx_stream() is not called next time and the allocated remain_skb is leaked. Our local Syzkaller instance was able to trigger that. 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 ath9k_hif_usb_rx_stream(). Urbs deallocation supposedly makes that link irrelevant so we need to free the cached skb in those cases. Fix the leak by introducing a function to explicitly free remain_skb (if it is not NULL) when the rx urbs have been deallocated. remain_skb is NULL when it has not been allocated at all (hif_dev struct is kzalloced) or when it has been processed in next call to ath9k_hif_usb_rx_stream(). Found by Linux Verification Center (linuxtesting.org) with Syzkaller. Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.") Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> --- v1->v2: move ath9k_hif_usb_free_rx_remain_skb call into urbs dealloc function as advised by Toke; add a stat macro drivers/net/wireless/ath/ath9k/hif_usb.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c index f521dfa2f194..e0130beb304d 100644 --- a/drivers/net/wireless/ath/ath9k/hif_usb.c +++ b/drivers/net/wireless/ath/ath9k/hif_usb.c @@ -534,6 +534,24 @@ static struct ath9k_htc_hif hif_usb = { .send = hif_usb_send, }; +/* Need to free remain_skb allocated in ath9k_hif_usb_rx_stream + * in case ath9k_hif_usb_rx_stream wasn't called next time to + * process the buffer and subsequently free it. + */ +static void ath9k_hif_usb_free_rx_remain_skb(struct hif_device_usb *hif_dev) +{ + unsigned long flags; + + spin_lock_irqsave(&hif_dev->rx_lock, flags); + if (hif_dev->remain_skb) { + dev_kfree_skb_any(hif_dev->remain_skb); + hif_dev->remain_skb = NULL; + hif_dev->rx_remain_len = 0; + RX_STAT_INC(hif_dev, skb_dropped); + } + spin_unlock_irqrestore(&hif_dev->rx_lock, flags); +} + static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev, struct sk_buff *skb) { @@ -868,6 +886,7 @@ static int ath9k_hif_usb_alloc_tx_urbs(struct hif_device_usb *hif_dev) static void ath9k_hif_usb_dealloc_rx_urbs(struct hif_device_usb *hif_dev) { usb_kill_anchored_urbs(&hif_dev->rx_submitted); + ath9k_hif_usb_free_rx_remain_skb(hif_dev); } static int ath9k_hif_usb_alloc_rx_urbs(struct hif_device_usb *hif_dev) -- 2.34.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] wifi: ath9k: hif_usb: fix memory leak of remain_skbs 2023-02-16 19:23 ` [PATCH v2] " Fedor Pchelkin @ 2023-02-16 20:54 ` Toke Høiland-Jørgensen 2023-02-20 8:37 ` Kalle Valo 1 sibling, 0 replies; 9+ messages in thread From: Toke Høiland-Jørgensen @ 2023-02-16 20:54 UTC (permalink / raw) To: Fedor Pchelkin Cc: Fedor Pchelkin, Kalle Valo, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, John W. Linville, Vasanthakumar Thiagarajan, Senthil Balasubramanian, Sujith, linux-wireless, netdev, linux-kernel, Alexey Khoroshilov, lvc-project Fedor Pchelkin <pchelkin@ispras.ru> writes: > hif_dev->remain_skb is allocated and used exclusively in > ath9k_hif_usb_rx_stream(). It is implied that an allocated remain_skb is > processed and subsequently freed (in error paths) only during the next > call of ath9k_hif_usb_rx_stream(). > > So, if the urbs are deallocated between those two calls due to the device > deinitialization or suspend, it is possible that ath9k_hif_usb_rx_stream() > is not called next time and the allocated remain_skb is leaked. Our local > Syzkaller instance was able to trigger that. > > 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 ath9k_hif_usb_rx_stream(). Urbs > deallocation supposedly makes that link irrelevant so we need to free the > cached skb in those cases. > > Fix the leak by introducing a function to explicitly free remain_skb (if > it is not NULL) when the rx urbs have been deallocated. remain_skb is NULL > when it has not been allocated at all (hif_dev struct is kzalloced) or > when it has been processed in next call to ath9k_hif_usb_rx_stream(). > > Found by Linux Verification Center (linuxtesting.org) with Syzkaller. > > Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.") > Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru> > Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> Thank you for the fix! Acked-by: Toke Høiland-Jørgensen <toke@toke.dk> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] wifi: ath9k: hif_usb: fix memory leak of remain_skbs 2023-02-16 19:23 ` [PATCH v2] " Fedor Pchelkin 2023-02-16 20:54 ` Toke Høiland-Jørgensen @ 2023-02-20 8:37 ` Kalle Valo 1 sibling, 0 replies; 9+ messages in thread From: Kalle Valo @ 2023-02-20 8:37 UTC (permalink / raw) To: Fedor Pchelkin Cc: Toke Høiland-Jørgensen, Fedor Pchelkin, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, John W. Linville, Vasanthakumar Thiagarajan, Senthil Balasubramanian, Sujith, linux-wireless, netdev, linux-kernel, Alexey Khoroshilov, lvc-project Fedor Pchelkin <pchelkin@ispras.ru> wrote: > hif_dev->remain_skb is allocated and used exclusively in > ath9k_hif_usb_rx_stream(). It is implied that an allocated remain_skb is > processed and subsequently freed (in error paths) only during the next > call of ath9k_hif_usb_rx_stream(). > > So, if the urbs are deallocated between those two calls due to the device > deinitialization or suspend, it is possible that ath9k_hif_usb_rx_stream() > is not called next time and the allocated remain_skb is leaked. Our local > Syzkaller instance was able to trigger that. > > 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 ath9k_hif_usb_rx_stream(). Urbs > deallocation supposedly makes that link irrelevant so we need to free the > cached skb in those cases. > > Fix the leak by introducing a function to explicitly free remain_skb (if > it is not NULL) when the rx urbs have been deallocated. remain_skb is NULL > when it has not been allocated at all (hif_dev struct is kzalloced) or > when it has been processed in next call to ath9k_hif_usb_rx_stream(). > > Found by Linux Verification Center (linuxtesting.org) with Syzkaller. > > Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.") > Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru> > Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> > Acked-by: Toke Høiland-Jørgensen <toke@toke.dk> > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> Patch applied to ath-next branch of ath.git, thanks. 7654cc03eb69 wifi: ath9k: hif_usb: fix memory leak of remain_skbs -- https://patchwork.kernel.org/project/linux-wireless/patch/20230216192301.171225-1-pchelkin@ispras.ru/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-02-20 8:37 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-02-12 14:52 [PATCH 0/1] wifi: ath9k: hif_usb: fix memory leak of remain_skbs Fedor Pchelkin 2023-02-12 14:52 ` [PATCH 1/1] " Fedor Pchelkin 2023-02-16 16:15 ` Toke Høiland-Jørgensen 2023-02-16 17:50 ` Fedor Pchelkin 2023-02-16 18:05 ` Toke Høiland-Jørgensen 2023-02-16 18:44 ` Fedor Pchelkin 2023-02-16 19:23 ` [PATCH v2] " Fedor Pchelkin 2023-02-16 20:54 ` Toke Høiland-Jørgensen 2023-02-20 8:37 ` Kalle Valo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).