[PATCH v2 0/2] Fix a use-after-free in ib_umad

[PATCH v2 0/2] Fix a use-after-free in ib_umad

[Bart Van Assche]

Changes compared to version 1 of this patch series:
* Folded the first patch into the second.
* Implemented Yann's suggestion to drop the test of "ret" in the
  non-error path of ib_umad_sm_open().
* Simplified the implementation of ib_umad_open() and ib_umad_sm_open()
  further.

This patch series consists of the following two patches:
0001-IB-umad-Fix-error-handling.patch
0002-IB-umad-Fix-a-use-after-free.patch
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/2] IB/umad: Fix error handling

[Bart Van Assche]

Avoid leaking a kref count in ib_umad_open() if port->ib_dev == NULL
or if nonseekable_open() fails. Avoid leaking a kref count, that
sm_sem is kept down and also that the IB_PORT_SM capability mask is
not cleared in ib_umad_sm_open() if nonseekable_open() fails.
Since container_of() never returns NULL, remove the code that tests
whether container_of() returns NULL. Note: moving the kref_get() call
from the start of ib_umad_*open() to the end is safe since it is the
responsibility of the caller of these functions to ensure that the
cdev pointer remains valid until at least when these functions return.

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: Alex Chiang <achiang-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Cc: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
---
 drivers/infiniband/core/user_mad.c | 58 ++++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 25 deletions(-)

diff --git a/drivers/infiniband/core/user_mad.c b/drivers/infiniband/core/user_mad.c
index f0d588f..2b3dfcc 100644
--- a/drivers/infiniband/core/user_mad.c
+++ b/drivers/infiniband/core/user_mad.c
@@ -780,27 +780,19 @@ static int ib_umad_open(struct inode *inode, struct file *filp)
 {
 	struct ib_umad_port *port;
 	struct ib_umad_file *file;
-	int ret;
+	int ret = -ENXIO;
 
 	port = container_of(inode->i_cdev, struct ib_umad_port, cdev);
-	if (port)
-		kref_get(&port->umad_dev->ref);
-	else
-		return -ENXIO;
 
 	mutex_lock(&port->file_mutex);
 
-	if (!port->ib_dev) {
-		ret = -ENXIO;
+	if (!port->ib_dev)
 		goto out;
-	}
 
+	ret = -ENOMEM;
 	file = kzalloc(sizeof *file, GFP_KERNEL);
-	if (!file) {
-		kref_put(&port->umad_dev->ref, ib_umad_release_dev);
-		ret = -ENOMEM;
+	if (!file)
 		goto out;
-	}
 
 	mutex_init(&file->mutex);
 	spin_lock_init(&file->send_lock);
@@ -815,9 +807,20 @@ static int ib_umad_open(struct inode *inode, struct file *filp)
 
 	ret = nonseekable_open(inode, filp);
 
+	if (ret)
+		goto del;
+
+	kref_get(&port->umad_dev->ref);
+
 out:
 	mutex_unlock(&port->file_mutex);
+
 	return ret;
+
+del:
+	list_del(&file->port_list);
+	kfree(file);
+	goto out;
 }
 
 static int ib_umad_close(struct inode *inode, struct file *filp)
@@ -880,36 +883,41 @@ static int ib_umad_sm_open(struct inode *inode, struct file *filp)
 	int ret;
 
 	port = container_of(inode->i_cdev, struct ib_umad_port, sm_cdev);
-	if (port)
-		kref_get(&port->umad_dev->ref);
-	else
-		return -ENXIO;
 
 	if (filp->f_flags & O_NONBLOCK) {
 		if (down_trylock(&port->sm_sem)) {
 			ret = -EAGAIN;
-			goto fail;
+			goto out;
 		}
 	} else {
 		if (down_interruptible(&port->sm_sem)) {
 			ret = -ERESTARTSYS;
-			goto fail;
+			goto out;
 		}
 	}
 
 	ret = ib_modify_port(port->ib_dev, port->port_num, 0, &props);
