浏览代码

libata: add refcounting to ata_host

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>
Signed-off-by: Tejun Heo <tj@kernel.org>
Taras Kondratiuk 7 年之前
父节点
当前提交
2623c7a5f2
共有 4 个文件被更改,包括 43 次插入8 次删除
  1. 36 8
      drivers/ata/libata-core.c
  2. 4 0
      drivers/ata/libata-transport.c
  3. 2 0
      drivers/ata/libata.h
  4. 1 0
      include/linux/libata.h

+ 36 - 8
drivers/ata/libata-core.c

@@ -6005,7 +6005,7 @@ struct ata_port *ata_port_alloc(struct ata_host *host)
 	return ap;
 	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);
 	struct ata_host *host = dev_get_drvdata(gendev);
 	int i;
 	int i;
@@ -6019,13 +6019,36 @@ static void ata_host_release(struct device *gendev, void *res)
 		if (ap->scsi_host)
 		if (ap->scsi_host)
 			scsi_host_put(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->pmp_link);
 		kfree(ap->slave_link);
 		kfree(ap->slave_link);
 		kfree(ap);
 		kfree(ap);
 		host->ports[i] = NULL;
 		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);
 }
 }
 
 
 /**
 /**
@@ -6053,26 +6076,31 @@ struct ata_host *ata_host_alloc(struct device *dev, int max_ports)
 	struct ata_host *host;
 	struct ata_host *host;
 	size_t sz;
 	size_t sz;
 	int i;
 	int i;
+	void *dr;
 
 
 	DPRINTK("ENTER\n");
 	DPRINTK("ENTER\n");
 
 
-	if (!devres_open_group(dev, NULL, GFP_KERNEL))
-		return NULL;
-
 	/* alloc a container for our list of ATA ports (buses) */
 	/* alloc a container for our list of ATA ports (buses) */
 	sz = sizeof(struct ata_host) + (max_ports + 1) * sizeof(void *);
 	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)
 	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;
 		goto err_out;
 
 
-	devres_add(dev, host);
+	devres_add(dev, dr);
 	dev_set_drvdata(dev, host);
 	dev_set_drvdata(dev, host);
 
 
 	spin_lock_init(&host->lock);
 	spin_lock_init(&host->lock);
 	mutex_init(&host->eh_mutex);
 	mutex_init(&host->eh_mutex);
 	host->dev = dev;
 	host->dev = dev;
 	host->n_ports = max_ports;
 	host->n_ports = max_ports;
+	kref_init(&host->kref);
 
 
 	/* allocate ports bound to this host */
 	/* allocate ports bound to this host */
 	for (i = 0; i < max_ports; i++) {
 	for (i = 0; i < max_ports; i++) {

+ 4 - 0
drivers/ata/libata-transport.c

@@ -224,6 +224,8 @@ static DECLARE_TRANSPORT_CLASS(ata_port_class,
 
 
 static void ata_tport_release(struct device *dev)
 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->type = &ata_port_type;
 
 
 	dev->parent = parent;
 	dev->parent = parent;
+	ata_host_get(ap->host);
 	dev->release = ata_tport_release;
 	dev->release = ata_tport_release;
 	dev_set_name(dev, "ata%d", ap->print_id);
 	dev_set_name(dev, "ata%d", ap->print_id);
 	transport_setup_device(dev);
 	transport_setup_device(dev);
@@ -314,6 +317,7 @@ int ata_tport_add(struct device *parent,
  tport_err:
  tport_err:
 	transport_destroy_device(dev);
 	transport_destroy_device(dev);
 	put_device(dev);
 	put_device(dev);
+	ata_host_put(ap->host);
 	return error;
 	return error;
 }
 }
 
 

+ 2 - 0
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 void __ata_port_probe(struct ata_port *ap);
 extern unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
 extern unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
 				      u8 page, void *buf, unsigned int sectors);
 				      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)
 #define to_ata_port(d) container_of(d, struct ata_port, tdev)
 
 

+ 1 - 0
include/linux/libata.h

@@ -617,6 +617,7 @@ struct ata_host {
 	void			*private_data;
 	void			*private_data;
 	struct ata_port_operations *ops;
 	struct ata_port_operations *ops;
 	unsigned long		flags;
 	unsigned long		flags;
+	struct kref		kref;
 
 
 	struct mutex		eh_mutex;
 	struct mutex		eh_mutex;
 	struct task_struct	*eh_owner;
 	struct task_struct	*eh_owner;