Re: [PATCH] Fix NULL dereference in rtlwifi driver

[Jesper Juhl]

On Thu, 20 Jan 2011, Larry Finger wrote:

> On 01/20/2011 04:18 PM, Jesper Juhl wrote:
> > In drivers/net/wireless/rtlwifi/pci.c::_rtl_pci_rx_interrupt() we call 
> > dev_alloc_skb(), which may fail and return NULL, but we do not check the 
> > returned value against NULL before dereferencing the returned pointer. 
> > This may lead to a NULL pointer dereference which means we'll crash - not 
> > good.
> > 
> > This patch tries to solve the issue by testing for NULL and bailing out if 
> > we couldn't allocate a skb. However, I don't know this code well, so I'm 
> > not sure that jumping to the 'done' label here is the correct action to 
> > take. Someone more knowledgable about this code than me should definately 
> > review it before it is applied anywhere.
> > While I was in the area I also moved an assignment in 
> > _rtl_pci_init_rx_ring() a bit - if the dev_alloc_skb() call in that 
> > function fails there's no reason to waste clock cycles assigning to the 
> > local variable 'entry', we may as well do that after the NULL check and 
> > potential bail out.
> > 
> > Here's the proposed patch, but please don't take it as much more than a 
> > bug report. If it happens to be correct, then by all means apply it, but 
> > I'm not personally making any guarantees.
> > 
> > Signed-off-by: Jesper Juhl <jj@xxxxxxxxxxxxx>
> > ---
> >  pci.c |    9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> >   compile tested only, I don't have the hardware to test for real.
> > 
> > diff --git a/drivers/net/wireless/rtlwifi/pci.c b/drivers/net/wireless/rtlwifi/pci.c
> > index 0fa36aa..5e99f89 100644
> > --- a/drivers/net/wireless/rtlwifi/pci.c
> > +++ b/drivers/net/wireless/rtlwifi/pci.c
> > @@ -619,6 +619,13 @@ static void _rtl_pci_rx_interrupt(struct ieee80211_hw *hw)
> >  					struct sk_buff *uskb = NULL;
> >  					u8 *pdata;
> >  					uskb = dev_alloc_skb(skb->len + 128);
> > +					if (!uskb) {
> > +						RT_TRACE(rtlpriv,
> > +							(COMP_INTR | COMP_RECV),
> > +							DBG_DMESG,
> > +							("can't alloc rx skb\n"));
> > +						goto done;
> > +					}
> >  					memcpy(IEEE80211_SKB_RXCB(uskb),
> >  							&rx_status,
> >  							sizeof(rx_status));
> > @@ -1066,9 +1073,9 @@ static int _rtl_pci_init_rx_ring(struct ieee80211_hw *hw)
> >  			struct sk_buff *skb =
> >  			    dev_alloc_skb(rtlpci->rxbuffersize);
> >  			u32 bufferaddress;
> > -			entry = &rtlpci->rx_ring[rx_queue_idx].desc[i];
> >  			if (!skb)
> >  				return 0;
> > +			entry = &rtlpci->rx_ring[rx_queue_idx].desc[i];
> >  
> >  			/*skb->dev = dev; */
> >  
> 
> This patch looks mostly good to me. The only change I would make is to replace
> "DBG_DMESG" in the RT_TRACE statement with "DBG_EMERG". The standard setting of
> the debug variable does not generate output for DBG_DMESG.
> 
> With that change, ACK and
> Signed-off-by: Larry Finger <Larry.Finger@xxxxxxxxxxxx>
> 

Here you go.

Signed-off-by: Jesper Juhl <jj@xxxxxxxxxxxxx>
---
 pci.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/rtlwifi/pci.c b/drivers/net/wireless/rtlwifi/pci.c
index 0fa36aa..1758d44 100644
--- a/drivers/net/wireless/rtlwifi/pci.c
+++ b/drivers/net/wireless/rtlwifi/pci.c
@@ -619,6 +619,13 @@ static void _rtl_pci_rx_interrupt(struct ieee80211_hw *hw)
 					struct sk_buff *uskb = NULL;
 					u8 *pdata;
 					uskb = dev_alloc_skb(skb->len + 128);
+					if (!uskb) {
+						RT_TRACE(rtlpriv,
+							(COMP_INTR | COMP_RECV),
+							DBG_EMERG,
+							("can't alloc rx skb\n"));
+						goto done;
+					}
 					memcpy(IEEE80211_SKB_RXCB(uskb),
 							&rx_status,
 							sizeof(rx_status));
@@ -641,7 +648,7 @@ static void _rtl_pci_rx_interrupt(struct ieee80211_hw *hw)
 			new_skb = dev_alloc_skb(rtlpci->rxbuffersize);
 			if (unlikely(!new_skb)) {
 				RT_TRACE(rtlpriv, (COMP_INTR | COMP_RECV),
-					 DBG_DMESG,
+					 DBG_EMERG,
 					 ("can't alloc skb for rx\n"));
 				goto done;
 			}
@@ -1066,9 +1073,9 @@ static int _rtl_pci_init_rx_ring(struct ieee80211_hw *hw)
 			struct sk_buff *skb =
 			    dev_alloc_skb(rtlpci->rxbuffersize);
 			u32 bufferaddress;
-			entry = &rtlpci->rx_ring[rx_queue_idx].desc[i];
 			if (!skb)
 				return 0;
+			entry = &rtlpci->rx_ring[rx_queue_idx].desc[i];
 
 			/*skb->dev = dev; */
 


-- 
Jesper Juhl <jj@xxxxxxxxxxxxx>            http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/