[PATCH 0/2] Two mmc fixes for IDA and KASAN issues

[PATCH 0/2] Two mmc fixes for IDA and KASAN issues

[Stephen Boyd]

Here's a followup to a thread I sent a couple months ago[1]. They're only
marginally related to each other, but I have bundled them here into one
series to make it easier to track. Resending to restart the discussion.

Cc: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
Cc: Sujit Kautkar <sujitka@chromium.org>
Cc: Zubin Mithra <zsm@chromium.org>

[1] https://lore.kernel.org/r/20210413003621.1403300-1-swboyd@chromium.org

Stephen Boyd (2):
  mmc: block: Use kref in place of struct mmc_blk_data::usage
  mmc: core: Don't allocate IDA for OF aliases

 drivers/mmc/core/block.c | 35 +++++++++++++++++++++--------------
 drivers/mmc/core/host.c  | 20 ++++++++++----------
 2 files changed, 31 insertions(+), 24 deletions(-)


base-commit: d07f6ca923ea0927a1024dfccafc5b53b61cfecc
-- 
https://chromeos.dev

[PATCH 1/2] mmc: block: Use kref in place of struct mmc_blk_data::usage

[Stephen Boyd]

Ulf reported the following KASAN splat after adding some manual hacks
into mmc-utils[1].

 DEBUG: mmc_blk_open: Let's sleep for 10s..
 mmc1: card 0007 removed
 BUG: KASAN: use-after-free in mmc_blk_get+0x58/0xb8
 Read of size 4 at addr ffff00000a394a28 by task mmc/180

 CPU: 2 PID: 180 Comm: mmc Not tainted 5.10.0-rc4-00069-gcc758c8c7127-dirty #5
 Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
 Call trace:
  dump_backtrace+0x0/0x2b4
  show_stack+0x18/0x6c
  dump_stack+0xfc/0x168
  print_address_description.constprop.0+0x6c/0x488
  kasan_report+0x118/0x210
  __asan_load4+0x94/0xd0
  mmc_blk_get+0x58/0xb8
  mmc_blk_open+0x7c/0xdc
  __blkdev_get+0x3b4/0x964
  blkdev_get+0x64/0x100
  blkdev_open+0xe8/0x104
  do_dentry_open+0x234/0x61c
  vfs_open+0x54/0x64
  path_openat+0xe04/0x1584
  do_filp_open+0xe8/0x1e4
  do_sys_openat2+0x120/0x230
  __arm64_sys_openat+0xf0/0x15c
  el0_svc_common.constprop.0+0xac/0x234
  do_el0_svc+0x84/0xa0
  el0_sync_handler+0x264/0x270
  el0_sync+0x174/0x180

 Allocated by task 33:
  stack_trace_save+0x9c/0xdc
  kasan_save_stack+0x28/0x60
  __kasan_kmalloc.constprop.0+0xc8/0xf0
  kasan_kmalloc+0x10/0x20
  mmc_blk_alloc_req+0x94/0x4b0
  mmc_blk_probe+0x2d4/0xaa4
  mmc_bus_probe+0x34/0x4c
  really_probe+0x148/0x6e0
  driver_probe_device+0x78/0xec
  __device_attach_driver+0x108/0x16c
  bus_for_each_drv+0xf4/0x15c
  __device_attach+0x168/0x240
  device_initial_probe+0x14/0x20
  bus_probe_device+0xec/0x100
  device_add+0x55c/0xaf0
  mmc_add_card+0x288/0x380
  mmc_attach_sd+0x18c/0x22c
  mmc_rescan+0x444/0x4f0
  process_one_work+0x3b8/0x650
  worker_thread+0xa0/0x724
  kthread+0x218/0x220
  ret_from_fork+0x10/0x38

 Freed by task 33:
  stack_trace_save+0x9c/0xdc
  kasan_save_stack+0x28/0x60
  kasan_set_track+0x28/0x40
  kasan_set_free_info+0x24/0x4c
  __kasan_slab_free+0x100/0x180
  kasan_slab_free+0x14/0x20
  kfree+0xb8/0x46c
  mmc_blk_put+0xe4/0x11c
  mmc_blk_remove_req.part.0+0x6c/0xe4
  mmc_blk_remove+0x368/0x370
  mmc_bus_remove+0x34/0x50
  __device_release_driver+0x228/0x31c
  device_release_driver+0x2c/0x44
  bus_remove_device+0x1e4/0x200
  device_del+0x2b0/0x770
  mmc_remove_card+0xf0/0x150
  mmc_sd_detect+0x9c/0x150
  mmc_rescan+0x110/0x4f0
  process_one_work+0x3b8/0x650
  worker_thread+0xa0/0x724
  kthread+0x218/0x220
  ret_from_fork+0x10/0x38

 The buggy address belongs to the object at ffff00000a394800
  which belongs to the cache kmalloc-1k of size 1024
 The buggy address is located 552 bytes inside of
  1024-byte region [ffff00000a394800, ffff00000a394c00)
 The buggy address belongs to the page:
 page:00000000ff84ed53 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x8a390
 head:00000000ff84ed53 order:3 compound_mapcount:0 compound_pincount:0
 flags: 0x3fffc0000010200(slab|head)
 raw: 03fffc0000010200 dead000000000100 dead000000000122 ffff000009f03800
 raw: 0000000000000000 0000000000100010 00000001ffffffff 0000000000000000
 page dumped because: kasan: bad access detected

 Memory state around the buggy address:
  ffff00000a394900: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
  ffff00000a394980: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 >ffff00000a394a00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
                                   ^
  ffff00000a394a80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
  ffff00000a394b00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb

Looking closer at the problem, it looks like a classic dangling pointer
bug. The 'struct mmc_blk_data' that is used after being freed in
mmc_blk_put() is stashed away in 'md->disk->private_data' via
mmc_blk_alloc_req() but used in mmc_blk_get() because the 'usage' count
isn't properly aligned with the lifetime of the pointer. You'd expect
the 'usage' member to be in sync with the kfree(), and it mostly is,
except that mmc_blk_get() needs to dereference the potentially freed
memory storage for the 'struct mmc_blk_data' stashed away in the
private_data member to look at 'usage' before it actually figures out if
it wants to consider it a valid pointer or not. That's not going to work
if the freed memory has been overwritten by something else after the
free, and KASAN rightly complains here.

To fix the immediate problem, let's set the private_data member to NULL
in mmc_blk_put() so that mmc_blk_get() can consider the object "on the
way out" if the pointer is NULL and not even try to look at 'usage' if
the object isn't going to be around much longer. With that set to NULL
on the last mmc_blk_put(), optimize the get path further and use a kref
underneath the 'open_lock' mutex to only up the reference count if it's
non-zero, i.e.  alive, and otherwise make mmc_blk_get() return NULL,
without actually testing the reference count if we're in the process of
removing the object from the system.

Finally, tighten the locking region on the put side to only be around
the parts that are removing the 'mmc_blk_data' from the system and
publishing that fact to the gendisk and then drop the lock as soon as we
can to avoid holding the lock around code that doesn't need it. This
fixes the KASAN issue.

Cc: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
Cc: Sujit Kautkar <sujitka@chromium.org>
Cc: Zubin Mithra <zsm@chromium.org>
Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
Link: https://lore.kernel.org/linux-mmc/CAPDyKFryT63Jc7+DXWSpAC19qpZRqFr1orxwYGMuSqx247O8cQ@mail.gmail.com/ [1]
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/mmc/core/block.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 689eb9afeeed..451f12d68ef3 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -27,6 +27,7 @@
 #include <linux/errno.h>
 #include <linux/hdreg.h>
 #include <linux/kdev_t.h>
+#include <linux/kref.h>
 #include <linux/blkdev.h>
 #include <linux/cdev.h>
 #include <linux/mutex.h>
@@ -110,7 +111,7 @@ struct mmc_blk_data {
 #define MMC_BLK_CMD23	(1 << 0)	/* Can do SET_BLOCK_COUNT for multiblock */
 #define MMC_BLK_REL_WR	(1 << 1)	/* MMC Reliable write support */
 
-	unsigned int	usage;
+	struct kref	kref;
 	unsigned int	read_only;
 	unsigned int	part_type;
 	unsigned int	reset_done;
@@ -180,10 +181,8 @@ static struct mmc_blk_data *mmc_blk_get(struct gendisk *disk)
 
 	mutex_lock(&open_lock);
 	md = disk->private_data;
-	if (md && md->usage == 0)
+	if (md && !kref_get_unless_zero(&md->kref))
 		md = NULL;
-	if (md)
-		md->usage++;
 	mutex_unlock(&open_lock);
 
 	return md;
@@ -195,18 +194,26 @@ static inline int mmc_get_devidx(struct gendisk *disk)
 	return devidx;
 }
 
-static void mmc_blk_put(struct mmc_blk_data *md)
+static void mmc_blk_kref_release(struct kref *ref)
 {
+	struct mmc_blk_data *md = container_of(ref, struct mmc_blk_data, kref);
+	int devidx;
+
+	devidx = mmc_get_devidx(md->disk);
+	blk_put_queue(md->queue.queue);
+	ida_simple_remove(&mmc_blk_ida, devidx);
+
 	mutex_lock(&open_lock);
-	md->usage--;
-	if (md->usage == 0) {
-		int devidx = mmc_get_devidx(md->disk);
-		blk_put_queue(md->queue.queue);
-		ida_simple_remove(&mmc_blk_ida, devidx);
-		put_disk(md->disk);
-		kfree(md);
-	}
+	md->disk->private_data = NULL;
 	mutex_unlock(&open_lock);
+
+	put_disk(md->disk);
+	kfree(md);
+}
+
+static void mmc_blk_put(struct mmc_blk_data *md)
+{
+	kref_put(&md->kref, mmc_blk_kref_release);
 }
 
 static ssize_t power_ro_lock_show(struct device *dev,
@@ -2318,7 +2325,7 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
 
 	INIT_LIST_HEAD(&md->part);
 	INIT_LIST_HEAD(&md->rpmbs);
-	md->usage = 1;
+	kref_init(&md->kref);
 
 	ret = mmc_init_queue(&md->queue, card);
 	if (ret)
-- 
https://chromeos.dev

[PATCH 2/2] mmc: core: Don't allocate IDA for OF aliases

[Stephen Boyd]

There's a chance that the IDA allocated in mmc_alloc_host() is not freed
for some time because it's freed as part of a class' release function
(see mmc_host_classdev_release() where the IDA is freed). If another
thread is holding a reference to the class, then only once all balancing
device_put() calls (in turn calling kobject_put()) have been made will
the IDA be released and usable again.

Normally this isn't a problem because the kobject is released before
anything else that may want to use the same number tries to again, but
with CONFIG_DEBUG_KOBJECT_RELEASE=y and OF aliases it becomes pretty
easy to try to allocate an alias from the IDA twice while the first time
it was allocated is still pending a call to ida_simple_remove(). It's
also possible to trigger it by using CONFIG_DEBUG_KOBJECT_RELEASE and
probe defering a driver at boot that calls mmc_alloc_host() before
trying to get resources that may defer likes clks or regulators.

Instead of allocating from the IDA in this scenario, let's just skip it
if we know this is an OF alias. The number is already "claimed" and
devices that aren't using OF aliases won't try to use the claimed
numbers anyway (see mmc_first_nonreserved_index()). This should avoid
any issues with mmc_alloc_host() returning failures from the
ida_simple_get() in the case that we're using an OF alias.

Cc: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
Cc: Sujit Kautkar <sujitka@chromium.org>
Reported-by: Zubin Mithra <zsm@chromium.org>
Fixes: fa2d0aa96941 ("mmc: core: Allow setting slot index via device tree alias")
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/mmc/core/host.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 0b0577990ddc..8375d4381d47 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -75,7 +75,8 @@ static void mmc_host_classdev_release(struct device *dev)
 {
 	struct mmc_host *host = cls_dev_to_mmc_host(dev);
 	wakeup_source_unregister(host->ws);
-	ida_simple_remove(&mmc_host_ida, host->index);
+	if (of_alias_get_id(host->parent->of_node, "mmc") < 0)
+		ida_simple_remove(&mmc_host_ida, host->index);
 	kfree(host);
 }
 
@@ -499,7 +500,7 @@ static int mmc_first_nonreserved_index(void)
  */
 struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
 {
-	int err;
+	int index;
 	struct mmc_host *host;
 	int alias_id, min_idx, max_idx;
 
@@ -512,20 +513,19 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
 
 	alias_id = of_alias_get_id(dev->of_node, "mmc");
 	if (alias_id >= 0) {
-		min_idx = alias_id;
-		max_idx = alias_id + 1;
+		index = alias_id;
 	} else {
 		min_idx = mmc_first_nonreserved_index();
 		max_idx = 0;
-	}
 
-	err = ida_simple_get(&mmc_host_ida, min_idx, max_idx, GFP_KERNEL);
-	if (err < 0) {
-		kfree(host);
-		return NULL;
+		index = ida_simple_get(&mmc_host_ida, min_idx, max_idx, GFP_KERNEL);
+		if (index < 0) {
+			kfree(host);
+			return NULL;
+		}
 	}
 
-	host->index = err;
+	host->index = index;
 
 	dev_set_name(&host->class_dev, "mmc%d", host->index);
 	host->ws = wakeup_source_register(NULL, dev_name(&host->class_dev));
-- 
https://chromeos.dev

Re: [PATCH 0/2] Two mmc fixes for IDA and KASAN issues

[Ulf Hansson]

On Wed, 23 Jun 2021 at 09:50, Stephen Boyd <swboyd@chromium.org> wrote:
>
> Here's a followup to a thread I sent a couple months ago[1]. They're only
> marginally related to each other, but I have bundled them here into one
> series to make it easier to track. Resending to restart the discussion.
>
> Cc: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> Cc: Sujit Kautkar <sujitka@chromium.org>
> Cc: Zubin Mithra <zsm@chromium.org>
>
> [1] https://lore.kernel.org/r/20210413003621.1403300-1-swboyd@chromium.org
>
> Stephen Boyd (2):
>   mmc: block: Use kref in place of struct mmc_blk_data::usage
>   mmc: core: Don't allocate IDA for OF aliases
>
>  drivers/mmc/core/block.c | 35 +++++++++++++++++++++--------------
>  drivers/mmc/core/host.c  | 20 ++++++++++----------
>  2 files changed, 31 insertions(+), 24 deletions(-)
>

My apologies for the delay! The changes look very good to me, thanks
for helping out!

FYI, I did some more thorough analysis of how the block device
reference counting/locking should be managed, from a generic point of
view. Your change in patch1 fixes the immediate problem with the KASAN
splat I reported, but also moves the code in the right direction.

However, there are lots of additional improvements that deserve to be
made (another possible KASAN splat) for this related mmc code. I am
looking into this and will keep you posted. :-)

So, this series is applied for fixes and stable tags added for both
patches. Again, thanks for helping out!

Kind regards
Uffe