-	if (ret) {
-		up(&port->sm_sem);
-		goto fail;
-	}
+	if (ret)
+		goto up_sem;
 
 	filp->private_data = port;
 
-	return nonseekable_open(inode, filp);
+	ret = nonseekable_open(inode, filp);
+	if (ret)
+		goto clr_sm_cap;
 
-fail:
-	kref_put(&port->umad_dev->ref, ib_umad_release_dev);
+	kref_get(&port->umad_dev->ref);
+
+out:
 	return ret;
+
+clr_sm_cap:
+	swap(props.set_port_cap_mask, props.clr_port_cap_mask);
+	ib_modify_port(port->ib_dev, port->port_num, 0, &props);
+
+up_sem:
+	up(&port->sm_sem);
+	goto out;
 }
 
 static int ib_umad_sm_close(struct inode *inode, struct file *filp)
-- 
1.8.4.5

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

[PATCH v2 2/2] IB/umad: Fix a use-after-free

[Bart Van Assche]

Avoid that closing /dev/infiniband/umad<n> or /dev/infiniband/issm<n>
triggers a use-after-free. __fput() in fs/file_table.c invokes
f_op->release() before it invokes cdev_put(). Make sure that the
ib_umad_device structure is freed by the cdev_put() call instead of
f_op->release(). This avoids that changing the port mode from IB into
Ethernet and back to IB followed by restarting opensmd triggers the
following kernel oops:

