diff mbox series

[-next] pinctrl: nsp: fix potential NULL dereference in nsp_pinmux_probe()

Message ID 1531312461-134547-1-git-send-email-weiyongjun1@huawei.com
State New
Headers show
Series [-next] pinctrl: nsp: fix potential NULL dereference in nsp_pinmux_probe() | expand

Commit Message

Wei Yongjun July 11, 2018, 12:34 p.m. UTC 
platform_get_resource() may fail and return NULL, so we should
better check it's return value to avoid a NULL pointer dereference
a bit later in the code.

This is detected by Coccinelle semantic patch.

@@
expression pdev, res, n, t, e, e1, e2;
@@

res = platform_get_resource(pdev, t, n);
+ if (!res)
+   return -EINVAL;
... when != res == NULL
e = devm_ioremap_nocache(e1, res->start, e2);

Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
---
 drivers/pinctrl/bcm/pinctrl-nsp-mux.c | 2 ++
 1 file changed, 2 insertions(+)


--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Ray Jui July 11, 2018, 4:48 p.m. UTC | #1 
On 7/11/2018 5:34 AM, Wei Yongjun wrote:
> platform_get_resource() may fail and return NULL, so we should
> better check it's return value to avoid a NULL pointer dereference
> a bit later in the code.
> 
> This is detected by Coccinelle semantic patch.
> 
> @@
> expression pdev, res, n, t, e, e1, e2;
> @@
> 
> res = platform_get_resource(pdev, t, n);
> + if (!res)
> +   return -EINVAL;
> ... when != res == NULL
> e = devm_ioremap_nocache(e1, res->start, e2);
> 
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> ---

Reviewed-by: Ray Jui <ray.jui@broadcom.com>

Change looks good to me, although the check could have been avoided if 
'devm_ioremap_resource' is used on the next line instead of 
'devm_ioremap_nocache', where validation of resource pointer is done.

But there's probably a reason why 'devm_ioremap_nocache' was used in 
this code here.

