drivers/mmc/card/block.c: fix potential null dereference 'idata'

PATCH In review Vladimir Motyka 4 participants 12 messages May 9, 2011 – May 11, 2011

Subsystems (2)

Kernel janitors MMC, SD and SDIO

lore

Share & tools

lore

Vladimir Motyka

author

2011-05-09 14:05 UTC #0 lore raw

PATCH 1/1 drivers/mmc/card/block.c: fix potential null dereference 'idata'

When allocation of idata fails there was a null dereferece.

Signed-off-by Vladimir Motyka <[email protected]>

---

drivers/mmc/card/block.c +0 -0

index 407836d..3dec493 100644
@@ -266,10 +266,10 @@ static struct mmc_blk_ioc_data

*mmc_blk_ioctl_copy_from_user(
return idata;

copy_err:
- kfree(idata->buf);
+ if(idata)
+ kfree(idata->buf);
kfree(idata);
return ERR_PTR(err);
-
}

static int mmc_blk_ioctl_cmd(struct block_device *bdev,

Julia Lawall

2011-05-09 14:32 UTC #1 lore raw

Quoted 1 line · Vladimir Motyka

When allocation of idata fails there was a null dereferece.

Why not have a different label for the two cases? That would make the code easier to statically analyze, and perhaps be more understandable as well.

julia

Quoted 26 lines

Signed-off-by Vladimir Motyka <[email protected]>
---
drivers/mmc/card/block.c +2 -2
index 407836d..3dec493 100644
@@ -266,10 +266,10 @@ static struct mmc_blk_ioc_data
 *mmc_blk_ioctl_copy_from_user(
 	return idata;
 
 copy_err:
-	kfree(idata->buf);
+	if(idata)
+		kfree(idata->buf);
 	kfree(idata);
 	return ERR_PTR(err);
-
 }
 
static int mmc_blk_ioctl_cmd(struct block_device *bdev,
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Vladimir Motyka

author

2011-05-09 15:05 UTC #2 lore raw

Quoted 10 lines · Julia Lawall

Quoted 1 line · Vladimir Motyka
When allocation of idata fails there was a null dereferece.
Why not have a different label for the two cases? That would make the code easier to statically analyze, and perhaps be more understandable as well.
julia

I think You are right. So it could be better like this?

drivers/mmc/card/block.c +0 -0

index 3dec493..a03cdc6 100644
@@ -237,7 +237,7 @@ static struct mmc_blk_ioc_data

*mmc_blk_ioctl_copy_from_user(
idata = kzalloc(sizeof(*idata), GFP_KERNEL); if (!idata) {
err = -ENOMEM;
- goto copy_err;
+ goto alloc_err;
}

if (copy_from_user(&idata->ic, user, sizeof(idata->ic))) {
@@ -266,9 +266,9 @@ static struct mmc_blk_ioc_data *mmc_blk_ioctl_copy_from_user(
return idata;

copy_err:
- if(idata)
- kfree(idata->buf);
+ kfree(idata->buf);
kfree(idata);
+alloc_err:
return ERR_PTR(err);
}

Or it could return right after allocation fails so there needn't be goto. It is simplier, but maybe worse looking and to read. What is your opinion?

Vladimir Motyka

Quoted 27 lines

Quoted 27 lines
Signed-off-by Vladimir Motyka <[email protected]>
---
drivers/mmc/card/block.c +2 -2
index 407836d..3dec493 100644
@@ -266,10 +266,10 @@ static struct mmc_blk_ioc_data
 *mmc_blk_ioctl_copy_from_user(
 	return idata;
 
 copy_err:
-	kfree(idata->buf);
+	if(idata)
+		kfree(idata->buf);
 	kfree(idata);
 	return ERR_PTR(err);
-
 }
 
static int mmc_blk_ioctl_cmd(struct block_device *bdev,
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Andy Shevchenko

2011-05-09 15:13 UTC #3 lore raw

Quoted 31 lines · Vladimir Motyka

Quoted 10 lines · Julia Lawall
Quoted 1 line · Vladimir Motyka
When allocation of idata fails there was a null dereferece.
Why not have a different label for the two cases? That would make the code easier to statically analyze, and perhaps be more understandable as well.
julia
I think You are right. So it could be better like this?
drivers/mmc/card/block.c +1 -1
index 3dec493..a03cdc6 100644
@@ -237,7 +237,7 @@ static struct mmc_blk_ioc_data
 *mmc_blk_ioctl_copy_from_user(
        idata = kzalloc(sizeof(*idata), GFP_KERNEL);
        if (!idata) {
                err = -ENOMEM;
-               goto copy_err;
+               goto alloc_err;
        }
 
if (copy_from_user(&idata->ic, user, sizeof(idata->ic))) {
drivers/mmc/card/block.c +0 -0
@@ -266,9 +266,9 @@ static struct mmc_blk_ioc_data
 *mmc_blk_ioctl_copy_from_user(
        return idata;
 

Quoted 4 lines

copy_err:
- if(idata)
- kfree(idata->buf);
+ kfree(idata->buf);

Make it one patch not series.

Quoted 45 lines

kfree(idata);
+alloc_err:
return ERR_PTR(err);
}
Or it could return right after allocation fails so there needn't be goto. It is simplier, but maybe worse looking and to read. What is your opinion?
Vladimir Motyka
Quoted 21 lines
Quoted 21 lines
Signed-off-by Vladimir Motyka <[email protected]>
---
drivers/mmc/card/block.c +0 -0
index 407836d..3dec493 100644
@@ -266,10 +266,10 @@ static struct mmc_blk_ioc_data
 *mmc_blk_ioctl_copy_from_user(
      return idata;
 
copy_err:
- kfree(idata->buf);
+ if(idata)
+ kfree(idata->buf);
kfree(idata);
return ERR_PTR(err);
-
}
Quoted 6 lines
Quoted 6 lines
static int mmc_blk_ioctl_cmd(struct block_device *bdev,
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
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/

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Julia Lawall

2011-05-09 15:14 UTC #4 lore raw

In reply to Vladimir Motyka

Quoted 40 lines · Vladimir Motyka

Quoted 10 lines · Julia Lawall
Quoted 1 line · Vladimir Motyka
When allocation of idata fails there was a null dereferece.
Why not have a different label for the two cases? That would make the code easier to statically analyze, and perhaps be more understandable as well.
julia
I think You are right. So it could be better like this?
drivers/mmc/card/block.c +1 -1
index 3dec493..a03cdc6 100644
@@ -237,7 +237,7 @@ static struct mmc_blk_ioc_data
 *mmc_blk_ioctl_copy_from_user(
 	idata = kzalloc(sizeof(*idata), GFP_KERNEL);
 	if (!idata) {
 		err = -ENOMEM;
-		goto copy_err;
+		goto alloc_err;
 	}
 
drivers/mmc/card/block.c +2 -2
 	if (copy_from_user(&idata->ic, user, sizeof(idata->ic))) {
@@ -266,9 +266,9 @@ static struct mmc_blk_ioc_data
 *mmc_blk_ioctl_copy_from_user(
 	return idata;
 
 copy_err:
-	if(idata)
-		kfree(idata->buf);
+	kfree(idata->buf);
 	kfree(idata);
+alloc_err:
 	return ERR_PTR(err);
 }

Quoted 3 lines

Or it could return right after allocation fails so there needn't be goto. It is simplier, but maybe worse looking and to read. What is your opinion?

Perhaps it is also pointless to call kfree on something that is known to be NULL. But I think that there is quite some code that does that, so others might have another opinion.

julia

Quoted 36 lines

Vladimir Motyka
Quoted 21 lines
Quoted 21 lines
Signed-off-by Vladimir Motyka <[email protected]>
---
drivers/mmc/card/block.c +2 -2
index 407836d..3dec493 100644
@@ -266,10 +266,10 @@ static struct mmc_blk_ioc_data
 *mmc_blk_ioctl_copy_from_user(
 	return idata;
 
 copy_err:
-	kfree(idata->buf);
+	if(idata)
+		kfree(idata->buf);
 	kfree(idata);
 	return ERR_PTR(err);
-
 }
 
Quoted 6 lines
Quoted 6 lines
static int mmc_blk_ioctl_cmd(struct block_device *bdev,
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Vladimir Motyka

author

2011-05-09 16:08 UTC #5 lore raw

When allocation of idata fails there was a null dereference.

Signed-off-by Vladimir Motyka <[email protected]>

---

drivers/mmc/card/block.c +0 -0

index 407836d..a03cdc6 100644
@@ -237,7 +237,7 @@ static struct mmc_blk_ioc_data

*mmc_blk_ioctl_copy_from_user(
idata = kzalloc(sizeof(*idata), GFP_KERNEL); if (!idata) {
err = -ENOMEM;
- goto copy_err;
+ goto alloc_err;
}

if (copy_from_user(&idata->ic, user, sizeof(idata->ic))) {
@@ -268,8 +268,8 @@ static struct mmc_blk_ioc_data *mmc_blk_ioctl_copy_from_user(
copy_err:
kfree(idata->buf);
kfree(idata);
+alloc_err:
return ERR_PTR(err);
-
}

static int mmc_blk_ioctl_cmd(struct block_device *bdev,

Julia Lawall

2011-05-09 16:12 UTC #6 lore raw

I guess there is also a point at which idata has been successfully allocated but idata->buf has not.

julia

Quoted 34 lines · Vladimir Motyka

When allocation of idata fails there was a null dereference.
Signed-off-by Vladimir Motyka <[email protected]>
---
drivers/mmc/card/block.c +1 -1
index 407836d..a03cdc6 100644
@@ -237,7 +237,7 @@ static struct mmc_blk_ioc_data
 *mmc_blk_ioctl_copy_from_user(
 	idata = kzalloc(sizeof(*idata), GFP_KERNEL);
 	if (!idata) {
 		err = -ENOMEM;
-		goto copy_err;
+		goto alloc_err;
 	}
 
drivers/mmc/card/block.c +1 -1
 	if (copy_from_user(&idata->ic, user, sizeof(idata->ic))) {
@@ -268,8 +268,8 @@ static struct mmc_blk_ioc_data
 *mmc_blk_ioctl_copy_from_user(
 copy_err:
 	kfree(idata->buf);
 	kfree(idata);
+alloc_err:
 	return ERR_PTR(err);
-
 }
 
static int mmc_blk_ioctl_cmd(struct block_device *bdev,
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Vladimir Motyka

author

2011-05-09 20:37 UTC #7 lore raw

Quoted 5 lines · Julia Lawall

I guess there is also a point at which idata has been successfully allocated but idata->buf has not.
julia

Yes there is. Thank You for pointing out.

Vladimir Motyka

Quoted 36 lines · Vladimir Motyka

Quoted 35 lines
When allocation of idata fails there was a null dereference.
Signed-off-by Vladimir Motyka <[email protected]>
---
drivers/mmc/card/block.c +1 -1
index 407836d..a03cdc6 100644
@@ -237,7 +237,7 @@ static struct mmc_blk_ioc_data
 *mmc_blk_ioctl_copy_from_user(
 	idata = kzalloc(sizeof(*idata), GFP_KERNEL);
 	if (!idata) {
 		err = -ENOMEM;
-		goto copy_err;
+		goto alloc_err;
 	}
 
drivers/mmc/card/block.c +1 -1
 	if (copy_from_user(&idata->ic, user, sizeof(idata->ic))) {
@@ -268,8 +268,8 @@ static struct mmc_blk_ioc_data
 *mmc_blk_ioctl_copy_from_user(
 copy_err:
 	kfree(idata->buf);
 	kfree(idata);
+alloc_err:
 	return ERR_PTR(err);
-
 }
 
static int mmc_blk_ioctl_cmd(struct block_device *bdev,
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Vladimir Motyka

author

2011-05-09 20:37 UTC #8 lore raw

In reply to Julia Lawall

When allocation of idata failed there was a null dereference. Also avoid calling kfree where it is needn't.

---

drivers/mmc/card/block.c +0 -0

index 407836d..126c7f4 100644
@@ -237,24 +237,24 @@ static struct mmc_blk_ioc_data

*mmc_blk_ioctl_copy_from_user(
idata = kzalloc(sizeof(*idata), GFP_KERNEL); if (!idata) {
err = -ENOMEM;
- goto copy_err;
+ goto out;
}

if (copy_from_user(&idata->ic, user, sizeof(idata->ic))) {
err = -EFAULT;
- goto copy_err;
+ goto idata_err;
}

idata->buf_bytes = (u64) idata->ic.blksz * idata->ic.blocks; if (idata->buf_bytes > MMC_IOC_MAX_BYTES) {
err = -EOVERFLOW;
- goto copy_err;
+ goto idata_err;
}

idata->buf = kzalloc(idata->buf_bytes, GFP_KERNEL); if (!idata->buf) {
err = -ENOMEM;
- goto copy_err;
+ goto idata_err;
}

if (copy_from_user(idata->buf, (void __user *)(unsigned long)
@@ -267,9 +267,10 @@ static struct mmc_blk_ioc_data *mmc_blk_ioctl_copy_from_user(

copy_err:
kfree(idata->buf);
+idata_err:
kfree(idata);
+out:
return ERR_PTR(err);
-
}

static int mmc_blk_ioctl_cmd(struct block_device *bdev,

Andy Shevchenko

2011-05-10 07:47 UTC #10 lore raw

In reply to Julia Lawall

Quoted 2 lines · Julia Lawall

I guess there is also a point at which idata has been successfully allocated but idata->buf has not.

And? kfree() simple ignores NULL pointers.
I would prefer to see previous version of patch, but let maintainer to choose.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Julia Lawall

2011-05-10 07:57 UTC #11 lore raw

Quoted 4 lines · Andy Shevchenko

Quoted 2 lines · Julia Lawall
I guess there is also a point at which idata has been successfully allocated but idata->buf has not.
And? kfree() simple ignores NULL pointers.

Unnecessarily calling a function suggests that calling that function is necessary when it is not. But it is probably not a big deal, especially for a well known function like kfree.

julia

Quoted 6 lines

I would prefer to see previous version of patch, but let maintainer to choose.
Signature · 2 lines
With Best Regards,
Andy Shevchenko

Chris Ball

2011-05-11 04:01 UTC #9 lore raw

In reply to Vladimir Motyka

Hi,

drivers/mmc/card/block.c static struct mmc_blk_ioc_data · line 237

Quoted 10 lines · Vladimir Motyka

When allocation of idata failed there was a null dereference. Also avoid calling kfree where it is needn't.
---
drivers/mmc/card/block.c +0 -0
index 407836d..126c7f4 100644
@@ -237,24 +237,24 @@ static struct mmc_blk_ioc_data
 *mmc_blk_ioctl_copy_from_user(

Thanks, I've pushed this version of the patch to mmc-next.

(The patch you sent was corrupted by gmail; it added a line break on the last line quoted above where there shouldn't be one. Please fix that for next time.)

- Chris.

Signature · 2 lines

Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child