2011-10-28 23:58:58

by Julia Lawall

Subject: [PATCH 2/5] drivers/net/wireless/brcm80211/brcmsmac/dma.c: eliminate a null pointer dereference

From: Julia Lawall <[email protected]>

Delete di->name from the error reporting code, as it is meaningless if di
is NULL.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@r@
expression E, E1;
identifier f;
statement S1,S2,S3;
@@

if (E == NULL)
{
... when != if (E == NULL || ...) S1 else S2
when != E = E1
*E->f
... when any
return ...;
}
else S3
// </smpl>

Signed-off-by: Julia Lawall <[email protected]>

---
drivers/net/wireless/brcm80211/brcmsmac/dma.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/brcm80211/brcmsmac/dma.c b/drivers/net/wireless/brcm80211/brcmsmac/dma.c
index b56a302..1d66f53 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/dma.c
+++ b/drivers/net/wireless/brcm80211/brcmsmac/dma.c
@@ -361,7 +361,7 @@ static uint _dma_ctrlflags(struct dma_info *di, uint mask, uint flags)
uint dmactrlflags = di->dma.dmactrlflags;

if (di == NULL) {
- DMA_ERROR(("%s: _dma_ctrlflags: NULL dma handle\n", di->name));
+ DMA_ERROR(("_dma_ctrlflags: NULL dma handle\n"));
return 0;
}

2011-10-29 09:29:13

by Arend van Spriel

Subject: Re: [PATCH 2/5] drivers/net/wireless/brcm80211/brcmsmac/dma.c: eliminate a null pointer dereference

