* [PATCH] i2c: busses/i2c-pxa.c: fix potential null pointer dereference error
@ 2013-02-04 22:11 Cong Ding
2013-02-04 23:47 ` Kyungmin Park
0 siblings, 1 reply; 10+ messages in thread
From: Cong Ding @ 2013-02-04 22:11 UTC (permalink / raw)
To: Wolfram Sang, Andrew Morton, Karol Lewandowski, Kyungmin Park,
Haojian Zhuang, linux-i2c, linux-kernel
Cc: Cong Ding
If it goes to eclk through line 1107, the variable res would be NULL. It will
cause a null pointer dereference error if we call release_mem_region.
Signed-off-by: Cong Ding <dinggnu@gmail.com>
---
drivers/i2c/busses/i2c-pxa.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 1034d93..eadf1a4 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -1211,7 +1211,8 @@ eremap:
eclk:
kfree(i2c);
emalloc:
- release_mem_region(res->start, resource_size(res));
+ if (!res)
+ release_mem_region(res->start, resource_size(res));
return ret;
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] i2c: busses/i2c-pxa.c: fix potential null pointer dereference error
2013-02-04 22:11 [PATCH] i2c: busses/i2c-pxa.c: fix potential null pointer dereference error Cong Ding
@ 2013-02-04 23:47 ` Kyungmin Park
2013-02-05 0:03 ` Cong Ding
2013-02-05 0:05 ` [PATCH v2] " Cong Ding
0 siblings, 2 replies; 10+ messages in thread
From: Kyungmin Park @ 2013-02-04 23:47 UTC (permalink / raw)
To: Cong Ding
Cc: Wolfram Sang, Andrew Morton, Karol Lewandowski, Haojian Zhuang,
linux-i2c, linux-kernel
Hi,
On Tue, Feb 5, 2013 at 7:11 AM, Cong Ding <dinggnu@gmail.com> wrote:
> If it goes to eclk through line 1107, the variable res would be NULL. It will
> cause a null pointer dereference error if we call release_mem_region.
>
> Signed-off-by: Cong Ding <dinggnu@gmail.com>
> ---
> drivers/i2c/busses/i2c-pxa.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
> index 1034d93..eadf1a4 100644
> --- a/drivers/i2c/busses/i2c-pxa.c
> +++ b/drivers/i2c/busses/i2c-pxa.c
> @@ -1211,7 +1211,8 @@ eremap:
> eclk:
> kfree(i2c);
> emalloc:
> - release_mem_region(res->start, resource_size(res));
> + if (!res)
if (res)?
It's not match with description. and it seems wrong.
Thank you,
Kyungmin Park
> + release_mem_region(res->start, resource_size(res));
> return ret;
> }
>
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] i2c: busses/i2c-pxa.c: fix potential null pointer dereference error
2013-02-04 23:47 ` Kyungmin Park
@ 2013-02-05 0:03 ` Cong Ding
2013-02-05 0:05 ` [PATCH v2] " Cong Ding
1 sibling, 0 replies; 10+ messages in thread
From: Cong Ding @ 2013-02-05 0:03 UTC (permalink / raw)
To: Kyungmin Park
Cc: Wolfram Sang, Andrew Morton, Karol Lewandowski, Haojian Zhuang,
linux-i2c, linux-kernel
On Tue, Feb 05, 2013 at 08:47:10AM +0900, Kyungmin Park wrote:
> Hi,
>
> On Tue, Feb 5, 2013 at 7:11 AM, Cong Ding <dinggnu@gmail.com> wrote:
> > If it goes to eclk through line 1107, the variable res would be NULL. It will
> > cause a null pointer dereference error if we call release_mem_region.
> >
> > Signed-off-by: Cong Ding <dinggnu@gmail.com>
> > ---
> > drivers/i2c/busses/i2c-pxa.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
> > index 1034d93..eadf1a4 100644
> > --- a/drivers/i2c/busses/i2c-pxa.c
> > +++ b/drivers/i2c/busses/i2c-pxa.c
> > @@ -1211,7 +1211,8 @@ eremap:
> > eclk:
> > kfree(i2c);
> > emalloc:
> > - release_mem_region(res->start, resource_size(res));
> > + if (!res)
> if (res)?
> It's not match with description. and it seems wrong.
sorry my fault. will send version 2.
- cong
>
> Thank you,
> Kyungmin Park
> > + release_mem_region(res->start, resource_size(res));
> > return ret;
> > }
> >
> > --
> > 1.7.9.5
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] i2c: busses/i2c-pxa.c: fix potential null pointer dereference error
2013-02-04 23:47 ` Kyungmin Park
2013-02-05 0:03 ` Cong Ding
@ 2013-02-05 0:05 ` Cong Ding
2013-02-05 1:14 ` Haojian Zhuang
1 sibling, 1 reply; 10+ messages in thread
From: Cong Ding @ 2013-02-05 0:05 UTC (permalink / raw)
To: Kyungmin Park
Cc: Wolfram Sang, Andrew Morton, Karol Lewandowski, Haojian Zhuang,
linux-i2c, linux-kernel
If it goes to eclk through line 1107, the variable res would be NULL. It will
cause a null pointer dereference error if we call release_mem_region.
Signed-off-by: Cong Ding <dinggnu@gmail.com>
---
drivers/i2c/busses/i2c-pxa.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 1034d93..00df535 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -1211,7 +1211,8 @@ eremap:
eclk:
kfree(i2c);
emalloc:
- release_mem_region(res->start, resource_size(res));
+ if (res)
+ release_mem_region(res->start, resource_size(res));
return ret;
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] i2c: busses/i2c-pxa.c: fix potential null pointer dereference error
2013-02-05 0:05 ` [PATCH v2] " Cong Ding
@ 2013-02-05 1:14 ` Haojian Zhuang
2013-02-05 10:25 ` Cong Ding
0 siblings, 1 reply; 10+ messages in thread
From: Haojian Zhuang @ 2013-02-05 1:14 UTC (permalink / raw)
To: Cong Ding
Cc: Kyungmin Park, Wolfram Sang, Andrew Morton, Karol Lewandowski,
Haojian Zhuang, linux-i2c, linux-kernel
On Tue, Feb 5, 2013 at 8:05 AM, Cong Ding <dinggnu@gmail.com> wrote:
> If it goes to eclk through line 1107, the variable res would be NULL. It will
> cause a null pointer dereference error if we call release_mem_region.
>
> Signed-off-by: Cong Ding <dinggnu@gmail.com>
> ---
> drivers/i2c/busses/i2c-pxa.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
> index 1034d93..00df535 100644
> --- a/drivers/i2c/busses/i2c-pxa.c
> +++ b/drivers/i2c/busses/i2c-pxa.c
> @@ -1211,7 +1211,8 @@ eremap:
> eclk:
> kfree(i2c);
> emalloc:
> - release_mem_region(res->start, resource_size(res));
> + if (res)
> + release_mem_region(res->start, resource_size(res));
> return ret;
> }
>
>
No. I don't agree on this. Your fix can't resolve all potential issues.
i2c = kzalloc(sizeof(struct pxa_i2c), GFP_KERNEL);
if (!i2c) {
ret = -ENOMEM;
goto emalloc;
}
ret = i2c_pxa_probe_dt(dev, i2c, &i2c_type);
if (ret > 0)
ret = i2c_pxa_probe_pdata(dev, i2c, &i2c_type);
if (ret < 0)
goto eclk;
res = platform_get_resource(dev, IORESOURCE_MEM, 0);
irq = platform_get_irq(dev, 0);
if (res == NULL || irq < 0) {
ret = -ENODEV;
goto eclk;
}
if (!request_mem_region(res->start, resource_size(res), res->name)) {
ret = -ENOMEM;
goto eclk;
}
We shouldn't jump to eclk for these error cases. Then we needn't to add
checking on res.
Regards
Haojian
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] i2c: busses/i2c-pxa.c: fix potential null pointer dereference error
2013-02-05 1:14 ` Haojian Zhuang
@ 2013-02-05 10:25 ` Cong Ding
2013-02-05 11:27 ` Haojian Zhuang
0 siblings, 1 reply; 10+ messages in thread
From: Cong Ding @ 2013-02-05 10:25 UTC (permalink / raw)
To: Haojian Zhuang
Cc: Kyungmin Park, Wolfram Sang, Andrew Morton, Karol Lewandowski,
Haojian Zhuang, linux-i2c, linux-kernel
On Tue, Feb 05, 2013 at 09:14:08AM +0800, Haojian Zhuang wrote:
> On Tue, Feb 5, 2013 at 8:05 AM, Cong Ding <dinggnu@gmail.com> wrote:
> > If it goes to eclk through line 1107, the variable res would be NULL. It will
> > cause a null pointer dereference error if we call release_mem_region.
> >
> > Signed-off-by: Cong Ding <dinggnu@gmail.com>
> > ---
> > drivers/i2c/busses/i2c-pxa.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
> > index 1034d93..00df535 100644
> > --- a/drivers/i2c/busses/i2c-pxa.c
> > +++ b/drivers/i2c/busses/i2c-pxa.c
> > @@ -1211,7 +1211,8 @@ eremap:
> > eclk:
> > kfree(i2c);
> > emalloc:
> > - release_mem_region(res->start, resource_size(res));
> > + if (res)
> > + release_mem_region(res->start, resource_size(res));
> > return ret;
> > }
> >
> >
>
> No. I don't agree on this. Your fix can't resolve all potential issues.
>
> i2c = kzalloc(sizeof(struct pxa_i2c), GFP_KERNEL);
> if (!i2c) {
> ret = -ENOMEM;
> goto emalloc;
> }
>
> ret = i2c_pxa_probe_dt(dev, i2c, &i2c_type);
> if (ret > 0)
> ret = i2c_pxa_probe_pdata(dev, i2c, &i2c_type);
> if (ret < 0)
> goto eclk;
>
> res = platform_get_resource(dev, IORESOURCE_MEM, 0);
> irq = platform_get_irq(dev, 0);
> if (res == NULL || irq < 0) {
> ret = -ENODEV;
> goto eclk;
> }
>
> if (!request_mem_region(res->start, resource_size(res), res->name)) {
> ret = -ENOMEM;
> goto eclk;
> }
>
> We shouldn't jump to eclk for these error cases. Then we needn't to add
> checking on res.
So what do you suggest to do for these error cases?
- cong
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] i2c: busses/i2c-pxa.c: fix potential null pointer dereference error
2013-02-05 10:25 ` Cong Ding
@ 2013-02-05 11:27 ` Haojian Zhuang
2013-02-14 11:28 ` [PATCH v3] " Cong Ding
0 siblings, 1 reply; 10+ messages in thread
From: Haojian Zhuang @ 2013-02-05 11:27 UTC (permalink / raw)
To: Cong Ding
Cc: Kyungmin Park, Wolfram Sang, Andrew Morton, Karol Lewandowski,
Haojian Zhuang, linux-i2c, linux-kernel
On Tue, Feb 5, 2013 at 6:25 PM, Cong Ding <dinggnu@gmail.com> wrote:
> On Tue, Feb 05, 2013 at 09:14:08AM +0800, Haojian Zhuang wrote:
>> On Tue, Feb 5, 2013 at 8:05 AM, Cong Ding <dinggnu@gmail.com> wrote:
>> > If it goes to eclk through line 1107, the variable res would be NULL. It will
>> > cause a null pointer dereference error if we call release_mem_region.
>> >
>> > Signed-off-by: Cong Ding <dinggnu@gmail.com>
>> > ---
>> > drivers/i2c/busses/i2c-pxa.c | 3 ++-
>> > 1 file changed, 2 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
>> > index 1034d93..00df535 100644
>> > --- a/drivers/i2c/busses/i2c-pxa.c
>> > +++ b/drivers/i2c/busses/i2c-pxa.c
>> > @@ -1211,7 +1211,8 @@ eremap:
>> > eclk:
>> > kfree(i2c);
>> > emalloc:
>> > - release_mem_region(res->start, resource_size(res));
>> > + if (res)
>> > + release_mem_region(res->start, resource_size(res));
>> > return ret;
>> > }
>> >
>> >
>>
>> No. I don't agree on this. Your fix can't resolve all potential issues.
>>
>> i2c = kzalloc(sizeof(struct pxa_i2c), GFP_KERNEL);
devm_kzalloc
>> if (!i2c) {
>> ret = -ENOMEM;
>> goto emalloc;
return -ENOMEM;
>> }
>>
>> ret = i2c_pxa_probe_dt(dev, i2c, &i2c_type);
>> if (ret > 0)
>> ret = i2c_pxa_probe_pdata(dev, i2c, &i2c_type);
>> if (ret < 0)
>> goto eclk;
return ret;
>>
>> res = platform_get_resource(dev, IORESOURCE_MEM, 0);
>> irq = platform_get_irq(dev, 0);
>> if (res == NULL || irq < 0) {
>> ret = -ENODEV;
>> goto eclk;
ditto
>> }
>>
>> if (!request_mem_region(res->start, resource_size(res), res->name)) {
>> ret = -ENOMEM;
>> goto eclk;
ditto
>> }
>>
>> We shouldn't jump to eclk for these error cases. Then we needn't to add
>> checking on res.
> So what do you suggest to do for these error cases?
> - cong
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3] i2c: busses/i2c-pxa.c: fix potential null pointer dereference error
2013-02-05 11:27 ` Haojian Zhuang
@ 2013-02-14 11:28 ` Cong Ding
2013-02-14 16:10 ` Haojian Zhuang
2013-03-21 10:54 ` Wolfram Sang
0 siblings, 2 replies; 10+ messages in thread
From: Cong Ding @ 2013-02-14 11:28 UTC (permalink / raw)
To: Haojian Zhuang
Cc: Kyungmin Park, Wolfram Sang, Andrew Morton, Karol Lewandowski,
Haojian Zhuang, linux-i2c, linux-kernel
If it goes to eclk through line 1107, the variable res would be NULL. It will
cause a null pointer dereference error if we call release_mem_region. The
correct way should be using devm_kzalloc rather than kzalloc to allocate
memory.
Signed-off-by: Cong Ding <dinggnu@gmail.com>
---
drivers/i2c/busses/i2c-pxa.c | 22 ++++++++--------------
1 file changed, 8 insertions(+), 14 deletions(-)
diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 1034d93..dd2d640 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -1094,29 +1094,23 @@ static int i2c_pxa_probe(struct platform_device *dev)
struct resource *res = NULL;
int ret, irq;
- i2c = kzalloc(sizeof(struct pxa_i2c), GFP_KERNEL);
- if (!i2c) {
- ret = -ENOMEM;
- goto emalloc;
- }
+ i2c = devm_kzalloc(sizeof(struct pxa_i2c), GFP_KERNEL);
+ if (!i2c)
+ return -ENOMEM;
ret = i2c_pxa_probe_dt(dev, i2c, &i2c_type);
if (ret > 0)
ret = i2c_pxa_probe_pdata(dev, i2c, &i2c_type);
if (ret < 0)
- goto eclk;
+ return ret;
res = platform_get_resource(dev, IORESOURCE_MEM, 0);
irq = platform_get_irq(dev, 0);
- if (res == NULL || irq < 0) {
- ret = -ENODEV;
- goto eclk;
- }
+ if (res == NULL || irq < 0)
+ return -ENODEV;
- if (!request_mem_region(res->start, resource_size(res), res->name)) {
- ret = -ENOMEM;
- goto eclk;
- }
+ if (!request_mem_region(res->start, resource_size(res), res->name))
+ return -ENOMEM;
i2c->adap.owner = THIS_MODULE;
i2c->adap.retries = 5;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3] i2c: busses/i2c-pxa.c: fix potential null pointer dereference error
2013-02-14 11:28 ` [PATCH v3] " Cong Ding
@ 2013-02-14 16:10 ` Haojian Zhuang
2013-03-21 10:54 ` Wolfram Sang
1 sibling, 0 replies; 10+ messages in thread
From: Haojian Zhuang @ 2013-02-14 16:10 UTC (permalink / raw)
To: Cong Ding
Cc: Kyungmin Park, Wolfram Sang, Andrew Morton, Karol Lewandowski,
Haojian Zhuang, linux-i2c, linux-kernel
On Thu, Feb 14, 2013 at 7:28 PM, Cong Ding <dinggnu@gmail.com> wrote:
> If it goes to eclk through line 1107, the variable res would be NULL. It will
> cause a null pointer dereference error if we call release_mem_region. The
> correct way should be using devm_kzalloc rather than kzalloc to allocate
> memory.
>
> Signed-off-by: Cong Ding <dinggnu@gmail.com>
> ---
> drivers/i2c/busses/i2c-pxa.c | 22 ++++++++--------------
> 1 file changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
> index 1034d93..dd2d640 100644
> --- a/drivers/i2c/busses/i2c-pxa.c
> +++ b/drivers/i2c/busses/i2c-pxa.c
> @@ -1094,29 +1094,23 @@ static int i2c_pxa_probe(struct platform_device *dev)
> struct resource *res = NULL;
> int ret, irq;
>
> - i2c = kzalloc(sizeof(struct pxa_i2c), GFP_KERNEL);
> - if (!i2c) {
> - ret = -ENOMEM;
> - goto emalloc;
> - }
> + i2c = devm_kzalloc(sizeof(struct pxa_i2c), GFP_KERNEL);
> + if (!i2c)
> + return -ENOMEM;
>
> ret = i2c_pxa_probe_dt(dev, i2c, &i2c_type);
> if (ret > 0)
> ret = i2c_pxa_probe_pdata(dev, i2c, &i2c_type);
> if (ret < 0)
> - goto eclk;
> + return ret;
>
> res = platform_get_resource(dev, IORESOURCE_MEM, 0);
> irq = platform_get_irq(dev, 0);
> - if (res == NULL || irq < 0) {
> - ret = -ENODEV;
> - goto eclk;
> - }
> + if (res == NULL || irq < 0)
> + return -ENODEV;
>
> - if (!request_mem_region(res->start, resource_size(res), res->name)) {
> - ret = -ENOMEM;
> - goto eclk;
> - }
> + if (!request_mem_region(res->start, resource_size(res), res->name))
> + return -ENOMEM;
>
> i2c->adap.owner = THIS_MODULE;
> i2c->adap.retries = 5;
> --
> 1.7.9.5
>
Acked-by: Haojian Zhuang <haojian.zhuang@gmail.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] i2c: busses/i2c-pxa.c: fix potential null pointer dereference error
2013-02-14 11:28 ` [PATCH v3] " Cong Ding
2013-02-14 16:10 ` Haojian Zhuang
@ 2013-03-21 10:54 ` Wolfram Sang
1 sibling, 0 replies; 10+ messages in thread
From: Wolfram Sang @ 2013-03-21 10:54 UTC (permalink / raw)
To: Cong Ding
Cc: Haojian Zhuang, Kyungmin Park, Wolfram Sang, Andrew Morton,
Karol Lewandowski, Haojian Zhuang, linux-i2c, linux-kernel
On Thu, Feb 14, 2013 at 12:28:18PM +0100, Cong Ding wrote:
> If it goes to eclk through line 1107, the variable res would be NULL. It will
> cause a null pointer dereference error if we call release_mem_region. The
> correct way should be using devm_kzalloc rather than kzalloc to allocate
> memory.
>
> Signed-off-by: Cong Ding <dinggnu@gmail.com>
> ---
> drivers/i2c/busses/i2c-pxa.c | 22 ++++++++--------------
> 1 file changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
> index 1034d93..dd2d640 100644
> --- a/drivers/i2c/busses/i2c-pxa.c
> +++ b/drivers/i2c/busses/i2c-pxa.c
> @@ -1094,29 +1094,23 @@ static int i2c_pxa_probe(struct platform_device *dev)
> struct resource *res = NULL;
> int ret, irq;
>
> - i2c = kzalloc(sizeof(struct pxa_i2c), GFP_KERNEL);
> - if (!i2c) {
> - ret = -ENOMEM;
> - goto emalloc;
> - }
> + i2c = devm_kzalloc(sizeof(struct pxa_i2c), GFP_KERNEL);
You are using devm_kzalloc here but plain kfree is used later. This
won't work.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-03-21 10:54 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-02-04 22:11 [PATCH] i2c: busses/i2c-pxa.c: fix potential null pointer dereference error Cong Ding
2013-02-04 23:47 ` Kyungmin Park
2013-02-05 0:03 ` Cong Ding
2013-02-05 0:05 ` [PATCH v2] " Cong Ding
2013-02-05 1:14 ` Haojian Zhuang
2013-02-05 10:25 ` Cong Ding
2013-02-05 11:27 ` Haojian Zhuang
2013-02-14 11:28 ` [PATCH v3] " Cong Ding
2013-02-14 16:10 ` Haojian Zhuang
2013-03-21 10:54 ` Wolfram Sang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).