[v1] wifi: ath9k: htc_hst: free skb in ath9k_htc_rx_msg() if there is no callback function

[PATCH] wifi: ath9k: htc_hst: free skb in ath9k_htc_rx_msg() if there is no callback function

Posted by Fedor Pchelkin 2 years, 8 months ago

It is stated that ath9k_htc_rx_msg() either frees the provided skb or
passes its management to another callback function. However, the skb is
not freed in case there is no another callback function, and Syzkaller was
able to cause a memory leak. Also minor comment fix.

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/htc_hst.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath9k/htc_hst.c b/drivers/net/wireless/ath/ath9k/htc_hst.c
index ca05b07a45e6..7d5041eb5f29 100644
--- a/drivers/net/wireless/ath/ath9k/htc_hst.c
+++ b/drivers/net/wireless/ath/ath9k/htc_hst.c
@@ -391,7 +391,7 @@ static void ath9k_htc_fw_panic_report(struct htc_target *htc_handle,
  * HTC Messages are handled directly here and the obtained SKB
  * is freed.
  *
- * Service messages (Data, WMI) passed to the corresponding
+ * Service messages (Data, WMI) are passed to the corresponding
  * endpoint RX handlers, which have to free the SKB.
  */
 void ath9k_htc_rx_msg(struct htc_target *htc_handle,
@@ -478,6 +478,8 @@ void ath9k_htc_rx_msg(struct htc_target *htc_handle,
 		if (endpoint->ep_callbacks.rx)
 			endpoint->ep_callbacks.rx(endpoint->ep_callbacks.priv,
 						  skb, epid);
+		else
+			kfree_skb(skb);
 	}
 }
 
-- 
2.34.1

[PATCH v2] wifi: ath9k: htc_hst: free skb in ath9k_htc_rx_msg() if there is no callback function

Posted by Fedor Pchelkin 2 years, 8 months ago

It is stated that ath9k_htc_rx_msg() either frees the provided skb or
passes its management to another callback function. However, the skb is
not freed in case there is no another callback function, and Syzkaller was
able to cause a memory leak. Also minor comment fix.

Found by Linux Verification Center (linuxtesting.org) with Syzkaller.

Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
Reported-by: syzbot+e008dccab31bd3647609@syzkaller.appspotmail.com
Reported-by: syzbot+6692c72009680f7c4eb2@syzkaller.appspotmail.com
Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
v1->v2: added Reported-by tag

 drivers/net/wireless/ath/ath9k/htc_hst.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath9k/htc_hst.c b/drivers/net/wireless/ath/ath9k/htc_hst.c
index ca05b07a45e6..7d5041eb5f29 100644
--- a/drivers/net/wireless/ath/ath9k/htc_hst.c
+++ b/drivers/net/wireless/ath/ath9k/htc_hst.c
@@ -391,7 +391,7 @@ static void ath9k_htc_fw_panic_report(struct htc_target *htc_handle,
  * HTC Messages are handled directly here and the obtained SKB
  * is freed.
  *
- * Service messages (Data, WMI) passed to the corresponding
+ * Service messages (Data, WMI) are passed to the corresponding
  * endpoint RX handlers, which have to free the SKB.
  */
 void ath9k_htc_rx_msg(struct htc_target *htc_handle,
@@ -478,6 +478,8 @@ void ath9k_htc_rx_msg(struct htc_target *htc_handle,
 		if (endpoint->ep_callbacks.rx)
 			endpoint->ep_callbacks.rx(endpoint->ep_callbacks.priv,
 						  skb, epid);
+		else
+			kfree_skb(skb);
 	}
 }
 
-- 
2.34.1

Re: [PATCH v2] wifi: ath9k: htc_hst: free skb in ath9k_htc_rx_msg() if there is no callback function

Posted by Toke Høiland-Jørgensen 2 years, 8 months ago

Fedor Pchelkin <pchelkin@ispras.ru> writes:

> It is stated that ath9k_htc_rx_msg() either frees the provided skb or
> passes its management to another callback function. However, the skb is
> not freed in case there is no another callback function, and Syzkaller was
> able to cause a memory leak. Also minor comment fix.
>
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
>
> Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
> Reported-by: syzbot+e008dccab31bd3647609@syzkaller.appspotmail.com
> Reported-by: syzbot+6692c72009680f7c4eb2@syzkaller.appspotmail.com
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
> ---
> v1->v2: added Reported-by tag
>
>  drivers/net/wireless/ath/ath9k/htc_hst.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/htc_hst.c b/drivers/net/wireless/ath/ath9k/htc_hst.c
> index ca05b07a45e6..7d5041eb5f29 100644
> --- a/drivers/net/wireless/ath/ath9k/htc_hst.c
> +++ b/drivers/net/wireless/ath/ath9k/htc_hst.c
> @@ -391,7 +391,7 @@ static void ath9k_htc_fw_panic_report(struct htc_target *htc_handle,
>   * HTC Messages are handled directly here and the obtained SKB
>   * is freed.
>   *
> - * Service messages (Data, WMI) passed to the corresponding
> + * Service messages (Data, WMI) are passed to the corresponding
>   * endpoint RX handlers, which have to free the SKB.
>   */
>  void ath9k_htc_rx_msg(struct htc_target *htc_handle,
> @@ -478,6 +478,8 @@ void ath9k_htc_rx_msg(struct htc_target *htc_handle,
>  		if (endpoint->ep_callbacks.rx)
>  			endpoint->ep_callbacks.rx(endpoint->ep_callbacks.priv,
>  						  skb, epid);
> +		else
> +			kfree_skb(skb);

Shouldn't this be 'goto invalid' like all the other error paths in that
function?

-Toke

Re: [PATCH v2] wifi: ath9k: htc_hst: free skb in ath9k_htc_rx_msg() if there is no callback function

Posted by Fedor Pchelkin 2 years, 8 months ago

> Shouldn't this be 'goto invalid' like all the other error paths in that
> function?

It should. What is also important: I mistakenly chose kfree_skb()
instead of dev_kfree_skb_any() in another patch so I must fix it.
Thanks)

[PATCH v3] wifi: ath9k: htc_hst: free skb in ath9k_htc_rx_msg() if there is no callback function

Posted by Fedor Pchelkin 2 years, 8 months ago

It is stated that ath9k_htc_rx_msg() either frees the provided skb or
passes its management to another callback function. However, the skb is
not freed in case there is no another callback function, and Syzkaller was
able to cause a memory leak. Also minor comment fix.

Found by Linux Verification Center (linuxtesting.org) with Syzkaller.

Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
Reported-by: syzbot+e008dccab31bd3647609@syzkaller.appspotmail.com
Reported-by: syzbot+6692c72009680f7c4eb2@syzkaller.appspotmail.com
Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
v1->v2: added Reported-by tag
v2->v3: use 'goto invalid' instead of freeing skb in place

 drivers/net/wireless/ath/ath9k/htc_hst.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/wireless/ath/ath9k/htc_hst.c b/drivers/net/wireless/ath/ath9k/htc_hst.c
index ca05b07a45e6..0c95f6b145ff 100644
--- a/drivers/net/wireless/ath/ath9k/htc_hst.c
+++ b/drivers/net/wireless/ath/ath9k/htc_hst.c
@@ -478,6 +478,8 @@ void ath9k_htc_rx_msg(struct htc_target *htc_handle,
 		if (endpoint->ep_callbacks.rx)
 			endpoint->ep_callbacks.rx(endpoint->ep_callbacks.priv,
 						  skb, epid);
+		else
+			goto invalid;
 	}
 }
 
-- 
2.34.1

Re: [PATCH v3] wifi: ath9k: htc_hst: free skb in ath9k_htc_rx_msg() if there is no callback function

Posted by Toke Høiland-Jørgensen 2 years, 8 months ago

Fedor Pchelkin <pchelkin@ispras.ru> writes:

> It is stated that ath9k_htc_rx_msg() either frees the provided skb or
> passes its management to another callback function. However, the skb is
> not freed in case there is no another callback function, and Syzkaller was
> able to cause a memory leak. Also minor comment fix.

The comment fix seems to be missing from this version? So either it
should be reinstated, or the commit message updated to not mention it...

-Toke

[PATCH v4] wifi: ath9k: htc_hst: free skb in ath9k_htc_rx_msg() if there is no callback function

Posted by Fedor Pchelkin 2 years, 8 months ago

It is stated that ath9k_htc_rx_msg() either frees the provided skb or
passes its management to another callback function. However, the skb is
not freed in case there is no another callback function, and Syzkaller was
able to cause a memory leak. Also minor comment fix.

Found by Linux Verification Center (linuxtesting.org) with Syzkaller.

Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
Reported-by: syzbot+e008dccab31bd3647609@syzkaller.appspotmail.com
Reported-by: syzbot+6692c72009680f7c4eb2@syzkaller.appspotmail.com
Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
v1->v2: added Reported-by tag
v2->v3: use 'goto invalid' instead of freeing skb in place
v3->v4: fix lost comment

 drivers/net/wireless/ath/ath9k/htc_hst.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath9k/htc_hst.c b/drivers/net/wireless/ath/ath9k/htc_hst.c
index ca05b07a45e6..fe62ff668f75 100644
--- a/drivers/net/wireless/ath/ath9k/htc_hst.c
+++ b/drivers/net/wireless/ath/ath9k/htc_hst.c
@@ -391,7 +391,7 @@ static void ath9k_htc_fw_panic_report(struct htc_target *htc_handle,
  * HTC Messages are handled directly here and the obtained SKB
  * is freed.
  *
- * Service messages (Data, WMI) passed to the corresponding
+ * Service messages (Data, WMI) are passed to the corresponding
  * endpoint RX handlers, which have to free the SKB.
  */
 void ath9k_htc_rx_msg(struct htc_target *htc_handle,
@@ -478,6 +478,8 @@ void ath9k_htc_rx_msg(struct htc_target *htc_handle,
 		if (endpoint->ep_callbacks.rx)
 			endpoint->ep_callbacks.rx(endpoint->ep_callbacks.priv,
 						  skb, epid);
+		else
+			goto invalid;
 	}
 }
 
-- 
2.34.1

Re: [PATCH v4] wifi: ath9k: htc_hst: free skb in ath9k_htc_rx_msg() if there is no callback function

Posted by Kalle Valo 2 years, 8 months ago

Fedor Pchelkin <pchelkin@ispras.ru> wrote:

> It is stated that ath9k_htc_rx_msg() either frees the provided skb or
> passes its management to another callback function. However, the skb is
> not freed in case there is no another callback function, and Syzkaller was
> able to cause a memory leak. Also minor comment fix.
> 
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
> 
> Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
> Reported-by: syzbot+e008dccab31bd3647609@syzkaller.appspotmail.com
> Reported-by: syzbot+6692c72009680f7c4eb2@syzkaller.appspotmail.com
> 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.

9b25e3985477 wifi: ath9k: htc_hst: free skb in ath9k_htc_rx_msg() if there is no callback function

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20230104123546.51427-1-pchelkin@ispras.ru/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v4] wifi: ath9k: htc_hst: free skb in ath9k_htc_rx_msg() if there is no callback function

Posted by Toke Høiland-Jørgensen 2 years, 8 months ago

Fedor Pchelkin <pchelkin@ispras.ru> writes:

> It is stated that ath9k_htc_rx_msg() either frees the provided skb or
> passes its management to another callback function. However, the skb is
> not freed in case there is no another callback function, and Syzkaller was
> able to cause a memory leak. Also minor comment fix.
>
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
>
> Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
> Reported-by: syzbot+e008dccab31bd3647609@syzkaller.appspotmail.com
> Reported-by: syzbot+6692c72009680f7c4eb2@syzkaller.appspotmail.com
> 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>