general protection fault: 0000 [#1] PREEMPT SMP
RIP: 0010:[<ffffffff810cc65c>]  [<ffffffff810cc65c>] module_put+0x2c/0x170
Call Trace:
 [<ffffffff81190f20>] cdev_put+0x20/0x30
 [<ffffffff8118e2ce>] __fput+0x1ae/0x1f0
 [<ffffffff8118e35e>] ____fput+0xe/0x10
 [<ffffffff810723bc>] task_work_run+0xac/0xe0
 [<ffffffff81002a9f>] do_notify_resume+0x9f/0xc0
 [<ffffffff814b8398>] int_signal+0x12/0x17

Reference: https://bugzilla.kernel.org/show_bug.cgi?id=75051
Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: Alex Chiang <achiang-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Cc: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
---
 drivers/infiniband/core/user_mad.c | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/infiniband/core/user_mad.c b/drivers/infiniband/core/user_mad.c
index 2b3dfcc..93d0823 100644
--- a/drivers/infiniband/core/user_mad.c
+++ b/drivers/infiniband/core/user_mad.c
@@ -98,7 +98,7 @@ struct ib_umad_port {
 
 struct ib_umad_device {
 	int                  start_port, end_port;
-	struct kref          ref;
+	struct kobject       kobj;
 	struct ib_umad_port  port[0];
 };
 
@@ -134,14 +134,18 @@ static DECLARE_BITMAP(dev_map, IB_UMAD_MAX_PORTS);
 static void ib_umad_add_one(struct ib_device *device);
 static void ib_umad_remove_one(struct ib_device *device);
 
-static void ib_umad_release_dev(struct kref *ref)
+static void ib_umad_release_dev(struct kobject *kobj)
 {
 	struct ib_umad_device *dev =
-		container_of(ref, struct ib_umad_device, ref);
+		container_of(kobj, struct ib_umad_device, kobj);
 
 	kfree(dev);
 }
 
+static struct kobj_type ib_umad_dev_ktype = {
+	.release = ib_umad_release_dev,
+};
+
 static int hdr_size(struct ib_umad_file *file)
 {
 	return file->use_pkey_index ? sizeof (struct ib_user_mad_hdr) :
@@ -810,7 +814,7 @@ static int ib_umad_open(struct inode *inode, struct file *filp)
 	if (ret)
 		goto del;
 
-	kref_get(&port->umad_dev->ref);
+	kobject_get(&port->umad_dev->kobj);
 
 out:
 	mutex_unlock(&port->file_mutex);
@@ -855,7 +859,7 @@ static int ib_umad_close(struct inode *inode, struct file *filp)
 	mutex_unlock(&file->port->file_mutex);
 
 	kfree(file);
-	kref_put(&dev->ref, ib_umad_release_dev);
+	kobject_put(&dev->kobj);
 
 	return 0;
 }
@@ -906,7 +910,7 @@ static int ib_umad_sm_open(struct inode *inode, struct file *filp)
 	if (ret)
 		goto clr_sm_cap;
 
-	kref_get(&port->umad_dev->ref);
+	kobject_get(&port->umad_dev->kobj);
 
 out:
 	return ret;
@@ -935,7 +939,7 @@ static int ib_umad_sm_close(struct inode *inode, struct file *filp)
 
 	up(&port->sm_sem);
 
-	kref_put(&port->umad_dev->ref, ib_umad_release_dev);
+	kobject_put(&port->umad_dev->kobj);
 
 	return ret;
 }
@@ -1003,6 +1007,7 @@ static int find_overflow_devnum(void)
 }
 
 static int ib_umad_init_port(struct ib_device *device, int port_num,
+			     struct ib_umad_device *umad_dev,
 			     struct ib_umad_port *port)
 {
 	int devnum;
@@ -1068,6 +1073,11 @@ static int ib_umad_init_port(struct ib_device *device, int port_num,
 	if (device_create_file(port->sm_dev, &dev_attr_port))
 		goto err_sm_dev;
 
+	port->cdev.kobj.parent = &umad_dev->kobj;
+	kobject_get(&umad_dev->kobj);
+	port->sm_cdev.kobj.parent = &umad_dev->kobj;
+	kobject_get(&umad_dev->kobj);
+
 	return 0;
 
 err_sm_dev:
@@ -1146,7 +1156,7 @@ static void ib_umad_add_one(struct ib_device *device)
 	if (!umad_dev)
 		return;
 
-	kref_init(&umad_dev->ref);
+	kobject_init(&umad_dev->kobj, &ib_umad_dev_ktype);
 
 	umad_dev->start_port = s;
 	umad_dev->end_port   = e;
@@ -1154,7 +1164,8 @@ static void ib_umad_add_one(struct ib_device *device)
 	for (i = s; i <= e; ++i) {
 		umad_dev->port[i - s].umad_dev = umad_dev;
 
-		if (ib_umad_init_port(device, i, &umad_dev->port[i - s]))
+		if (ib_umad_init_port(device, i, umad_dev,
+				      &umad_dev->port[i - s]))
 			goto err;
 	}
 
@@ -1166,7 +1177,7 @@ err:
 	while (--i >= s)
 		ib_umad_kill_port(&umad_dev->port[i - s]);
 
-	kref_put(&umad_dev->ref, ib_umad_release_dev);
+	kobject_put(&umad_dev->kobj);
 }
 
 static void ib_umad_remove_one(struct ib_device *device)
@@ -1180,7 +1191,7 @@ static void ib_umad_remove_one(struct ib_device *device)
 	for (i = 0; i <= umad_dev->end_port - umad_dev->start_port; ++i)
 		ib_umad_kill_port(&umad_dev->port[i]);
 
-	kref_put(&umad_dev->ref, ib_umad_release_dev);
+	kobject_put(&umad_dev->kobj);
 }
 
 static char *umad_devnode(struct device *dev, umode_t *mode)
-- 
1.8.4.5

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

Re: [PATCH v2 2/2] IB/umad: Fix a use-after-free

[Yann Droneaud]

Hi,

