2020-01-07 08:10:44

by Tony Chuang

Subject: [PATCH] rtw88: fix potential NULL skb access in TX ISR

From: Yan-Hsuan Chuang <[email protected]>

Sometimes the TX queue may be empty and we could possible
dequeue a NULL pointer, crash the kernel. If the skb is NULL
then there is nothing to do, just leave the ISR.

And the TX queue should not be empty here, so print an error
to see if there is anything wrong for DMA ring.

Fixes: e3037485c68e ("rtw88: new Realtek 802.11ac driver")
Signed-off-by: Yan-Hsuan Chuang <[email protected]>
---
drivers/net/wireless/realtek/rtw88/pci.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
index a58e8276a41a..a6746b5a9ff2 100644
--- a/drivers/net/wireless/realtek/rtw88/pci.c
+++ b/drivers/net/wireless/realtek/rtw88/pci.c
@@ -832,6 +832,11 @@ static void rtw_pci_tx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,

while (count--) {
skb = skb_dequeue(&ring->queue);
+ if (!skb) {
+ rtw_err(rtwdev, "failed to dequeue %d skb TX queue %d, BD=0x%08x, rp %d -> %d\n",
+ count, hw_queue, bd_idx, ring->r.rp, cur_rp);
+ break;
+ }
tx_data = rtw_pci_get_tx_data(skb);
pci_unmap_single(rtwpci->pdev, tx_data->dma, skb->len,
PCI_DMA_TODEVICE);
--
2.17.1

2020-01-07 10:41:20

by Chris Chiu

Subject: Re: [PATCH] rtw88: fix potential NULL skb access in TX ISR

On Tue, Jan 7, 2020 at 4:08 PM <[email protected]> wrote:
>
> From: Yan-Hsuan Chuang <[email protected]>
>
> Sometimes the TX queue may be empty and we could possible
> dequeue a NULL pointer, crash the kernel. If the skb is NULL
> then there is nothing to do, just leave the ISR.
>
> And the TX queue should not be empty here, so print an error
> to see if there is anything wrong for DMA ring.
>
> Fixes: e3037485c68e ("rtw88: new Realtek 802.11ac driver")
> Signed-off-by: Yan-Hsuan Chuang <[email protected]>
> ---
> drivers/net/wireless/realtek/rtw88/pci.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
> index a58e8276a41a..a6746b5a9ff2 100644
> --- a/drivers/net/wireless/realtek/rtw88/pci.c
> +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> @@ -832,6 +832,11 @@ static void rtw_pci_tx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
>
> while (count--) {
> skb = skb_dequeue(&ring->queue);
> + if (!skb) {
> + rtw_err(rtwdev, "failed to dequeue %d skb TX queue %d, BD=0x%08x, rp %d -> %d\n",
> + count, hw_queue, bd_idx, ring->r.rp, cur_rp);
> + break;
> + }
> tx_data = rtw_pci_get_tx_data(skb);
> pci_unmap_single(rtwpci->pdev, tx_data->dma, skb->len,
> PCI_DMA_TODEVICE);
> --
> 2.17.1
>

Maybe we can simply do 'while (count -- &&
!skb_queue_empty(&ring->queue))' to achieve the same thing?
I don't think it worths to raise an error unless the count is expected
to exactly match the queue length in any
circumstances.

Chris

2020-01-07 11:22:29

by Tony Chuang

Subject: RE: [PATCH] rtw88: fix potential NULL skb access in TX ISR

From: Chris Chiu
> Subject: Re: [PATCH] rtw88: fix potential NULL skb access in TX ISR
>
> On Tue, Jan 7, 2020 at 4:08 PM <[email protected]> wrote:
> >
> > From: Yan-Hsuan Chuang <[email protected]>
> >
> > Sometimes the TX queue may be empty and we could possible
> > dequeue a NULL pointer, crash the kernel. If the skb is NULL
> > then there is nothing to do, just leave the ISR.
> >
> > And the TX queue should not be empty here, so print an error
> > to see if there is anything wrong for DMA ring.
> >
> > Fixes: e3037485c68e ("rtw88: new Realtek 802.11ac driver")
> > Signed-off-by: Yan-Hsuan Chuang <[email protected]>
> > ---
> > drivers/net/wireless/realtek/rtw88/pci.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c
> b/drivers/net/wireless/realtek/rtw88/pci.c
> > index a58e8276a41a..a6746b5a9ff2 100644
> > --- a/drivers/net/wireless/realtek/rtw88/pci.c
> > +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> > @@ -832,6 +832,11 @@ static void rtw_pci_tx_isr(struct rtw_dev *rtwdev,
> struct rtw_pci *rtwpci,
> >
> > while (count--) {
> > skb = skb_dequeue(&ring->queue);
> > + if (!skb) {
> > + rtw_err(rtwdev, "failed to dequeue %d skb TX
> queue %d, BD=0x%08x, rp %d -> %d\n",
> > + count, hw_queue, bd_idx, ring->r.rp,
> cur_rp);
> > + break;
> > + }
> > tx_data = rtw_pci_get_tx_data(skb);
> > pci_unmap_single(rtwpci->pdev, tx_data->dma,
> skb->len,
> > PCI_DMA_TODEVICE);
> > --
> > 2.17.1
> >
>
> Maybe we can simply do 'while (count -- &&
> !skb_queue_empty(&ring->queue))' to achieve the same thing?
> I don't think it worths to raise an error unless the count is expected
> to exactly match the queue length in any
> circumstances.
>

Yes, I expected that the queue length should match with the DMA ring.
And so I printed an error to see why the count mismatched.

Yan-Hsuan

2020-01-09 11:29:01

by Chris Chiu

Subject: Re: [PATCH] rtw88: fix potential NULL skb access in TX ISR

On Tue, Jan 7, 2020 at 7:21 PM Tony Chuang <[email protected]> wrote:
>
> From: Chris Chiu
> > Subject: Re: [PATCH] rtw88: fix potential NULL skb access in TX ISR
> >
> > On Tue, Jan 7, 2020 at 4:08 PM <[email protected]> wrote:
> > >
> > > From: Yan-Hsuan Chuang <[email protected]>
> > >
> > > Sometimes the TX queue may be empty and we could possible
> > > dequeue a NULL pointer, crash the kernel. If the skb is NULL
> > > then there is nothing to do, just leave the ISR.
> > >
> > > And the TX queue should not be empty here, so print an error
> > > to see if there is anything wrong for DMA ring.
> > >
> > > Fixes: e3037485c68e ("rtw88: new Realtek 802.11ac driver")
> > > Signed-off-by: Yan-Hsuan Chuang <[email protected]>
> > > ---
> > > drivers/net/wireless/realtek/rtw88/pci.c | 5 +++++
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c
> > b/drivers/net/wireless/realtek/rtw88/pci.c
> > > index a58e8276a41a..a6746b5a9ff2 100644
> > > --- a/drivers/net/wireless/realtek/rtw88/pci.c
> > > +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> > > @@ -832,6 +832,11 @@ static void rtw_pci_tx_isr(struct rtw_dev *rtwdev,
> > struct rtw_pci *rtwpci,
> > >
> > > while (count--) {
> > > skb = skb_dequeue(&ring->queue);
> > > + if (!skb) {
> > > + rtw_err(rtwdev, "failed to dequeue %d skb TX
> > queue %d, BD=0x%08x, rp %d -> %d\n",
> > > + count, hw_queue, bd_idx, ring->r.rp,
> > cur_rp);
> > > + break;
> > > + }
> > > tx_data = rtw_pci_get_tx_data(skb);
> > > pci_unmap_single(rtwpci->pdev, tx_data->dma,
> > skb->len,
> > > PCI_DMA_TODEVICE);
> > > --
> > > 2.17.1
> > >
> >
> > Maybe we can simply do 'while (count -- &&
> > !skb_queue_empty(&ring->queue))' to achieve the same thing?
> > I don't think it worths to raise an error unless the count is expected
> > to exactly match the queue length in any
> > circumstances.
> >
>
> Yes, I expected that the queue length should match with the DMA ring.
> And so I printed an error to see why the count mismatched.
>
> Yan-Hsuan

Maybe you can spin lock around skb_dequeue and skb_enqueue to prevent
some possible race conditions?

Chris

2020-01-26 15:43:11

by Kalle Valo

Subject: Re: [PATCH] rtw88: fix potential NULL skb access in TX ISR

<[email protected]> wrote:

> From: Yan-Hsuan Chuang <[email protected]>
>
> Sometimes the TX queue may be empty and we could possible
> dequeue a NULL pointer, crash the kernel. If the skb is NULL
> then there is nothing to do, just leave the ISR.
>
> And the TX queue should not be empty here, so print an error
> to see if there is anything wrong for DMA ring.
>
> Fixes: e3037485c68e ("rtw88: new Realtek 802.11ac driver")
> Signed-off-by: Yan-Hsuan Chuang <[email protected]>

Patch applied to wireless-drivers-next.git, thanks.

f4f84ff8377d rtw88: fix potential NULL skb access in TX ISR

--
https://patchwork.kernel.org/patch/11320567/

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