[v3] net: ethernet: cavium: octeon: octeon_mgmt: Handle return NULL error from devm_ioremap

[v3] net: ethernet: cavium: octeon: octeon_mgmt: Handle return NULL error from devm_ioremap

[Arvind Yadav]

Here, If devm_ioremap will fail. It will return NULL.
Kernel can run into a NULL-pointer dereference.
This error check will avoid NULL pointer dereference.

Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
---
 drivers/net/ethernet/cavium/octeon/octeon_mgmt.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/cavium/octeon/octeon_mgmt.c b/drivers/net/ethernet/cavium/octeon/octeon_mgmt.c
index 4ab404f..33c2fec 100644
--- a/drivers/net/ethernet/cavium/octeon/octeon_mgmt.c
+++ b/drivers/net/ethernet/cavium/octeon/octeon_mgmt.c
@@ -1479,6 +1479,12 @@ static int octeon_mgmt_probe(struct platform_device *pdev)
 	p->agl = (u64)devm_ioremap(&pdev->dev, p->agl_phys, p->agl_size);
 	p->agl_prt_ctl = (u64)devm_ioremap(&pdev->dev, p->agl_prt_ctl_phys,
 					   p->agl_prt_ctl_size);
+	if (!p->mix || !p->agl || !p->agl_prt_ctl) {
+		dev_err(&pdev->dev, "failed to map I/O memory\n");
+		result = -ENOMEM;
+		goto err;
+	}
+
 	spin_lock_init(&p->lock);
 
 	skb_queue_head_init(&p->tx_list);
-- 
2.7.4

Re: [v3] net: ethernet: cavium: octeon: octeon_mgmt: Handle return NULL error from devm_ioremap

[David Daney]

On 12/14/2016 11:03 AM, Arvind Yadav wrote:
> Here, If devm_ioremap will fail. It will return NULL.
> Kernel can run into a NULL-pointer dereference.
> This error check will avoid NULL pointer dereference.

I have asked you twice already this question, but could not determine 
from your response what the answer is:

Q: Have you tested the patch on OCTEON based hardware that contains the 
"octeon_mgmt" Ethernet ports?  Please answer either "yes" or "no".


Thanks,
David Daney


>
> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
> ---
>  drivers/net/ethernet/cavium/octeon/octeon_mgmt.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/net/ethernet/cavium/octeon/octeon_mgmt.c b/drivers/net/ethernet/cavium/octeon/octeon_mgmt.c
> index 4ab404f..33c2fec 100644
> --- a/drivers/net/ethernet/cavium/octeon/octeon_mgmt.c
> +++ b/drivers/net/ethernet/cavium/octeon/octeon_mgmt.c
> @@ -1479,6 +1479,12 @@ static int octeon_mgmt_probe(struct platform_device *pdev)
>  	p->agl = (u64)devm_ioremap(&pdev->dev, p->agl_phys, p->agl_size);
>  	p->agl_prt_ctl = (u64)devm_ioremap(&pdev->dev, p->agl_prt_ctl_phys,
>  					   p->agl_prt_ctl_size);
> +	if (!p->mix || !p->agl || !p->agl_prt_ctl) {
> +		dev_err(&pdev->dev, "failed to map I/O memory\n");
> +		result = -ENOMEM;
> +		goto err;
> +	}
> +
>  	spin_lock_init(&p->lock);
>
>  	skb_queue_head_init(&p->tx_list);
>

Re: [v3] net: ethernet: cavium: octeon: octeon_mgmt: Handle return NULL error from devm_ioremap

[arvind Yadav]

Hi David,

I did not tested this feature.  I have build it and flashed on hardware.
You can check below commit id. Which has similar check for ioremap.
1- Commit id - de9e397e40f56b9f34af4bf6a5bd7a75ea02456c
       In 'drivers/net/phy/mdio-octeon.c'