Le mardi 13 mai 2014 à 12:31 +0200, Bart Van Assche a écrit :
> Avoid that closing /dev/infiniband/umad<n> or /dev/infiniband/issm<n>
> triggers a use-after-free. __fput() in fs/file_table.c invokes
> f_op->release() before it invokes cdev_put(). Make sure that the
> ib_umad_device structure is freed by the cdev_put() call instead of
> f_op->release(). This avoids that changing the port mode from IB into
> Ethernet and back to IB followed by restarting opensmd triggers the
> following kernel oops:
> 
> general protection fault: 0000 [#1] PREEMPT SMP
> RIP: 0010:[<ffffffff810cc65c>]  [<ffffffff810cc65c>] module_put+0x2c/0x170
> Call Trace:
>  [<ffffffff81190f20>] cdev_put+0x20/0x30
>  [<ffffffff8118e2ce>] __fput+0x1ae/0x1f0
>  [<ffffffff8118e35e>] ____fput+0xe/0x10
>  [<ffffffff810723bc>] task_work_run+0xac/0xe0
>  [<ffffffff81002a9f>] do_notify_resume+0x9f/0xc0
>  [<ffffffff814b8398>] int_signal+0x12/0x17
> 
> Reference: https://bugzilla.kernel.org/show_bug.cgi?id=75051
> Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
> Cc: Alex Chiang <achiang-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> Cc: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> ---
>  drivers/infiniband/core/user_mad.c | 33 ++++++++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/infiniband/core/user_mad.c b/drivers/infiniband/core/user_mad.c
> index 2b3dfcc..93d0823 100644
> --- a/drivers/infiniband/core/user_mad.c
> +++ b/drivers/infiniband/core/user_mad.c
> @@ -98,7 +98,7 @@ struct ib_umad_port {
>  
>  struct ib_umad_device {
>  	int                  start_port, end_port;
> -	struct kref          ref;
> +	struct kobject       kobj;
>  	struct ib_umad_port  port[0];
>  };
>  
> @@ -134,14 +134,18 @@ static DECLARE_BITMAP(dev_map, IB_UMAD_MAX_PORTS);
>  static void ib_umad_add_one(struct ib_device *device);
>  static void ib_umad_remove_one(struct ib_device *device);
>  
> -static void ib_umad_release_dev(struct kref *ref)
> +static void ib_umad_release_dev(struct kobject *kobj)
>  {
>  	struct ib_umad_device *dev =
> -		container_of(ref, struct ib_umad_device, ref);
> +		container_of(kobj, struct ib_umad_device, kobj);
>  
>  	kfree(dev);
>  }
>  
> +static struct kobj_type ib_umad_dev_ktype = {
> +	.release = ib_umad_release_dev,
> +};
> +
>  static int hdr_size(struct ib_umad_file *file)
>  {
>  	return file->use_pkey_index ? sizeof (struct ib_user_mad_hdr) :
> @@ -810,7 +814,7 @@ static int ib_umad_open(struct inode *inode, struct file *filp)
>  	if (ret)
>  		goto del;
>  
> -	kref_get(&port->umad_dev->ref);
> +	kobject_get(&port->umad_dev->kobj);
>  
>  out:
>  	mutex_unlock(&port->file_mutex);
> @@ -855,7 +859,7 @@ static int ib_umad_close(struct inode *inode, struct file *filp)
>  	mutex_unlock(&file->port->file_mutex);
>  
>  	kfree(file);
> -	kref_put(&dev->ref, ib_umad_release_dev);
> +	kobject_put(&dev->kobj);
>  
>  	return 0;
>  }
> @@ -906,7 +910,7 @@ static int ib_umad_sm_open(struct inode *inode, struct file *filp)
>  	if (ret)
>  		goto clr_sm_cap;
>  
> -	kref_get(&port->umad_dev->ref);
> +	kobject_get(&port->umad_dev->kobj);
>  
>  out:
>  	return ret;
> @@ -935,7 +939,7 @@ static int ib_umad_sm_close(struct inode *inode, struct file *filp)
>  
>  	up(&port->sm_sem);
>  
> -	kref_put(&port->umad_dev->ref, ib_umad_release_dev);
> +	kobject_put(&port->umad_dev->kobj);
>  
>  	return ret;
>  }
> @@ -1003,6 +1007,7 @@ static int find_overflow_devnum(void)
>  }
>  
>  static int ib_umad_init_port(struct ib_device *device, int port_num,
> +			     struct ib_umad_device *umad_dev,
>  			     struct ib_umad_port *port)
>  {
>  	int devnum;
> @@ -1068,6 +1073,11 @@ static int ib_umad_init_port(struct ib_device *device, int port_num,
>  	if (device_create_file(port->sm_dev, &dev_attr_port))
>  		goto err_sm_dev;
>  
> +	port->cdev.kobj.parent = &umad_dev->kobj;
> +	kobject_get(&umad_dev->kobj);

That could be written as

+	port->cdev.kobj.parent = kobject_get(&umad_dev->kobj);

And it seems to me it's semantically better to get the reference before
storing it.

> +	port->sm_cdev.kobj.parent = &umad_dev->kobj;
> +	kobject_get(&umad_dev->kobj);
> +

Same here:

+	port->sm_cdev.kobj.parent = kobject_get(&umad_dev->kobj);

But I think it could be written even more simpler:
as part of cdev_add(), kobject_get() is already done on
cdev->kobj.parent:

fs/char_dev.c:474

  int cdev_add(struct cdev *p, dev_t dev, unsigned count)
  {
          int error;

          p->dev = dev;
          p->count = count;

          error = kobj_map(cdev_map, dev, count, NULL,
                         exact_match, exact_lock, p);
          if (error)
                  return error;

          kobject_get(p->kobj.parent);

          return 0;
  }

So setting cdev->kobj.parent before calling cdev_add() should be enough.

Since there's no related kobject_put() in ib_umad_kill_port(), it seems
you already make use of cdev_put() calling kobject_put() on cdev kobj
parent. The same should be done for ib_umad_init_port().

>  	return 0;
>  
>  err_sm_dev:
> @@ -1146,7 +1156,7 @@ static void ib_umad_add_one(struct ib_device *device)
>  	if (!umad_dev)
>  		return;
>  
> -	kref_init(&umad_dev->ref);
> +	kobject_init(&umad_dev->kobj, &ib_umad_dev_ktype);
>  
>  	umad_dev->start_port = s;
>  	umad_dev->end_port   = e;
> @@ -1154,7 +1164,8 @@ static void ib_umad_add_one(struct ib_device *device)
>  	for (i = s; i <= e; ++i) {
>  		umad_dev->port[i - s].umad_dev = umad_dev;
>  
> -		if (ib_umad_init_port(device, i, &umad_dev->port[i - s]))
> +		if (ib_umad_init_port(device, i, umad_dev,
> +				      &umad_dev->port[i - s]))
>  			goto err;
>  	}
>  
> @@ -1166,7 +1177,7 @@ err:
>  	while (--i >= s)
>  		ib_umad_kill_port(&umad_dev->port[i - s]);
>  
> -	kref_put(&umad_dev->ref, ib_umad_release_dev);
> +	kobject_put(&umad_dev->kobj);
>  }
>  
>  static void ib_umad_remove_one(struct ib_device *device)
> @@ -1180,7 +1191,7 @@ static void ib_umad_remove_one(struct ib_device *device)
>  	for (i = 0; i <= umad_dev->end_port - umad_dev->start_port; ++i)
>  		ib_umad_kill_port(&umad_dev->port[i]);
>  
> -	kref_put(&umad_dev->ref, ib_umad_release_dev);
> +	kobject_put(&umad_dev->kobj);
>  }
>  
>  static char *umad_devnode(struct device *dev, umode_t *mode)

Regards.

-- 
Yann Droneaud
OPTEYA


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