>   drivers/pinctrl/bcm/pinctrl-nsp-mux.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pinctrl/bcm/pinctrl-nsp-mux.c b/drivers/pinctrl/bcm/pinctrl-nsp-mux.c
> index 5cd8166..87618a4 100644
> --- a/drivers/pinctrl/bcm/pinctrl-nsp-mux.c
> +++ b/drivers/pinctrl/bcm/pinctrl-nsp-mux.c
> @@ -577,6 +577,8 @@ static int nsp_pinmux_probe(struct platform_device *pdev)
>   		return PTR_ERR(pinctrl->base0);
>   
>   	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (!res)
> +		return -EINVAL;
>   	pinctrl->base1 = devm_ioremap_nocache(&pdev->dev, res->start,
>   					      resource_size(res));
>   	if (!pinctrl->base1) {
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ray Jui July 11, 2018, 4:56 p.m. UTC | #2 
On 7/11/2018 9:48 AM, Ray Jui wrote:
> 
> 
> On 7/11/2018 5:34 AM, Wei Yongjun wrote:
>> platform_get_resource() may fail and return NULL, so we should
>> better check it's return value to avoid a NULL pointer dereference
>> a bit later in the code.
>>
>> This is detected by Coccinelle semantic patch.
>>
>> @@
>> expression pdev, res, n, t, e, e1, e2;
>> @@
>>
>> res = platform_get_resource(pdev, t, n);
>> + if (!res)
>> +   return -EINVAL;
>> ... when != res == NULL
>> e = devm_ioremap_nocache(e1, res->start, e2);

I forgot to mention this in my previous reply. Given that this is a fix 
for a potential NULL pointer dereference and then a kernel crash in the 
case when 'platform_get_resource' returns NULL, can you please add the 
Fixes tag so this fix is picked by all LTS kernels under maintenance?

Thanks,

Ray

>>
>> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
>> ---
> 
> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> 
> Change looks good to me, although the check could have been avoided if 
> 'devm_ioremap_resource' is used on the next line instead of 
> 'devm_ioremap_nocache', where validation of resource pointer is done.
> 
> But there's probably a reason why 'devm_ioremap_nocache' was used in 
> this code here.
>

>>   drivers/pinctrl/bcm/pinctrl-nsp-mux.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/pinctrl/bcm/pinctrl-nsp-mux.c 
>> b/drivers/pinctrl/bcm/pinctrl-nsp-mux.c
>> index 5cd8166..87618a4 100644
>> --- a/drivers/pinctrl/bcm/pinctrl-nsp-mux.c
>> +++ b/drivers/pinctrl/bcm/pinctrl-nsp-mux.c
>> @@ -577,6 +577,8 @@ static int nsp_pinmux_probe(struct platform_device 
>> *pdev)
>>           return PTR_ERR(pinctrl->base0);
>>       res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> +    if (!res)
>> +        return -EINVAL;
>>       pinctrl->base1 = devm_ioremap_nocache(&pdev->dev, res->start,
>>                             resource_size(res));
>>       if (!pinctrl->base1) {
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sudeep Holla July 11, 2018, 5:01 p.m. UTC | #3 
On 11/07/18 17:48, Ray Jui wrote:
> 
> 
> On 7/11/2018 5:34 AM, Wei Yongjun wrote:
>> platform_get_resource() may fail and return NULL, so we should
>> better check it's return value to avoid a NULL pointer dereference
>> a bit later in the code.
>>
>> This is detected by Coccinelle semantic patch.
>>
>> @@
>> expression pdev, res, n, t, e, e1, e2;
>> @@
>>
>> res = platform_get_resource(pdev, t, n);
>> + if (!res)
>> +   return -EINVAL;
>> ... when != res == NULL
>> e = devm_ioremap_nocache(e1, res->start, e2);
>>
>> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
>> ---
> 
> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> 
> Change looks good to me, although the check could have been avoided if
> 'devm_ioremap_resource' is used on the next line instead of
> 'devm_ioremap_nocache', where validation of resource pointer is done.
> 
> But there's probably a reason why 'devm_ioremap_nocache' was used in
> this code here.
> 

I am not sure about that. Both ARM and ARM64 has same definition as
ioremp. However, arch/arm/include/asm/io.h do mention:
"ioremap_nocache() is the same as ioremap() as there are too many device

 drivers using this for device registers, and documentation which tells

 people to use it for such for this to be any different."

You could technically use devm_ioremap_resource if you want.
Ray Jui July 11, 2018, 5:11 p.m. UTC | #4 
On 7/11/2018 10:01 AM, Sudeep Holla wrote:
> 
> 
> On 11/07/18 17:48, Ray Jui wrote:
>>
>>
>> On 7/11/2018 5:34 AM, Wei Yongjun wrote:
>>> platform_get_resource() may fail and return NULL, so we should
>>> better check it's return value to avoid a NULL pointer dereference
>>> a bit later in the code.
>>>
>>> This is detected by Coccinelle semantic patch.
>>>
>>> @@
>>> expression pdev, res, n, t, e, e1, e2;
>>> @@
>>>
>>> res = platform_get_resource(pdev, t, n);
>>> + if (!res)
>>> +   return -EINVAL;
>>> ... when != res == NULL
>>> e = devm_ioremap_nocache(e1, res->start, e2);
>>>
>>> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
>>> ---
>>
>> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
>>
>> Change looks good to me, although the check could have been avoided if
>> 'devm_ioremap_resource' is used on the next line instead of
>> 'devm_ioremap_nocache', where validation of resource pointer is done.
>>
>> But there's probably a reason why 'devm_ioremap_nocache' was used in
>> this code here.
>>
> 
> I am not sure about that. Both ARM and ARM64 has same definition as
> ioremp. However, arch/arm/include/asm/io.h do mention:
> "ioremap_nocache() is the same as ioremap() as there are too many device
> 
>   drivers using this for device registers, and documentation which tells
> 
>   people to use it for such for this to be any different."
> 
> You could technically use devm_ioremap_resource if you want.
> 

I did not mean the difference on _nocache, which I'm aware it's the same 
on ARM/ARM64 based platforms.

I meant there's a reason why xxx_resource was not used, which is most 
likely due to some resource conflict with another driver on NSP.

Ray
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sudeep Holla July 11, 2018, 5:14 p.m. UTC | #5 
On 11/07/18 18:11, Ray Jui wrote:
> 
> 

[...]

> 
> I meant there's a reason why xxx_resource was not used, which is most
> likely due to some resource conflict with another driver on NSP.
> 

Ah OK, sorry for the noise then.
Ray Jui July 11, 2018, 5:18 p.m. UTC | #6 
On 7/11/2018 10:14 AM, Sudeep Holla wrote:
> 
> 
> On 11/07/18 18:11, Ray Jui wrote:
>>
>>
> 
> [...]
> 
>>
>> I meant there's a reason why xxx_resource was not used, which is most
>> likely due to some resource conflict with another driver on NSP.
>>
> 
> Ah OK, sorry for the noise then.
> 

Not a noise at all. Helpful discussion.

Thanks!

Ray
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij July 13, 2018, 7:46 a.m. UTC | #7 
On Wed, Jul 11, 2018 at 2:25 PM Wei Yongjun <weiyongjun1@huawei.com> wrote:

> platform_get_resource() may fail and return NULL, so we should
> better check it's return value to avoid a NULL pointer dereference
> a bit later in the code.
>
> This is detected by Coccinelle semantic patch.
>
> @@
> expression pdev, res, n, t, e, e1, e2;
> @@
>
> res = platform_get_resource(pdev, t, n);
> + if (!res)
> +   return -EINVAL;
> ... when != res == NULL
> e = devm_ioremap_nocache(e1, res->start, e2);
>
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>

Patch applied with Ray's ACK.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ray Jui July 13, 2018, 4:53 p.m. UTC | #8 
Hi Linus,

On 7/13/2018 12:46 AM, Linus Walleij wrote:
> On Wed, Jul 11, 2018 at 2:25 PM Wei Yongjun <weiyongjun1@huawei.com> wrote:
> 
>> platform_get_resource() may fail and return NULL, so we should
>> better check it's return value to avoid a NULL pointer dereference
>> a bit later in the code.
>>
>> This is detected by Coccinelle semantic patch.
>>
>> @@
>> expression pdev, res, n, t, e, e1, e2;
>> @@
>>
>> res = platform_get_resource(pdev, t, n);
>> + if (!res)
>> +   return -EINVAL;
>> ... when != res == NULL
>> e = devm_ioremap_nocache(e1, res->start, e2);
>>
>> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> 
> Patch applied with Ray's ACK.

Would be nice to add the following Fixes tag:

Fixes: cc4fa83f66e9 ("pinctrl: nsp: add pinmux driver support for 
Broadcom NSP SoC")

Thanks,

Ray

> 
> Yours,
> Linus Walleij
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij July 14, 2018, 10:49 a.m. UTC | #9 
On Fri, Jul 13, 2018 at 6:53 PM Ray Jui <ray.jui@broadcom.com> wrote:

> > Patch applied with Ray's ACK.
>
> Would be nice to add the following Fixes tag:
>
> Fixes: cc4fa83f66e9 ("pinctrl: nsp: add pinmux driver support for
> Broadcom NSP SoC")

OK fixed it!

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

diff --git a/drivers/pinctrl/bcm/pinctrl-nsp-mux.c b/drivers/pinctrl/bcm/pinctrl-nsp-mux.c
index 5cd8166..87618a4 100644
--- a/drivers/pinctrl/bcm/pinctrl-nsp-mux.c
+++ b/drivers/pinctrl/bcm/pinctrl-nsp-mux.c
@@ -577,6 +577,8 @@  static int nsp_pinmux_probe(struct platform_device *pdev)
 		return PTR_ERR(pinctrl->base0);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!res)
+		return -EINVAL;
 	pinctrl->base1 = devm_ioremap_nocache(&pdev->dev, res->start,
 					      resource_size(res));
 	if (!pinctrl->base1) {