ath10k: add missing error handling
diff mbox series

Message ID 20190523071534.254611-1-tientzu@chromium.org
State New
Headers show
Series
  • ath10k: add missing error handling
Related show

Commit Message

Claire Chang May 23, 2019, 7:15 a.m. UTC
In function ath10k_sdio_mbox_rx_alloc() [sdio.c],
ath10k_sdio_mbox_alloc_rx_pkt() is called without handling the error cases.
This will make the driver think the allocation for skb is successful and
try to access the skb. If we enable failslab, system will easily crash with
NULL pointer dereferencing.

Call trace of CONFIG_FAILSLAB:
ath10k_sdio_irq_handler+0x570/0xa88 [ath10k_sdio]
process_sdio_pending_irqs+0x4c/0x174
sdio_run_irqs+0x3c/0x64
sdio_irq_work+0x1c/0x28

Fixes: d96db25d2025 ("ath10k: add initial SDIO support")
Signed-off-by: Claire Chang <tientzu@chromium.org>
---
 drivers/net/wireless/ath/ath10k/sdio.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Brian Norris May 23, 2019, 4:42 p.m. UTC | #1
On Thu, May 23, 2019 at 12:15 AM Claire Chang <tientzu@chromium.org> wrote:
> --- a/drivers/net/wireless/ath/ath10k/sdio.c
> +++ b/drivers/net/wireless/ath/ath10k/sdio.c
> @@ -607,6 +607,10 @@ static int ath10k_sdio_mbox_rx_alloc(struct ath10k *ar,
>                                                     full_len,
>                                                     last_in_bundle,
>                                                     last_in_bundle);
> +               if (ret) {

IIUC, you have basically the same failure case a few lines up, where
ath10k_sdio_mbox_alloc_pkt_bundle() may fail. Do the same there?

This (including the error label to which it's jumping) looks fine to me though:

Reviewed-by: Brian Norris <briannorris@chromium.org>

> +                       ath10k_warn(ar, "alloc_rx_pkt error %d\n", ret);
> +                       goto err;
> +               }
>         }
>
>         ar_sdio->n_rx_pkts = i;
Brian Norris May 23, 2019, 11:18 p.m. UTC | #2
On Thu, May 23, 2019 at 9:42 AM Brian Norris <briannorris@chromium.org> wrote:
> IIUC, you have basically the same failure case a few lines up, where
> ath10k_sdio_mbox_alloc_pkt_bundle() may fail. Do the same there?

Oh, I see Erik Stromdahl already got that one:

ath10k: sdio: add missing error check
https://patchwork.kernel.org/patch/10906009/
Kalle Valo June 25, 2019, 12:58 p.m. UTC | #3
Claire Chang <tientzu@chromium.org> wrote:

> In function ath10k_sdio_mbox_rx_alloc() [sdio.c],
> ath10k_sdio_mbox_alloc_rx_pkt() is called without handling the error cases.
> This will make the driver think the allocation for skb is successful and
> try to access the skb. If we enable failslab, system will easily crash with
> NULL pointer dereferencing.
> 
> Call trace of CONFIG_FAILSLAB:
> ath10k_sdio_irq_handler+0x570/0xa88 [ath10k_sdio]
> process_sdio_pending_irqs+0x4c/0x174
> sdio_run_irqs+0x3c/0x64
> sdio_irq_work+0x1c/0x28
> 
> Fixes: d96db25d2025 ("ath10k: add initial SDIO support")
> Signed-off-by: Claire Chang <tientzu@chromium.org>
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>

Patch applied to ath-next branch of ath.git, thanks.

4b553f3ca4cb ath10k: add missing error handling

Patch
diff mbox series

diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
index 9bbd5b54b8ca..4b7f1c6f13e2 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -607,6 +607,10 @@  static int ath10k_sdio_mbox_rx_alloc(struct ath10k *ar,
 						    full_len,
 						    last_in_bundle,
 						    last_in_bundle);
+		if (ret) {
+			ath10k_warn(ar, "alloc_rx_pkt error %d\n", ret);
+			goto err;
+		}
 	}
 
 	ar_sdio->n_rx_pkts = i;