i2c: busses/i2c-pxa.c: fix potential null pointer dereference error

PATCH Superseded Cong Ding 4 participants 10 messages Feb 4, 2013 – Mar 21, 2013 I2C subsystem host drivers

Share & tools

Cong Ding

author

2013-02-04 22:11 UTC #0 lore raw

PATCH 1/1 i2c: busses/i2c-pxa.c: fix potential null pointer dereference error

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 <[email protected]>

---
 drivers/i2c/busses/i2c-pxa.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/i2c/busses/i2c-pxa.c +2 -1

index 1034d93..eadf1a4 100644
@@ -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;
 }

Signature · 1 line

1.7.9.5

Kyungmin Park

2013-02-04 23:47 UTC #1 lore raw

Hi,

Quoted 13 lines · 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 <[email protected]>
---
 drivers/i2c/busses/i2c-pxa.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/i2c/busses/i2c-pxa.c +0 -0
index 1034d93..eadf1a4 100644

drivers/i2c/busses/i2c-pxa.c eremap: · line 1214

Quoted 5 lines

drivers/i2c/busses/i2c-pxa.c +1 -1

@@ -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

Quoted 12 lines

drivers/i2c/busses/i2c-pxa.c +1 -1
+               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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Cong Ding

author

2013-02-05 00:03 UTC #2 lore raw

Quoted 11 lines · Kyungmin Park

Hi,
Quoted 8 lines · 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 <[email protected]>
---
 drivers/i2c/busses/i2c-pxa.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Quoted 12 lines

drivers/i2c/busses/i2c-pxa.c eremap: · line 1214
Quoted 10 lines
drivers/i2c/busses/i2c-pxa.c +1 -1
index 1034d93..eadf1a4 100644
@@ -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

Quoted 15 lines

Thank you,
Kyungmin Park
Quoted 12 lines
drivers/i2c/busses/i2c-pxa.c +1 -1
+               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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Cong Ding

author

2013-02-05 00:05 UTC #3 lore raw

PATCH v2 1/1 i2c: busses/i2c-pxa.c: fix potential null pointer dereference error

In reply to Kyungmin Park

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 <[email protected]>

---
 drivers/i2c/busses/i2c-pxa.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/i2c/busses/i2c-pxa.c +2 -1

index 1034d93..00df535 100644
@@ -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;
 }

Signature · 1 line

1.7.9.5

Haojian Zhuang

2013-02-05 01:14 UTC #4 lore raw

Quoted 14 lines · 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 <[email protected]>
---
 drivers/i2c/busses/i2c-pxa.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/i2c/busses/i2c-pxa.c +0 -0
index 1034d93..00df535 100644

drivers/i2c/busses/i2c-pxa.c eremap: · line 1218

Quoted 9 lines

drivers/i2c/busses/i2c-pxa.c +2 -1

@@ -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

Cong Ding

author

2013-02-05 10:25 UTC #5 lore raw

Quoted 51 lines · Haojian Zhuang

Quoted 14 lines · 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 <[email protected]>
---
 drivers/i2c/busses/i2c-pxa.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/i2c/busses/i2c-pxa.c +0 -0
index 1034d93..00df535 100644

drivers/i2c/busses/i2c-pxa.c eremap: · line 1218

Quoted 9 lines

drivers/i2c/busses/i2c-pxa.c +2 -1

@@ -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;
        }

Quoted 2 lines

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

Haojian Zhuang

2013-02-05 11:27 UTC #6 lore raw

Quoted 28 lines · Cong Ding

Quoted 27 lines · Haojian Zhuang
Quoted 14 lines · 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 <[email protected]>
---
 drivers/i2c/busses/i2c-pxa.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/i2c/busses/i2c-pxa.c +0 -0
index 1034d93..00df535 100644
drivers/i2c/busses/i2c-pxa.c eremap: · line 1218
Quoted 9 lines
drivers/i2c/busses/i2c-pxa.c +2 -1
@@ -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.

Quoted 1 line

        i2c = kzalloc(sizeof(struct pxa_i2c), GFP_KERNEL);

devm_kzalloc

Quoted 3 lines

        if (!i2c) {
                ret = -ENOMEM;
                goto emalloc;

return -ENOMEM;

Quoted 7 lines

        }

        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;

Quoted 6 lines

        res = platform_get_resource(dev, IORESOURCE_MEM, 0);
        irq = platform_get_irq(dev, 0);
        if (res == NULL || irq < 0) {
                ret = -ENODEV;
                goto eclk;

ditto

Quoted 5 lines

        }

        if (!request_mem_region(res->start, resource_size(res), res->name)) {
                ret = -ENOMEM;
                goto eclk;

ditto

Quoted 7 lines

Quoted 4 lines
        }
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

Cong Ding

author

2013-02-14 11:28 UTC #7 lore raw

PATCH v3 1/1 i2c: busses/i2c-pxa.c: fix potential null pointer dereference error

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 <[email protected]>

---
 drivers/i2c/busses/i2c-pxa.c |   22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

drivers/i2c/busses/i2c-pxa.c +8 ~~-14~~

index 1034d93..dd2d640 100644
@@ -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;

Signature · 1 line

1.7.9.5

Haojian Zhuang

Acked-by on v3 1/1

2013-02-14 16:10 UTC #8 lore raw

Quoted 53 lines · Cong Ding

Signed-off-by Cong Ding <[email protected]>

---
 drivers/i2c/busses/i2c-pxa.c |   22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

drivers/i2c/busses/i2c-pxa.c +8 ~~-15~~

index 1034d93..dd2d640 100644
@@ -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;
--

Quoted 2 lines

1.7.9.5

Acked-by Haojian Zhuang <[email protected]>

Wolfram Sang

2013-03-21 10:54 UTC #9 lore raw

In reply to Cong Ding

Quoted 15 lines · 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. The correct way should be using devm_kzalloc rather than kzalloc to allocate memory.
Signed-off-by Cong Ding <[email protected]>
---
 drivers/i2c/busses/i2c-pxa.c |   22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)
drivers/i2c/busses/i2c-pxa.c +0 -0
index 1034d93..dd2d640 100644

drivers/i2c/busses/i2c-pxa.c static int i2c_pxa_probe(struct platform_device *dev) · line 1097

Quoted 9 lines

drivers/i2c/busses/i2c-pxa.c +1 -5

@@ -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.