| Message ID | 20130205000517.GB9969@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
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(-)