2- Commit id - 592569de4c247fe4f25db8369dc0c63860f9560b
       In 'drivers/gpio/gpio-octeon.c'

Thanks
Arvind

On Thursday 15 December 2016 12:58 AM, David Daney wrote:
> On 12/14/2016 11:03 AM, Arvind Yadav wrote:
>> Here, If devm_ioremap will fail. It will return NULL.
>> Kernel can run into a NULL-pointer dereference.
>> This error check will avoid NULL pointer dereference. t
>
> I have asked you twice already this question, but could not determine 
> from your response what the answer is:
>
> Q: Have you tested the patch on OCTEON based hardware that contains 
> the "octeon_mgmt" Ethernet ports?  Please answer either "yes" or "no".
>
>
> Thanks,
> David Daney
>
>
>>
>> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
>> ---
>>  drivers/net/ethernet/cavium/octeon/octeon_mgmt.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/cavium/octeon/octeon_mgmt.c 
>> b/drivers/net/ethernet/cavium/octeon/octeon_mgmt.c
>> index 4ab404f..33c2fec 100644
>> --- a/drivers/net/ethernet/cavium/octeon/octeon_mgmt.c
>> +++ b/drivers/net/ethernet/cavium/octeon/octeon_mgmt.c
>> @@ -1479,6 +1479,12 @@ static int octeon_mgmt_probe(struct 
>> platform_device *pdev)
>>      p->agl = (u64)devm_ioremap(&pdev->dev, p->agl_phys, p->agl_size);
>>      p->agl_prt_ctl = (u64)devm_ioremap(&pdev->dev, p->agl_prt_ctl_phys,
>>                         p->agl_prt_ctl_size);
>> +    if (!p->mix || !p->agl || !p->agl_prt_ctl) {
>> +        dev_err(&pdev->dev, "failed to map I/O memory\n");
>> +        result = -ENOMEM;
>> +        goto err;
>> +    }
>> +
>>      spin_lock_init(&p->lock);
>>
>>      skb_queue_head_init(&p->tx_list);
>>
>

Re: [v3] net: ethernet: cavium: octeon: octeon_mgmt: Handle return NULL error from devm_ioremap

[David Miller]

From: Arvind Yadav <arvind.yadav.cs@gmail.com>
Date: Thu, 15 Dec 2016 00:33:30 +0530

> Here, If devm_ioremap will fail. It will return NULL.
> Kernel can run into a NULL-pointer dereference.
> This error check will avoid NULL pointer dereference.
> 
> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>

Since ioremap() is in fact designed to possibly fail, we do
need to always check it's return value.  So this change is
correct and I have applied it to the 'net' tree.

Thanks.

Re: [v3] net: ethernet: cavium: octeon: octeon_mgmt: Handle return NULL error from devm_ioremap

[David Daney]

On 12/19/2016 08:04 AM, David Miller wrote:
> From: Arvind Yadav <arvind.yadav.cs@gmail.com>
> Date: Thu, 15 Dec 2016 00:33:30 +0530
>
>> Here, If devm_ioremap will fail. It will return NULL.
>> Kernel can run into a NULL-pointer dereference.
>> This error check will avoid NULL pointer dereference.
>>
>> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
>
> Since ioremap() is in fact designed to possibly fail, we do
> need to always check it's return value.  So this change is
> correct and I have applied it to the 'net' tree.

Yes, I think that is fine, although I have not tested the patch.

In general I like to know if a patch fixes a problem that has occurred 
on a platform used by the patch author, or if the author just noticed an 
issue through code inspection or automated tool for a platform that they 
cannot test on.

This patch appears to fall into the second category, but attempts to 
determine this for sure were for the most part unsuccessful.

With respect to ioremap(), in general I agree that it is designed to 
possibly fail.  For mips64 however, I think the implementation can never 
fail.  Certainly testing for failure fits better with what we expect to 
see in Linux kernel code.


>
> Thanks.
>