diff mbox series

[v3] mmc: block: Fix bug when removing RPMB chardev

Message ID 20171003135123.4934-1-linus.walleij@linaro.org
State Superseded
Headers show
Series [v3] mmc: block: Fix bug when removing RPMB chardev | expand

Commit Message

Linus Walleij Oct. 3, 2017, 1:51 p.m. UTC
I forgot to account for the fact that the device core holds a
reference to a device added with device_initialize() that need
to be released with a corresponding put_device() to reach a 0
refcount at the end of the lifecycle.

This led to a NULL pointer reference when freeing the device
when e.g. unbidning the host device in sysfs.

Fix this and use the device .release() callback to free the
IDA and free:ing the memory used by the RPMB device.

Before this patch:

/sys/bus/amba/drivers/mmci-pl18x$ echo 80114000.sdi4_per2 > unbind
[   29.797332] mmc3: card 0001 removed
[   29.810791] Unable to handle kernel NULL pointer dereference at
               virtual address 00000050
[   29.818878] pgd = de70c000
[   29.821624] [00000050] *pgd=1e70a831, *pte=00000000, *ppte=00000000
[   29.827911] Internal error: Oops: 17 [#1] PREEMPT SMP ARM
[   29.833282] Modules linked in:
[   29.836334] CPU: 1 PID: 154 Comm: sh Not tainted
               4.14.0-rc3-00039-g83318e309566-dirty #736
[   29.844604] Hardware name: ST-Ericsson Ux5x0 platform (Device Tree Support)
[   29.851562] task: de572700 task.stack: de742000
[   29.856079] PC is at kernfs_find_ns+0x8/0x100
[   29.860443] LR is at kernfs_find_and_get_ns+0x30/0x48

After this patch:

/sys/bus/amba/drivers/mmci-pl18x$ echo 80005000.sdi4_per2 > unbind
[   20.623382] mmc3: card 0001 removed

Fixes: 0ab0c7c0e65a ("mmc: block: Convert RPMB to a character device")
Reported-by: Adrian Hunter <adrian.hunter@intel.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
ChangeLog v2->v3:
- Fix the block device reference counting: instead of manipulating
  md->usage directly, call the proper refcounting functions
  mmc_blk_get() and mmc_blk_put().
ChangeLog v1->v2:
- Drop the print from removing the RPMB partition
- Refactor the error path of the RPMB to use refcounting
- Drop the verbose comment on refcounts
---
 drivers/mmc/core/block.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

-- 
2.13.5

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Adrian Hunter Oct. 4, 2017, 7:39 a.m. UTC | #1
On 03/10/17 16:51, Linus Walleij wrote:
> I forgot to account for the fact that the device core holds a

> reference to a device added with device_initialize() that need

> to be released with a corresponding put_device() to reach a 0

> refcount at the end of the lifecycle.

> 

> This led to a NULL pointer reference when freeing the device

> when e.g. unbidning the host device in sysfs.

> 

> Fix this and use the device .release() callback to free the

> IDA and free:ing the memory used by the RPMB device.

> 

> Before this patch:

> 

> /sys/bus/amba/drivers/mmci-pl18x$ echo 80114000.sdi4_per2 > unbind

> [   29.797332] mmc3: card 0001 removed

> [   29.810791] Unable to handle kernel NULL pointer dereference at

>                virtual address 00000050

> [   29.818878] pgd = de70c000

> [   29.821624] [00000050] *pgd=1e70a831, *pte=00000000, *ppte=00000000

> [   29.827911] Internal error: Oops: 17 [#1] PREEMPT SMP ARM

> [   29.833282] Modules linked in:

> [   29.836334] CPU: 1 PID: 154 Comm: sh Not tainted

>                4.14.0-rc3-00039-g83318e309566-dirty #736

> [   29.844604] Hardware name: ST-Ericsson Ux5x0 platform (Device Tree Support)

> [   29.851562] task: de572700 task.stack: de742000

> [   29.856079] PC is at kernfs_find_ns+0x8/0x100

> [   29.860443] LR is at kernfs_find_and_get_ns+0x30/0x48

> 

> After this patch:

> 

> /sys/bus/amba/drivers/mmci-pl18x$ echo 80005000.sdi4_per2 > unbind

> [   20.623382] mmc3: card 0001 removed

> 

> Fixes: 0ab0c7c0e65a ("mmc: block: Convert RPMB to a character device")

> Reported-by: Adrian Hunter <adrian.hunter@intel.com>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---

> ChangeLog v2->v3:

> - Fix the block device reference counting: instead of manipulating

>   md->usage directly, call the proper refcounting functions

>   mmc_blk_get() and mmc_blk_put().

> ChangeLog v1->v2:

> - Drop the print from removing the RPMB partition

> - Refactor the error path of the RPMB to use refcounting

> - Drop the verbose comment on refcounts

> ---

>  drivers/mmc/core/block.c | 28 ++++++++++++++++++----------

>  1 file changed, 18 insertions(+), 10 deletions(-)

> 

> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c

> index c29dbcec7c61..46ecbaec8035 100644

> --- a/drivers/mmc/core/block.c

> +++ b/drivers/mmc/core/block.c

> @@ -2299,7 +2299,7 @@ static int mmc_rpmb_chrdev_open(struct inode *inode, struct file *filp)

>  	get_device(&rpmb->dev);

>  	filp->private_data = rpmb;

>  	mutex_lock(&open_lock);

> -	rpmb->md->usage++;

> +	mmc_blk_get(rpmb->md->disk);


mmc_blk_get() also tries to mutex_lock(&open_lock)

But how do you know md still exists?  Looks like you need
to make the disk the parent of the rpmb device and then
use the disk to get a reference to md.

Another thing that looks odd: mmc_blk_ioctl() checks for
capable(CAP_SYS_RAWIO) but mmc_rpmb_ioctl() does not.
Is that intentional?

>  	mutex_unlock(&open_lock);

>  

>  	return nonseekable_open(inode, filp);

> @@ -2312,7 +2312,7 @@ static int mmc_rpmb_chrdev_release(struct inode *inode, struct file *filp)

>  

>  	put_device(&rpmb->dev);

>  	mutex_lock(&open_lock);

> -	rpmb->md->usage--;

> +	mmc_blk_put(rpmb->md);

>  	mutex_unlock(&open_lock);

>  

>  	return 0;

> @@ -2329,6 +2329,13 @@ static const struct file_operations mmc_rpmb_fileops = {

>  #endif

>  };

>  

> +static void mmc_blk_rpmb_device_release(struct device *dev)

> +{

> +	struct mmc_rpmb_data *rpmb = dev_get_drvdata(dev);

> +

> +	ida_simple_remove(&mmc_rpmb_ida, rpmb->id);

> +	kfree(rpmb);

> +}

>  

>  static int mmc_blk_alloc_rpmb_part(struct mmc_card *card,

>  				   struct mmc_blk_data *md,

> @@ -2347,8 +2354,10 @@ static int mmc_blk_alloc_rpmb_part(struct mmc_card *card,

>  		return devidx;

>  

>  	rpmb = kzalloc(sizeof(*rpmb), GFP_KERNEL);

> -	if (!rpmb)

> +	if (!rpmb) {

> +		ida_simple_remove(&mmc_rpmb_ida, devidx);

>  		return -ENOMEM;

> +	}

>  

>  	snprintf(rpmb_name, sizeof(rpmb_name),

>  		 "mmcblk%u%s", card->host->index, subname ? subname : "");

> @@ -2359,6 +2368,7 @@ static int mmc_blk_alloc_rpmb_part(struct mmc_card *card,

>  	rpmb->dev.bus = &mmc_rpmb_bus_type;

>  	rpmb->dev.devt = MKDEV(MAJOR(mmc_rpmb_devt), rpmb->id);

>  	rpmb->dev.parent = &card->dev;

> +	rpmb->dev.release = mmc_blk_rpmb_device_release;

>  	device_initialize(&rpmb->dev);

>  	dev_set_drvdata(&rpmb->dev, rpmb);

>  	rpmb->md = md;

> @@ -2368,7 +2378,7 @@ static int mmc_blk_alloc_rpmb_part(struct mmc_card *card,

>  	ret = cdev_device_add(&rpmb->chrdev, &rpmb->dev);

>  	if (ret) {

>  		pr_err("%s: could not add character device\n", rpmb_name);

> -		goto out_remove_ida;

> +		goto out_put_device;

>  	}

>  

>  	list_add(&rpmb->node, &md->rpmbs);

> @@ -2383,18 +2393,16 @@ static int mmc_blk_alloc_rpmb_part(struct mmc_card *card,

>  

>  	return 0;

>  

> -out_remove_ida:

> -	ida_simple_remove(&mmc_rpmb_ida, rpmb->id);

> -	kfree(rpmb);

> +out_put_device:

> +	put_device(&rpmb->dev);

>  	return ret;

>  }

>  

>  static void mmc_blk_remove_rpmb_part(struct mmc_rpmb_data *rpmb)

> +

>  {

>  	cdev_device_del(&rpmb->chrdev, &rpmb->dev);

> -	device_del(&rpmb->dev);

> -	ida_simple_remove(&mmc_rpmb_ida, rpmb->id);

> -	kfree(rpmb);

> +	put_device(&rpmb->dev);

>  }

>  

>  /* MMC Physical partitions consist of two boot partitions and

> 


--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Oct. 4, 2017, 9:09 a.m. UTC | #2
On Wed, Oct 4, 2017 at 9:39 AM, Adrian Hunter <adrian.hunter@intel.com> wrote:

>> @@ -2299,7 +2299,7 @@ static int mmc_rpmb_chrdev_open(struct inode *inode, struct file *filp)

>>       get_device(&rpmb->dev);

>>       filp->private_data = rpmb;

>>       mutex_lock(&open_lock);

>> -     rpmb->md->usage++;

>> +     mmc_blk_get(rpmb->md->disk);

>

> mmc_blk_get() also tries to mutex_lock(&open_lock)


Yeah :/ I fix.
Sorry for this flunky (too)...

> But how do you know md still exists?


I was thinking that the reference counting is exactly for making sure
it exists across open()/close()?

>  Looks like you need

> to make the disk the parent of the rpmb device and then

> use the disk to get a reference to md.


That sounds like it can be done relatively easily. Sounds like it should be a
separate patch though, let me iterate this one first so we plug the biggest
holes and then I try to come up with something for that too.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Adrian Hunter Oct. 4, 2017, 11:58 a.m. UTC | #3
On 04/10/17 12:09, Linus Walleij wrote:
> On Wed, Oct 4, 2017 at 9:39 AM, Adrian Hunter <adrian.hunter@intel.com> wrote:

> 

>>> @@ -2299,7 +2299,7 @@ static int mmc_rpmb_chrdev_open(struct inode *inode, struct file *filp)

>>>       get_device(&rpmb->dev);

>>>       filp->private_data = rpmb;

>>>       mutex_lock(&open_lock);

>>> -     rpmb->md->usage++;

>>> +     mmc_blk_get(rpmb->md->disk);

>>

>> mmc_blk_get() also tries to mutex_lock(&open_lock)

> 

> Yeah :/ I fix.

> Sorry for this flunky (too)...

> 

>> But how do you know md still exists?

> 

> I was thinking that the reference counting is exactly for making sure

> it exists across open()/close()?


Previously the device being opened was the block device which had to exist.

But I think it is OK to assume md exists in mmc_rpmb_chrdev_open() because
rpmb->chrdev is removed before the main disk.

> 

>>  Looks like you need

>> to make the disk the parent of the rpmb device and then

>> use the disk to get a reference to md.

> 

> That sounds like it can be done relatively easily. Sounds like it should be a

> separate patch though, let me iterate this one first so we plug the biggest

> holes and then I try to come up with something for that too.


OK.  What about the "capable(CAP_SYS_RAWIO)" question?
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Oct. 5, 2017, 10:48 a.m. UTC | #4
On Wed, Oct 4, 2017 at 1:58 PM, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 04/10/17 12:09, Linus Walleij wrote:


>>> But how do you know md still exists?

>>

>> I was thinking that the reference counting is exactly for making sure

>> it exists across open()/close()?

>

> Previously the device being opened was the block device which had to exist.

>

> But I think it is OK to assume md exists in mmc_rpmb_chrdev_open() because

> rpmb->chrdev is removed before the main disk.


That's my thought too. But thinking it over there could be a race if e.g.
you write a script with a loop hammering the device in/out with bind/unbind
at the same time opening and closing the RPMB chardev and holding
a handle to it.

It is not a very realistic usecase but I will try to provoke it anyways.

> OK.  What about the "capable(CAP_SYS_RAWIO)" question?


The flag has a very strange definition in
include/uapi/linux/capability.h:

/* Allow sending USB messages to any device via /dev/bus/usb */
#define CAP_SYS_RAWIO        17

So I read in the codebase how this flag is actuallt used. It seems to
universally apply for cases where userspace is allowed to read or
write directly into device registers.

So I dropped that since the character device is only able to do ioctl()s,
no raw I/O. RPMB is and always was strictly ioctl-based.
We should simply plug the hole and not letting people shoot themselves
in the foot with this.

I think it is bogus that we have it even for the block device, but it may
be argued that the stuff returned from the ioctl reading card info
is "kind of" RAWIO. I guess that is why it's there. But we are using well
structured ioctls so it is really irrelevant. The flag is not
needed or used for anything (the subsystems using it is checking for
it explicitly, it is not checked in the core fs code or anything), we could
just drop it from the block devices too.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index c29dbcec7c61..46ecbaec8035 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -2299,7 +2299,7 @@  static int mmc_rpmb_chrdev_open(struct inode *inode, struct file *filp)
 	get_device(&rpmb->dev);
 	filp->private_data = rpmb;
 	mutex_lock(&open_lock);
-	rpmb->md->usage++;
+	mmc_blk_get(rpmb->md->disk);
 	mutex_unlock(&open_lock);
 
 	return nonseekable_open(inode, filp);
@@ -2312,7 +2312,7 @@  static int mmc_rpmb_chrdev_release(struct inode *inode, struct file *filp)
 
 	put_device(&rpmb->dev);
 	mutex_lock(&open_lock);
-	rpmb->md->usage--;
+	mmc_blk_put(rpmb->md);
 	mutex_unlock(&open_lock);
 
 	return 0;
@@ -2329,6 +2329,13 @@  static const struct file_operations mmc_rpmb_fileops = {
 #endif
 };
 
+static void mmc_blk_rpmb_device_release(struct device *dev)
+{
+	struct mmc_rpmb_data *rpmb = dev_get_drvdata(dev);
+
+	ida_simple_remove(&mmc_rpmb_ida, rpmb->id);
+	kfree(rpmb);
+}
 
 static int mmc_blk_alloc_rpmb_part(struct mmc_card *card,
 				   struct mmc_blk_data *md,
@@ -2347,8 +2354,10 @@  static int mmc_blk_alloc_rpmb_part(struct mmc_card *card,
 		return devidx;
 
 	rpmb = kzalloc(sizeof(*rpmb), GFP_KERNEL);
-	if (!rpmb)
+	if (!rpmb) {
+		ida_simple_remove(&mmc_rpmb_ida, devidx);
 		return -ENOMEM;
+	}
 
 	snprintf(rpmb_name, sizeof(rpmb_name),
 		 "mmcblk%u%s", card->host->index, subname ? subname : "");
@@ -2359,6 +2368,7 @@  static int mmc_blk_alloc_rpmb_part(struct mmc_card *card,
 	rpmb->dev.bus = &mmc_rpmb_bus_type;
 	rpmb->dev.devt = MKDEV(MAJOR(mmc_rpmb_devt), rpmb->id);
 	rpmb->dev.parent = &card->dev;
+	rpmb->dev.release = mmc_blk_rpmb_device_release;
 	device_initialize(&rpmb->dev);
 	dev_set_drvdata(&rpmb->dev, rpmb);
 	rpmb->md = md;
@@ -2368,7 +2378,7 @@  static int mmc_blk_alloc_rpmb_part(struct mmc_card *card,
 	ret = cdev_device_add(&rpmb->chrdev, &rpmb->dev);
 	if (ret) {
 		pr_err("%s: could not add character device\n", rpmb_name);
-		goto out_remove_ida;
+		goto out_put_device;
 	}
 
 	list_add(&rpmb->node, &md->rpmbs);
@@ -2383,18 +2393,16 @@  static int mmc_blk_alloc_rpmb_part(struct mmc_card *card,
 
 	return 0;
 
-out_remove_ida:
-	ida_simple_remove(&mmc_rpmb_ida, rpmb->id);
-	kfree(rpmb);
+out_put_device:
+	put_device(&rpmb->dev);
 	return ret;
 }
 
 static void mmc_blk_remove_rpmb_part(struct mmc_rpmb_data *rpmb)
+
 {
 	cdev_device_del(&rpmb->chrdev, &rpmb->dev);
-	device_del(&rpmb->dev);
-	ida_simple_remove(&mmc_rpmb_ida, rpmb->id);
-	kfree(rpmb);
+	put_device(&rpmb->dev);
 }
 
 /* MMC Physical partitions consist of two boot partitions and