On 10/29/2011 04:27 AM, Julian Calaby wrote:
> On 29/10/11 10:58, Julia Lawall wrote:
>> From: Julia Lawall <[email protected]>
>>
>> Delete di->name from the error reporting code, as it is meaningless if di
>> is NULL.
>>
>> The semantic match that finds this problem is as follows:
>> (http://coccinelle.lip6.fr/)
>>
>> // <smpl>
>> @r@
>> expression E, E1;
>> identifier f;
>> statement S1,S2,S3;
>> @@
>>
>> if (E == NULL)
>> {
>> ... when != if (E == NULL || ...) S1 else S2
>> when != E = E1
>> *E->f
>> ... when any
>> return ...;
>> }
>> else S3
>> // </smpl>
>>
>> Signed-off-by: Julia Lawall <[email protected]>
>>
>> ---
>> drivers/net/wireless/brcm80211/brcmsmac/dma.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/brcm80211/brcmsmac/dma.c b/drivers/net/wireless/brcm80211/brcmsmac/dma.c
>> index b56a302..1d66f53 100644
>> --- a/drivers/net/wireless/brcm80211/brcmsmac/dma.c
>> +++ b/drivers/net/wireless/brcm80211/brcmsmac/dma.c
>> @@ -361,7 +361,7 @@ static uint _dma_ctrlflags(struct dma_info *di, uint mask, uint flags)
>> uint dmactrlflags = di->dma.dmactrlflags;
>
> If di is null, we've already failed as it's dereferenced here.
>
>> if (di == NULL) {
>> - DMA_ERROR(("%s: _dma_ctrlflags: NULL dma handle\n", di->name));
>> + DMA_ERROR(("_dma_ctrlflags: NULL dma handle\n"));
>> return 0;
>> }
>
> So, a better patch would be something like this:
>
> (apologies if this doesn't apply - I've pretty much built it manually)
>
> ---
>
> Though it's unlikely, di may be null, so we can't dereference
> di->dma.dmactrlflags until we've checked it.
>
> Move this de-reference after the check, and adjust the error
> message to not require de-referencing di.
>
> This is based upon Julia's original patch:
> <[email protected]>
>
> Reported-by: Julia Lawall <[email protected]>
> Signed-off-by: Julian Calaby <[email protected]>
> CC: Julia Lawall <[email protected]>
>
> diff --git a/drivers/net/wireless/brcm80211/brcmsmac/dma.c b/drivers/net/wireless/brcm80211/brcmsmac/dma.c
> index b56a302..6ebec8f 100644
> --- a/drivers/net/wireless/brcm80211/brcmsmac/dma.c
> +++ b/drivers/net/wireless/brcm80211/brcmsmac/dma.c
> @@ -358,13 +358,14 @@ static uint nrxdactive(struct dma_info *di, uint h, uint t
>
> static uint _dma_ctrlflags(struct dma_info *di, uint mask, uint flags)
> {
> - uint dmactrlflags = di->dma.dmactrlflags;
> + uint dmactrlflags;
>
> if (di == NULL) {
> - DMA_ERROR(("%s: _dma_ctrlflags: NULL dma handle\n", di->name));
> + DMA_ERROR(("_dma_ctrlflags: NULL dma handle\n"));
> return 0;
> }
>
> + dmactrlflags = di->dma.dmactrlflags;
> dmactrlflags &= ~mask;
> dmactrlflags |= flags;
>
>
>
>
>

Hi Julian, Julia

That change looks good, but it does not apply on top of our pending
patches. Probably need to resend those after the merge window. I will
create a applying patch and send it to John (and resend later if
needed). Thanks for finding this one.

Gr. AvS

2011-10-29 02:27:41

by Julian Calaby

Subject: Re: [PATCH 2/5] drivers/net/wireless/brcm80211/brcmsmac/dma.c: eliminate a null pointer dereference

On 29/10/11 10:58, Julia Lawall wrote:
> From: Julia Lawall <[email protected]>
>
> Delete di->name from the error reporting code, as it is meaningless if di
> is NULL.
>
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @r@
> expression E, E1;
> identifier f;
> statement S1,S2,S3;
> @@
>
> if (E == NULL)
> {
> ... when != if (E == NULL || ...) S1 else S2
> when != E = E1
> *E->f
> ... when any
> return ...;
> }
> else S3
> // </smpl>
>
> Signed-off-by: Julia Lawall <[email protected]>
>
> ---
> drivers/net/wireless/brcm80211/brcmsmac/dma.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/brcm80211/brcmsmac/dma.c b/drivers/net/wireless/brcm80211/brcmsmac/dma.c
> index b56a302..1d66f53 100644
> --- a/drivers/net/wireless/brcm80211/brcmsmac/dma.c
> +++ b/drivers/net/wireless/brcm80211/brcmsmac/dma.c
> @@ -361,7 +361,7 @@ static uint _dma_ctrlflags(struct dma_info *di, uint mask, uint flags)
> uint dmactrlflags = di->dma.dmactrlflags;

If di is null, we've already failed as it's dereferenced here.

> if (di == NULL) {
> - DMA_ERROR(("%s: _dma_ctrlflags: NULL dma handle\n", di->name));
> + DMA_ERROR(("_dma_ctrlflags: NULL dma handle\n"));
> return 0;
> }

So, a better patch would be something like this:

(apologies if this doesn't apply - I've pretty much built it manually)

---

Though it's unlikely, di may be null, so we can't dereference
di->dma.dmactrlflags until we've checked it.

Move this de-reference after the check, and adjust the error
message to not require de-referencing di.

This is based upon Julia's original patch:
<[email protected]>

Reported-by: Julia Lawall <[email protected]>
Signed-off-by: Julian Calaby <[email protected]>
CC: Julia Lawall <[email protected]>

diff --git a/drivers/net/wireless/brcm80211/brcmsmac/dma.c b/drivers/net/wireless/brcm80211/brcmsmac/dma.c
index b56a302..6ebec8f 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/dma.c
+++ b/drivers/net/wireless/brcm80211/brcmsmac/dma.c
@@ -358,13 +358,14 @@ static uint nrxdactive(struct dma_info *di, uint h, uint t

static uint _dma_ctrlflags(struct dma_info *di, uint mask, uint flags)
{
- uint dmactrlflags = di->dma.dmactrlflags;
+ uint dmactrlflags;

if (di == NULL) {
- DMA_ERROR(("%s: _dma_ctrlflags: NULL dma handle\n", di->name));
+ DMA_ERROR(("_dma_ctrlflags: NULL dma handle\n"));
return 0;
}

+ dmactrlflags = di->dma.dmactrlflags;
dmactrlflags &= ~mask;
dmactrlflags |= flags;





--
Julian Calaby

Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby