Manual unbind of ATA devices causes use-after-free

Manual unbind of ATA devices causes use-after-free

[Taras Kondratiuk]

Hi Heo,

Manual unbind/remove unconditionally invokes devres_release_all which
calls ata_host_release() and frees ata_host/ata_port memory while it is
still being referenced (e.g as a parent of SCSI host).

Is there a reason why ata_host is using derves which is not refcounted?
Does it make sense to add recounting to ata_host?

We have noticed the issue when put_device(parent) in
scsi_host_dev_release() complained about not initialized kobject.

  WARNING: CPU: 3 PID: 247 at lib/kobject.c:690 kobject_put+0x34/0x92()
  kobject: '(null)' (ffff8804040baf18): is not initialized, yet kobject_put() is being called.
  Modules linked in: lxc_wd(O) contdev_generic(O) pca8550(O) uhci_hcd tipc ip6_udp_tunnel udp_tunnel liin(O) lfts(O) ez_np5c(O) quack(O) ngio(O) ds26521(O) ds31408(O) astro(O) epa(O) dev_obj_lib(PO) pdmaif(O) cpp_kipc_mod(O) cpp_pdma_mod(O) cpp_il_mod(O) cpp_intr_mod(O) yoda_drv_mod(O) cpp_hw_drv_mod(O) cpp_drv_mod(O) ich9spi(O) mtdblock mtd_blkdevs oct_drv_mcp(PO) gladden_edac(O) edac_core max3674(O) pmbus_ps(O) pmbus_core(O) seeprom(O) beeprom(O) luna_fpga(O) i2c_2kh_luna(O) i2c_2kh_reset(O) ck420(O) adm1066(O) ltc4215(O) ltc4151(O) pc
  CPU: 3 PID: 247 Comm: kworker/3:2 Tainted: P           O    4.4.76 #1
  Workqueue: events sg_remove_sfp_usercontext
   0000000000000006 ffffffff81294c0e ffff8804247e3c88 0000000000000009
   ffffffff81044b1e ffffffff812966ab ffff8804040baf18 ffff8804247e3ce0
   ffff880403f54300 ffff8804278a92b0 ffffffff81044b7b ffffffff818224af
  Call Trace:
   [<ffffffff81294c0e>] ? dump_stack+0x5e/0x84
   [<ffffffff81044b1e>] ? warn_slowpath_common+0x93/0xa8
   [<ffffffff812966ab>] ? kobject_put+0x34/0x92
   [<ffffffff81044b7b>] ? warn_slowpath_fmt+0x48/0x50
   [<ffffffff8134d618>] ? scsi_host_dev_release+0xe2/0x107
   [<ffffffff812966ab>] ? kobject_put+0x34/0x92
   [<ffffffff8134d631>] ? scsi_host_dev_release+0xfb/0x107
   [<ffffffff8133d0a2>] ? device_release+0x54/0x86
   [<ffffffff812966f3>] ? kobject_put+0x7c/0x92
   [<ffffffff8133d0a2>] ? device_release+0x54/0x86
   [<ffffffff812966f3>] ? kobject_put+0x7c/0x92
   [<ffffffff81056f56>] ? execute_in_process_context+0x20/0x59
   [<ffffffff8133d0a2>] ? device_release+0x54/0x86
   [<ffffffff812966f3>] ? kobject_put+0x7c/0x92
   [<ffffffff813600d5>] ? sg_remove_sfp_usercontext+0xcc/0xef
   [<ffffffff81057706>] ? process_one_work+0x1c4/0x333
   [<ffffffff810582a5>] ? worker_thread+0x264/0x347
   [<ffffffff81058041>] ? rescuer_thread+0x274/0x274
   [<ffffffff8105c028>] ? kthread+0xd0/0xd8
   [<ffffffff8105bf58>] ? kthread_worker_fn+0x129/0x129
   [<ffffffff8152832f>] ? ret_from_fork+0x3f/0x70
   [<ffffffff8105bf58>] ? kthread_worker_fn+0x129/0x129

This happens if freed memory is already zeroed by the next user. Otherwise 
put_device() just silently corrupts memory. KASAN reports issues in
several other places where freed ata_host memory is accessed.

My setup has v4.4 kernel, but the related code seems to be the same in
v4.14.  I'm reproducing the issue by manually removing or unbinding
device. KASAN starts to complain about use-after-free within 10 cycles.

while true;
    do echo 1 > /sys/devices/pci0000\:00/0000\:00\:1c.0/rescan
    sleep 5
    echo 1 > /sys/devices/pci0000\:00/0000\:00\:1c.0/0000\:05\:00.0/remove
    sleep 5
done

Re: Manual unbind of ATA devices causes use-after-free

[Tejun Heo]

Hello,

On Wed, Nov 01, 2017 at 04:24:47PM -0700, Taras Kondratiuk wrote:
> Manual unbind/remove unconditionally invokes devres_release_all which
> calls ata_host_release() and frees ata_host/ata_port memory while it is
> still being referenced (e.g as a parent of SCSI host).
> 
> Is there a reason why ata_host is using derves which is not refcounted?
> Does it make sense to add recounting to ata_host?

Hmm... the removal path is supposed to drain everything synchronously.
What kind of controller is it?

Thanks.

-- 
tejun

Re: Manual unbind of ATA devices causes use-after-free

[Taras Kondratiuk]

Quoting Tejun Heo (2017-11-03 06:19:37)
> Hello,
> 
> On Wed, Nov 01, 2017 at 04:24:47PM -0700, Taras Kondratiuk wrote:
> > Manual unbind/remove unconditionally invokes devres_release_all which
> > calls ata_host_release() and frees ata_host/ata_port memory while it is
> > still being referenced (e.g as a parent of SCSI host).
> > 
> > Is there a reason why ata_host is using derves which is not refcounted?
> > Does it make sense to add recounting to ata_host?
> 
> Hmm... the removal path is supposed to drain everything synchronously.
> What kind of controller is it?

It drains synchronously if scsi_host_put(ap->scsi_host) in
ata_host_release() releases the last scsi_host reference. But when the issue
happens there is one more reference to scsi_host because sg device is
still open. The last reference will be dropped from sg_release.

I forgot to mention that the disk may not be clearly unmounted when I'm
unbinding it, but IMO it shouldn't cause use-after-free in the kernel.
Also even if sg_release() is called before ata_host_release() there is
still no guarantee that the last reference will be dropped, because
sg_release() schedules sg_remove_sfp_usercontext() to do actual release
and the work may not be completed in time.

Driver is AHCI PCI.

Re: Manual unbind of ATA devices causes use-after-free

[Tejun Heo]

Hello,

On Fri, Nov 03, 2017 at 09:32:16AM -0700, Taras Kondratiuk wrote:
> Quoting Tejun Heo (2017-11-03 06:19:37)
> > Hello,
> > 
> > On Wed, Nov 01, 2017 at 04:24:47PM -0700, Taras Kondratiuk wrote:
> > > Manual unbind/remove unconditionally invokes devres_release_all which
> > > calls ata_host_release() and frees ata_host/ata_port memory while it is
> > > still being referenced (e.g as a parent of SCSI host).
> > > 
> > > Is there a reason why ata_host is using derves which is not refcounted?
> > > Does it make sense to add recounting to ata_host?
> > 
> > Hmm... the removal path is supposed to drain everything synchronously.
> > What kind of controller is it?
> 
> It drains synchronously if scsi_host_put(ap->scsi_host) in
> ata_host_release() releases the last scsi_host reference. But when the issue
> happens there is one more reference to scsi_host because sg device is
> still open. The last reference will be dropped from sg_release.

I see.

> I forgot to mention that the disk may not be clearly unmounted when I'm
> unbinding it, but IMO it shouldn't cause use-after-free in the kernel.

Oh, it shouldn't.  It's a bug.

> Also even if sg_release() is called before ata_host_release() there is
> still no guarantee that the last reference will be dropped, because
> sg_release() schedules sg_remove_sfp_usercontext() to do actual release
> and the work may not be completed in time.

Hmmm, we didn't use to put in ata device structs in the kobject tree,
so this wasn't an issue.  This was changed by 9a6d6a2ddabb ("ata: make
ata port as parent device of scsi host") while adding the transport
support.  While doing that, we didn't change the release path to match
it, so the failure.

cc'ing Lin.  Lin, can you take a look at this?

Thanks.

-- 
tejun

Re: Manual unbind of ATA devices causes use-after-free

[Taras Kondratiuk]

Hi Lin,

Quoting Tejun Heo (2017-11-06 07:24:52)
> Hello,
> 
> On Fri, Nov 03, 2017 at 09:32:16AM -0700, Taras Kondratiuk wrote:
> > Also even if sg_release() is called before ata_host_release() there is
> > still no guarantee that the last reference will be dropped, because
> > sg_release() schedules sg_remove_sfp_usercontext() to do actual release
> > and the work may not be completed in time.
> 
> Hmmm, we didn't use to put in ata device structs in the kobject tree,
> so this wasn't an issue.  This was changed by 9a6d6a2ddabb ("ata: make
> ata port as parent device of scsi host") while adding the transport
> support.  While doing that, we didn't change the release path to match
> it, so the failure.
> 
> cc'ing Lin.  Lin, can you take a look at this?

I'm ready to test whenever you have something. If you don't have time to
look at this then can you recommend a proper way to fix it. Is it better
to change device hierarchy back to previous state (revert 9a6d6a2ddabb)
or add reference counting to ata_host?

Re: Manual unbind of ATA devices causes use-after-free

[Tejun Heo]

Hello,

On Mon, Nov 13, 2017 at 12:09:27PM -0800, Taras Kondratiuk wrote:
> > cc'ing Lin.  Lin, can you take a look at this?
> 
> I'm ready to test whenever you have something. If you don't have time to
> look at this then can you recommend a proper way to fix it. Is it better
> to change device hierarchy back to previous state (revert 9a6d6a2ddabb)
> or add reference counting to ata_host?

We'll have to add refcnting to ata_host.

Thanks.

-- 
tejun

[PATCH] libata: add refcounting to ata_host

[Taras Kondratiuk]

After commit 9a6d6a2ddabb ("ata: make ata port as parent device of scsi
host") manual driver unbind/remove causes use-after-free.

Unbind unconditionally invokes devres_release_all() which calls
ata_host_release() and frees ata_host/ata_port memory while it is still
being referenced as a parent of SCSI host. When SCSI host is finally
released scsi_host_dev_release() calls put_device(parent) and accesses
freed ata_port memory.

Add reference counting to make sure that ata_host lives long enough.

Bug report: https://lkml.org/lkml/2017/11/1/945
Fixes: 9a6d6a2ddabb ("ata: make ata port as parent device of scsi host")
Cc: Tejun Heo <tj@kernel.org>
Cc: Lin Ming <minggr@gmail.com>
Cc: linux-ide@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Taras Kondratiuk <takondra@cisco.com>
---

Based on v4.16-rc4.

 drivers/ata/libata-core.c      | 44 ++++++++++++++++++++++++++++++++++--------
 drivers/ata/libata-transport.c |  4 ++++
 drivers/ata/libata.h           |  2 ++
 include/linux/libata.h         |  1 +
 4 files changed, 43 insertions(+), 8 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 3c09122bf038..ee67f4b113c5 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6006,7 +6006,7 @@ struct ata_port *ata_port_alloc(struct ata_host *host)
 	return ap;
 }
 
-static void ata_host_release(struct device *gendev, void *res)
+static void ata_devres_release(struct device *gendev, void *res)
 {
 	struct ata_host *host = dev_get_drvdata(gendev);
 	int i;
@@ -6020,13 +6020,36 @@ static void ata_host_release(struct device *gendev, void *res)
 		if (ap->scsi_host)
 			scsi_host_put(ap->scsi_host);
 
+	}
+
+	dev_set_drvdata(gendev, NULL);
+	ata_host_put(host);
+}
+
+static void ata_host_release(struct kref *kref)
+{
+	struct ata_host *host = container_of(kref, struct ata_host, kref);
+	int i;
+
+	for (i = 0; i < host->n_ports; i++) {
+		struct ata_port *ap = host->ports[i];
+
 		kfree(ap->pmp_link);
 		kfree(ap->slave_link);
 		kfree(ap);
 		host->ports[i] = NULL;
 	}
+	kfree(host);
+}
 
-	dev_set_drvdata(gendev, NULL);
+void ata_host_get(struct ata_host *host)
+{
+	kref_get(&host->kref);
+}
+
+void ata_host_put(struct ata_host *host)
+{
+	kref_put(&host->kref, ata_host_release);
 }
 
 /**
@@ -6054,26 +6077,31 @@ struct ata_host *ata_host_alloc(struct device *dev, int max_ports)
 	struct ata_host *host;
 	size_t sz;
 	int i;
+	void *dr;
 
 	DPRINTK("ENTER\n");
 
-	if (!devres_open_group(dev, NULL, GFP_KERNEL))
-		return NULL;
-
 	/* alloc a container for our list of ATA ports (buses) */
 	sz = sizeof(struct ata_host) + (max_ports + 1) * sizeof(void *);
-	/* alloc a container for our list of ATA ports (buses) */
-	host = devres_alloc(ata_host_release, sz, GFP_KERNEL);
+	host = kzalloc(sz, GFP_KERNEL);
 	if (!host)
+		return NULL;
+
+	if (!devres_open_group(dev, NULL, GFP_KERNEL))
+		return NULL;
+
+	dr = devres_alloc(ata_devres_release, 0, GFP_KERNEL);
+	if (!dr)
 		goto err_out;
 
-	devres_add(dev, host);
+	devres_add(dev, dr);
 	dev_set_drvdata(dev, host);
 
 	spin_lock_init(&host->lock);
 	mutex_init(&host->eh_mutex);
 	host->dev = dev;
 	host->n_ports = max_ports;
+	kref_init(&host->kref);
 
 	/* allocate ports bound to this host */
 	for (i = 0; i < max_ports; i++) {
diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
index 19e6e539a061..a0b0b4d986f2 100644
--- a/drivers/ata/libata-transport.c
+++ b/drivers/ata/libata-transport.c
@@ -224,6 +224,8 @@ static DECLARE_TRANSPORT_CLASS(ata_port_class,
 
 static void ata_tport_release(struct device *dev)
 {
+	struct ata_port *ap = tdev_to_port(dev);
+	ata_host_put(ap->host);
 }
 
 /**
@@ -284,6 +286,7 @@ int ata_tport_add(struct device *parent,
 	dev->type = &ata_port_type;
 
 	dev->parent = parent;
+	ata_host_get(ap->host);
 	dev->release = ata_tport_release;
 	dev_set_name(dev, "ata%d", ap->print_id);
 	transport_setup_device(dev);
@@ -314,6 +317,7 @@ int ata_tport_add(struct device *parent,
  tport_err:
 	transport_destroy_device(dev);
 	put_device(dev);
+	ata_host_put(ap->host);
 	return error;
 }
 
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index f953cb4bb1ba..9e21c49cf6be 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -100,6 +100,8 @@ extern int ata_port_probe(struct ata_port *ap);
 extern void __ata_port_probe(struct ata_port *ap);
 extern unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
 				      u8 page, void *buf, unsigned int sectors);
+extern void ata_host_get(struct ata_host *host);
+extern void ata_host_put(struct ata_host *host);
 
 #define to_ata_port(d) container_of(d, struct ata_port, tdev)
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index ed9826b21c5e..1795fecdea17 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -617,6 +617,7 @@ struct ata_host {
 	void			*private_data;
 	struct ata_port_operations *ops;
 	unsigned long		flags;
+	struct kref		kref;
 
 	struct mutex		eh_mutex;
 	struct task_struct	*eh_owner;
-- 
2.10.3.dirty

Re: [PATCH] libata: add refcounting to ata_host

[Tejun Heo]

On Fri, Mar 09, 2018 at 08:34:41AM +0000, Taras Kondratiuk wrote:
> After commit 9a6d6a2ddabb ("ata: make ata port as parent device of scsi
> host") manual driver unbind/remove causes use-after-free.
> 
> Unbind unconditionally invokes devres_release_all() which calls
> ata_host_release() and frees ata_host/ata_port memory while it is still
> being referenced as a parent of SCSI host. When SCSI host is finally
> released scsi_host_dev_release() calls put_device(parent) and accesses
> freed ata_port memory.
> 
> Add reference counting to make sure that ata_host lives long enough.
> 
> Bug report: https://lkml.org/lkml/2017/11/1/945
> Fixes: 9a6d6a2ddabb ("ata: make ata port as parent device of scsi host")
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Lin Ming <minggr@gmail.com>
> Cc: linux-ide@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Taras Kondratiuk <takondra@cisco.com>

Applied to libata/for-4.17.

Thanks.

-- 
tejun