* [PATCH] i2c: busses/i2c-pxa.c: fix potential null pointer dereference error @ 2013-02-04 22:11 ` Cong Ding 0 siblings, 0 replies; 17+ 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-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> --- 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] 17+ messages in thread
* [PATCH] i2c: busses/i2c-pxa.c: fix potential null pointer dereference error @ 2013-02-04 22:11 ` Cong Ding 0 siblings, 0 replies; 17+ 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] 17+ messages in thread
* Re: [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 [not found] ` <CAH9JG2XSpFJeUvv7cw-JWadmi=X0-4EsN5FFKuDkwo=-mmzHSQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> -1 siblings, 1 reply; 17+ 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] 17+ messages in thread
[parent not found: <CAH9JG2XSpFJeUvv7cw-JWadmi=X0-4EsN5FFKuDkwo=-mmzHSQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* 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 0 siblings, 0 replies; 17+ 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-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > > --- > > 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.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] 17+ messages in thread
* Re: [PATCH] i2c: busses/i2c-pxa.c: fix potential null pointer dereference error @ 2013-02-05 0:03 ` Cong Ding 0 siblings, 0 replies; 17+ 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] 17+ 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:05 ` Cong Ding 0 siblings, 0 replies; 17+ 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-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> --- 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] 17+ messages in thread
* [PATCH v2] i2c: busses/i2c-pxa.c: fix potential null pointer dereference error @ 2013-02-05 0:05 ` Cong Ding 0 siblings, 0 replies; 17+ 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] 17+ messages in thread
[parent not found: <20130205000517.GB9969-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH v2] i2c: busses/i2c-pxa.c: fix potential null pointer dereference error 2013-02-05 0:05 ` Cong Ding @ 2013-02-05 1:14 ` Haojian Zhuang -1 siblings, 0 replies; 17+ 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-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org On Tue, Feb 5, 2013 at 8:05 AM, Cong Ding <dinggnu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > --- > 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] 17+ messages in thread
* Re: [PATCH v2] i2c: busses/i2c-pxa.c: fix potential null pointer dereference error @ 2013-02-05 1:14 ` Haojian Zhuang 0 siblings, 0 replies; 17+ 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@vger.kernel.org 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] 17+ messages in thread
[parent not found: <CAN1soZwM6qNTJn3vvMdc6TR1eJHc=K+_c0WuWOBBuFYMWB_oCA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* 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 -1 siblings, 0 replies; 17+ 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-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > > --- > > 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] 17+ messages in thread
* Re: [PATCH v2] i2c: busses/i2c-pxa.c: fix potential null pointer dereference error @ 2013-02-05 10:25 ` Cong Ding 0 siblings, 0 replies; 17+ 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@vger.kernel.org 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] 17+ messages in thread
[parent not found: <20130205102540.GC9969-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* 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 -1 siblings, 0 replies; 17+ 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-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org On Tue, Feb 5, 2013 at 6:25 PM, Cong Ding <dinggnu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> >> > --- >> > 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] 17+ messages in thread
* Re: [PATCH v2] i2c: busses/i2c-pxa.c: fix potential null pointer dereference error @ 2013-02-05 11:27 ` Haojian Zhuang 0 siblings, 0 replies; 17+ 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@vger.kernel.org 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] 17+ 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 [not found] ` <20130214112818.GA18774-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2013-03-21 10:54 ` Wolfram Sang -1 siblings, 2 replies; 17+ 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@vger.kernel.org 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] 17+ messages in thread
[parent not found: <20130214112818.GA18774-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* 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; 17+ 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-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org On Thu, Feb 14, 2013 at 7:28 PM, Cong Ding <dinggnu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > --- > 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3] i2c: busses/i2c-pxa.c: fix potential null pointer dereference error @ 2013-02-14 16:10 ` Haojian Zhuang 0 siblings, 0 replies; 17+ 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@vger.kernel.org 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] 17+ 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 [not found] ` <20130214112818.GA18774-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2013-03-21 10:54 ` Wolfram Sang 1 sibling, 0 replies; 17+ 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@vger.kernel.org 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] 17+ messages in thread
end of thread, other threads:[~2013-03-21 10:54 UTC | newest]
Thread overview: 17+ 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 22:11 ` Cong Ding
2013-02-04 23:47 ` Kyungmin Park
[not found] ` <CAH9JG2XSpFJeUvv7cw-JWadmi=X0-4EsN5FFKuDkwo=-mmzHSQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-02-05 0:03 ` Cong Ding
2013-02-05 0:03 ` Cong Ding
2013-02-05 0:05 ` [PATCH v2] " Cong Ding
2013-02-05 0:05 ` Cong Ding
[not found] ` <20130205000517.GB9969-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2013-02-05 1:14 ` Haojian Zhuang
2013-02-05 1:14 ` Haojian Zhuang
[not found] ` <CAN1soZwM6qNTJn3vvMdc6TR1eJHc=K+_c0WuWOBBuFYMWB_oCA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-02-05 10:25 ` Cong Ding
2013-02-05 10:25 ` Cong Ding
[not found] ` <20130205102540.GC9969-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2013-02-05 11:27 ` Haojian Zhuang
2013-02-05 11:27 ` Haojian Zhuang
2013-02-14 11:28 ` [PATCH v3] " Cong Ding
[not found] ` <20130214112818.GA18774-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2013-02-14 16:10 ` Haojian Zhuang
2013-02-14 16:10 ` Haojian Zhuang
2013-03-21 10:54 ` Wolfram Sang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.