Re: [PATCH] mwifiex: fix possible NULL dereference

[Andy Shevchenko]

On Tue, Apr 12, 2016 at 8:43 PM, Rustad, Mark D  wrote:
> Andy Shevchenko  wrote:
>
>> On Mon, Apr 11, 2016 at 6:27 PM, Sudip Mukherjee
>>  wrote:
>>>
>>> From: Sudip Mukherjee 
>>>
>>> We have a check for card just after dereferencing it. So if it is NULL
>>> we have already dereferenced it before its check. Lets dereference it
>>> after checking card for NULL.

>>> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
>>> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
>>> @@ -2884,10 +2884,11 @@ static void mwifiex_unregister_dev(struct
>>> mwifiex_adapter *adapter)
>>>  {
>>> struct pcie_service_card *card = adapter->card;>>
>>
>> Let's say it's 0.
>>
>>>const struct mwifiex_pcie_card_reg *reg;
>>> -   struct pci_dev *pdev = card->dev;>>
>>
>> This would be equal to offset of dev member in pcie_service_card struct.
>>
>> Nothing wrong here.
>
> Actually, that is not true. The dereference of card tells the compiler that
> card can't be NULL, so it is free to eliminate the check below.
> Unbelievably, this can even happen for a reference such as >thing where
> the pointer isn't even actually dereferenced at all!

Hmm... Can we look at the result assembly? If I'm not mistaken,
compiler wouldn't even try to calculate pdev pointer before first use
of it.


>
>>> +   struct pci_dev *pdev;
>>> int i;
>>>
>>> if (card) {
>>> +   pdev = card->dev;
>>> if (card->msix_enable) {
>>> for (i = 0; i < MWIFIEX_NUM_MSIX_VECTORS; i++)


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] mwifiex: fix possible NULL dereference

[Rustad, Mark D]

Andy Shevchenko  wrote:


On Mon, Apr 11, 2016 at 6:27 PM, Sudip Mukherjee
 wrote:

From: Sudip Mukherjee 

We have a check for card just after dereferencing it. So if it is NULL
we have already dereferenced it before its check. Lets dereference it
after checking card for NULL.


IIUC the code does nothing with dereference.

I would have told NAK if I would have been maintainer.


Signed-off-by: Sudip Mukherjee 
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c  
b/drivers/net/wireless/marvell/mwifiex/pcie.c

index edf8b07..84562d0 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -2884,10 +2884,11 @@ static void mwifiex_unregister_dev(struct  
mwifiex_adapter *adapter)

 {
struct pcie_service_card *card = adapter->card;


Let's say it's 0.


   const struct mwifiex_pcie_card_reg *reg;
-   struct pci_dev *pdev = card->dev;


This would be equal to offset of dev member in pcie_service_card struct.

Nothing wrong here.


Actually, that is not true. The dereference of card tells the compiler that  
card can't be NULL, so it is free to eliminate the check below.  
Unbelievably, this can even happen for a reference such as >thing  
where the pointer isn't even actually dereferenced at all!



+   struct pci_dev *pdev;
int i;

if (card) {
+   pdev = card->dev;
if (card->msix_enable) {
for (i = 0; i < MWIFIEX_NUM_MSIX_VECTORS; i++)
synchronize_irq(card->msix_entries[i].vector);
--
1.9.1




--
With Best Regards,
Andy Shevchenko



--
Mark Rustad, Networking Division, Intel Corporation


signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] mwifiex: fix possible NULL dereference

[Andy Shevchenko]

On Mon, Apr 11, 2016 at 6:27 PM, Sudip Mukherjee
 wrote:
> From: Sudip Mukherjee 
>
> We have a check for card just after dereferencing it. So if it is NULL
> we have already dereferenced it before its check. Lets dereference it
> after checking card for NULL.

IIUC the code does nothing with dereference.

I would have told NAK if I would have been maintainer.

>
> Signed-off-by: Sudip Mukherjee 
> ---
>  drivers/net/wireless/marvell/mwifiex/pcie.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c 
> b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index edf8b07..84562d0 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -2884,10 +2884,11 @@ static void mwifiex_unregister_dev(struct 
> mwifiex_adapter *adapter)
>  {
> struct pcie_service_card *card = adapter->card;

Let's say it's 0.

> const struct mwifiex_pcie_card_reg *reg;
> -   struct pci_dev *pdev = card->dev;

This would be equal to offset of dev member in pcie_service_card struct.

Nothing wrong here.

> +   struct pci_dev *pdev;
> int i;
>
> if (card) {
> +   pdev = card->dev;
> if (card->msix_enable) {
> for (i = 0; i < MWIFIEX_NUM_MSIX_VECTORS; i++)
> synchronize_irq(card->msix_entries[i].vector);
> --
> 1.9.1
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] mwifiex: fix possible NULL dereference

[Christian Daudt]

On Mon, Apr 11, 2016 at 8:27 AM, Sudip Mukherjee
 wrote:
>
> From: Sudip Mukherjee 
>
> We have a check for card just after dereferencing it. So if it is NULL
> we have already dereferenced it before its check. Lets dereference it
> after checking card for NULL.
>
> Signed-off-by: Sudip Mukherjee 
> ---
>  drivers/net/wireless/marvell/mwifiex/pcie.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c 
> b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index edf8b07..84562d0 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -2884,10 +2884,11 @@ static void mwifiex_unregister_dev(struct 
> mwifiex_adapter *adapter)
>  {
> struct pcie_service_card *card = adapter->card;
> const struct mwifiex_pcie_card_reg *reg;
> -   struct pci_dev *pdev = card->dev;
> +   struct pci_dev *pdev;


you might want to move the variable declaration into the if block
below to avoid it being left undefined

> int i;
>
> if (card) {
> +   pdev = card->dev;


to here
  +struct pci_dev pdev = card->dev;

>
> if (card->msix_enable) {
> for (i = 0; i < MWIFIEX_NUM_MSIX_VECTORS; i++)
> synchronize_irq(card->msix_entries[i].vector);
>

cheers,
  csd

[PATCH] mwifiex: fix possible NULL dereference

[Sudip Mukherjee]

From: Sudip Mukherjee 

We have a check for card just after dereferencing it. So if it is NULL
we have already dereferenced it before its check. Lets dereference it
after checking card for NULL.

Signed-off-by: Sudip Mukherjee 
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c 
b/drivers/net/wireless/marvell/mwifiex/pcie.c
index edf8b07..84562d0 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -2884,10 +2884,11 @@ static void mwifiex_unregister_dev(struct 
mwifiex_adapter *adapter)
 {
struct pcie_service_card *card = adapter->card;
const struct mwifiex_pcie_card_reg *reg;
-   struct pci_dev *pdev = card->dev;
+   struct pci_dev *pdev;
int i;
 
if (card) {
+   pdev = card->dev;
if (card->msix_enable) {
for (i = 0; i < MWIFIEX_NUM_MSIX_VECTORS; i++)
synchronize_irq(card->msix_entries[i].vector);
-- 
1.9.1