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

[Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>]

Le vendredi 16 mai 2014 à 13:05 +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 | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/infiniband/core/user_mad.c b/drivers/infiniband/core/user_mad.c
> index 2b3dfcc..4ac0d42 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;
> @@ -1035,6 +1040,7 @@ static int ib_umad_init_port(struct ib_device *device, int port_num,
>  
>  	cdev_init(&port->cdev, &umad_fops);
>  	port->cdev.owner = THIS_MODULE;
> +	port->cdev.kobj.parent = &umad_dev->kobj;
>  	kobject_set_name(&port->cdev.kobj, "umad%d", port->dev_num);
>  	if (cdev_add(&port->cdev, base, 1))
>  		goto err_cdev;
> @@ -1053,6 +1059,7 @@ static int ib_umad_init_port(struct ib_device *device, int port_num,
>  	base += IB_UMAD_MAX_PORTS;
>  	cdev_init(&port->sm_cdev, &umad_sm_fops);
>  	port->sm_cdev.owner = THIS_MODULE;
> +	port->sm_cdev.kobj.parent = &umad_dev->kobj;
>  	kobject_set_name(&port->sm_cdev.kobj, "issm%d", port->dev_num);
>  	if (cdev_add(&port->sm_cdev, base, 1))
>  		goto err_sm_cdev;
> @@ -1146,7 +1153,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 +1161,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 +1174,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 +1188,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)

There's now a equal number of kobject_init() + kobject_get() and
kobject_put() (not counting one in the error path), so this sound well
balanced.

Regards.

Reviewed-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>

-- 
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