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

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

[Bart Van Assche]

This patch series consists of the following three patches:

0001-IB-umad-Remove-container_of-NULL-tests.patch
0002-IB-umad-Fix-error-handling.patch
0003-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 1/3] IB/umad: Remove container_of() != NULL tests

[Bart Van Assche]

container_of() never returns NULL. Hence remove the code that tests
whether container_of() returns NULL.

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: Alex Chiang <achiang-VXdhtT5mjnY@public.gmane.org>
Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
---
 drivers/infiniband/core/user_mad.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/infiniband/core/user_mad.c b/drivers/infiniband/core/user_mad.c
index f0d588f..e61287c 100644
--- a/drivers/infiniband/core/user_mad.c
+++ b/drivers/infiniband/core/user_mad.c
@@ -783,10 +783,7 @@ static int ib_umad_open(struct inode *inode, struct file *filp)
 	int ret;
 
 	port = container_of(inode->i_cdev, struct ib_umad_port, cdev);
-	if (port)
-		kref_get(&port->umad_dev->ref);
-	else
-		return -ENXIO;
+	kref_get(&port->umad_dev->ref);
 
 	mutex_lock(&port->file_mutex);
 
@@ -880,10 +877,7 @@ 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;
+	kref_get(&port->umad_dev->ref);
 
 	if (filp->f_flags & O_NONBLOCK) {
 		if (down_trylock(&port->sm_sem)) {
-- 
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 2/3] 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.

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
---
 drivers/infiniband/core/user_mad.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/drivers/infiniband/core/user_mad.c b/drivers/infiniband/core/user_mad.c
index e61287c..5c67d80 100644
--- a/drivers/infiniband/core/user_mad.c
+++ b/drivers/infiniband/core/user_mad.c
@@ -780,24 +780,20 @@ 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);
 	kref_get(&port->umad_dev->ref);
 
 	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);
@@ -814,6 +810,10 @@ static int ib_umad_open(struct inode *inode, struct file *filp)
 
 out:
 	mutex_unlock(&port->file_mutex);
+
+	if (ret)
+		kref_put(&port->umad_dev->ref, ib_umad_release_dev);
+
 	return ret;
 }
 
@@ -892,18 +892,27 @@ static int ib_umad_sm_open(struct inode *inode, struct file *filp)
 	}
 
 	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);
+	if (ret)
+		kref_put(&port->umad_dev->ref, ib_umad_release_dev);
 	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 fail;
 }
 
 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 3/3] 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: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
---
 drivers/infiniband/core/user_mad.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/drivers/infiniband/core/user_mad.c b/drivers/infiniband/core/user_mad.c
index 5c67d80..7bec4ca 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) :
@@ -783,7 +787,7 @@ static int ib_umad_open(struct inode *inode, struct file *filp)
 	int ret = -ENXIO;
 
 	port = container_of(inode->i_cdev, struct ib_umad_port, cdev);
-	kref_get(&port->umad_dev->ref);
+	kobject_get(&port->umad_dev->kobj);
 
 	mutex_lock(&port->file_mutex);
 
@@ -812,7 +816,7 @@ out:
 	mutex_unlock(&port->file_mutex);
 
 	if (ret)
-		kref_put(&port->umad_dev->ref, ib_umad_release_dev);
+		kobject_put(&port->umad_dev->kobj);
 
 	return ret;
 }
@@ -849,7 +853,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;
 }
@@ -877,7 +881,7 @@ 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);
-	kref_get(&port->umad_dev->ref);
+	kobject_get(&port->umad_dev->kobj);
 
 	if (filp->f_flags & O_NONBLOCK) {
 		if (down_trylock(&port->sm_sem)) {
@@ -903,7 +907,7 @@ static int ib_umad_sm_open(struct inode *inode, struct file *filp)
 
 fail:
 	if (ret)
-		kref_put(&port->umad_dev->ref, ib_umad_release_dev);
+		kobject_put(&port->umad_dev->kobj);
 	return ret;
 
 clr_sm_cap:
@@ -930,7 +934,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;
 }
@@ -998,6 +1002,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;
@@ -1063,6 +1068,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:
@@ -1141,7 +1151,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;
@@ -1149,7 +1159,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;
 	}
 
@@ -1161,7 +1172,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)
@@ -1175,7 +1186,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

[PATCH 1/3] IB/umad: Remove container_of() != NULL tests

[Bart Van Assche]

container_of() never returns NULL. Hence remove the code that tests
whether container_of() returns NULL.

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: Alex Chiang <achiang-VXdhtT5mjnY@public.gmane.org>
Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
---
 drivers/infiniband/core/user_mad.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/infiniband/core/user_mad.c b/drivers/infiniband/core/user_mad.c
index f0d588f..e61287c 100644
--- a/drivers/infiniband/core/user_mad.c
+++ b/drivers/infiniband/core/user_mad.c
@@ -783,10 +783,7 @@ static int ib_umad_open(struct inode *inode, struct file *filp)
 	int ret;
  	port = container_of(inode->i_cdev, struct ib_umad_port, cdev);
-	if (port)
-		kref_get(&port->umad_dev->ref);
-	else
-		return -ENXIO;
+	kref_get(&port->umad_dev->ref);
  	mutex_lock(&port->file_mutex);
 @@ -880,10 +877,7 @@ 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;
+	kref_get(&port->umad_dev->ref);
  	if (filp->f_flags & O_NONBLOCK) {
 		if (down_trylock(&port->sm_sem)) {
-- 
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 1/3] IB/umad: Remove container_of() != NULL tests

[Yann Droneaud]

Hi,

Le lundi 12 mai 2014 à 10:30 +0200, Bart Van Assche a écrit :
> container_of() never returns NULL. Hence remove the code that tests
> whether container_of() returns NULL.
> 
> Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
> Cc: Alex Chiang <achiang-VXdhtT5mjnY@public.gmane.org>
> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>

The patch seems fine.

But I don't think this one qualify for linux-stable.

> ---
>  drivers/infiniband/core/user_mad.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/infiniband/core/user_mad.c b/drivers/infiniband/core/user_mad.c
> index f0d588f..e61287c 100644
> --- a/drivers/infiniband/core/user_mad.c
> +++ b/drivers/infiniband/core/user_mad.c
> @@ -783,10 +783,7 @@ static int ib_umad_open(struct inode *inode, struct file *filp)
>  	int ret;
>  
>  	port = container_of(inode->i_cdev, struct ib_umad_port, cdev);
> -	if (port)
> -		kref_get(&port->umad_dev->ref);
> -	else
> -		return -ENXIO;
> +	kref_get(&port->umad_dev->ref);
>  
>  	mutex_lock(&port->file_mutex);
>  
> @@ -880,10 +877,7 @@ 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;
> +	kref_get(&port->umad_dev->ref);
>  
>  	if (filp->f_flags & O_NONBLOCK) {
>  		if (down_trylock(&port->sm_sem)) {

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

Re: [PATCH 2/3] IB/umad: Fix error handling

[Yann Droneaud]

Hi,

Le lundi 12 mai 2014 à 10:30 +0200, Bart Van Assche a écrit :
> 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.
> 
> Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> ---
>  drivers/infiniband/core/user_mad.c | 37 +++++++++++++++++++++++--------------
>  1 file changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/infiniband/core/user_mad.c b/drivers/infiniband/core/user_mad.c
> index e61287c..5c67d80 100644
> --- a/drivers/infiniband/core/user_mad.c
> +++ b/drivers/infiniband/core/user_mad.c
> @@ -780,24 +780,20 @@ 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;
>  

I don't like the way ret is gratuitously set,

>  	port = container_of(inode->i_cdev, struct ib_umad_port, cdev);
>  	kref_get(&port->umad_dev->ref);
>  
>  	mutex_lock(&port->file_mutex);
>  
> -	if (!port->ib_dev) {
> -		ret = -ENXIO;
> +	if (!port->ib_dev)
>  		goto out;
> -	}
>  
> +	ret = -ENOMEM;

especially here: I think it should be moved in the error handling path:

>  	file = kzalloc(sizeof *file, GFP_KERNEL);
> -	if (!file) {
> -		kref_put(&port->umad_dev->ref, ib_umad_release_dev);
> -		ret = -ENOMEM;

keep it here.

> +	if (!file)
>  		goto out;
> -	}
>  
>  	mutex_init(&file->mutex);
>  	spin_lock_init(&file->send_lock);
> @@ -814,6 +810,10 @@ static int ib_umad_open(struct inode *inode, struct file *filp)
>  
>  out:
>  	mutex_unlock(&port->file_mutex);
> +
> +	if (ret)
> +		kref_put(&port->umad_dev->ref, ib_umad_release_dev);
> +
>  	return ret;
>  }
>  
> @@ -892,18 +892,27 @@ static int ib_umad_sm_open(struct inode *inode, struct file *filp)
>  	}
>  
>  	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);
> +	if (ret)
> +		kref_put(&port->umad_dev->ref, ib_umad_release_dev);
>  	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 fail;

I dislike jump backward, why not unconditionally call kref_put() in the
error path and return ret here.

This way you could drop fail label and the test on ret in the default
code path.

>  }
>  
>  static int ib_umad_sm_close(struct inode *inode, struct file *filp)

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

Re: [PATCH 2/3] IB/umad: Fix error handling

[Bart Van Assche]

On 05/12/14 12:18, Yann Droneaud wrote:
> Le lundi 12 mai 2014 à 10:30 +0200, Bart Van Assche a écrit :
>>  	port = container_of(inode->i_cdev, struct ib_umad_port, cdev);
>>  	kref_get(&port->umad_dev->ref);
>>  
>>  	mutex_lock(&port->file_mutex);
>>  
>> -	if (!port->ib_dev) {
>> -		ret = -ENXIO;
>> +	if (!port->ib_dev)
>>  		goto out;
>> -	}
>>  
>> +	ret = -ENOMEM;
> especially here: I think it should be moved in the error handling path:
>
>>  	file = kzalloc(sizeof *file, GFP_KERNEL);
>> -	if (!file) {
>> -		kref_put(&port->umad_dev->ref, ib_umad_release_dev);
>> -		ret = -ENOMEM;
> keep it here.

Does this mean that you are not aware that setting the return code
before an if-test is a common coding style in the Linux kernel ? See
e.g. kernel/futex.c or kernel/events/core.c for other examples.

Bart.
--
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 1/3] IB/umad: Remove container_of() != NULL tests

[Bart Van Assche]

On 05/12/14 12:04, Yann Droneaud wrote:
> Le lundi 12 mai 2014 à 10:30 +0200, Bart Van Assche a écrit :
>> container_of() never returns NULL. Hence remove the code that tests
>> whether container_of() returns NULL.
>>
>> Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
>> Cc: Alex Chiang <achiang-VXdhtT5mjnY@public.gmane.org>
>> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> The patch seems fine.
>
> But I don't think this one qualify for linux-stable.

One chunk in patch 3/3 touches code that is also touched by this patch.
This means that if only patches 2/3 and 3/3 would be sent to stable that
these wouldn't apply properly. Hence the "Cc: stable" tag in this patch.

Bart.

--
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 2/3] IB/umad: Fix error handling

[Yann Droneaud]

Le lundi 12 mai 2014 à 12:32 +0200, Bart Van Assche a écrit :
> On 05/12/14 12:18, Yann Droneaud wrote:
> > Le lundi 12 mai 2014 à 10:30 +0200, Bart Van Assche a écrit :
> >>  	port = container_of(inode->i_cdev, struct ib_umad_port, cdev);
> >>  	kref_get(&port->umad_dev->ref);
> >>  
> >>  	mutex_lock(&port->file_mutex);
> >>  
> >> -	if (!port->ib_dev) {
> >> -		ret = -ENXIO;
> >> +	if (!port->ib_dev)
> >>  		goto out;
> >> -	}
> >>  
> >> +	ret = -ENOMEM;
> > especially here: I think it should be moved in the error handling path:
> >
> >>  	file = kzalloc(sizeof *file, GFP_KERNEL);
> >> -	if (!file) {
> >> -		kref_put(&port->umad_dev->ref, ib_umad_release_dev);
> >> -		ret = -ENOMEM;
> > keep it here.
> 
> Does this mean that you are not aware that setting the return code
> before an if-test is a common coding style in the Linux kernel ? See
> e.g. kernel/futex.c or kernel/events/core.c for other examples.
> 

Perhaps, but it's nowhere else in user_mad.c

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

Re: [PATCH 1/3] IB/umad: Remove container_of() != NULL tests

[Yann Droneaud]

Le lundi 12 mai 2014 à 12:36 +0200, Bart Van Assche a écrit :
> On 05/12/14 12:04, Yann Droneaud wrote:
> > Le lundi 12 mai 2014 à 10:30 +0200, Bart Van Assche a écrit :
> >> container_of() never returns NULL. Hence remove the code that tests
> >> whether container_of() returns NULL.
> >>
> >> Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
> >> Cc: Alex Chiang <achiang-VXdhtT5mjnY@public.gmane.org>
> >> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> > The patch seems fine.
> >
> > But I don't think this one qualify for linux-stable.
> 
> One chunk in patch 3/3 touches code that is also touched by this patch.
> This means that if only patches 2/3 and 3/3 would be sent to stable that
> these wouldn't apply properly. Hence the "Cc: stable" tag in this patch.
> 

You could re-order the patches to make this one apply on top of the two
others.

Anyway, it's a minor issue that doesn't justifiy by itself to respin the
patchset. But if you're going to take account of my others remarks, it
might be worth to reorder the patchset.

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

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

[Bart Van Assche]

Changes compared to version 2 of this patch series:
* Converted explicit kobject() get calls into implicit calls by moving
  the kobj.parent assignments in front of the corresponding cdev_add()
  calls.

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 v3 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 v3 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 | 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)
-- 
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 v3 2/2] IB/umad: Fix a use-after-free

[Yann Droneaud]

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

[PATCH v3.1] IB/umad: Fix error handling

[Yann Droneaud]

From: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>

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>
[ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org: rework a bit to reduce the amount of code changed]
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>

---
Hi Bart,

Please find a slightly modified version of your patch to simplify
a bit the error paths (no backward goto's) and to reduces the amount
of lines touched.

I wasn't able to explain it clearly enough in the previous patch review,
so I've made the changes directly in the file and propose the modified
patch for you to review.

As it's only suggestion, feel free to integrate the changes in your
patch or discard them.

Regards.

 drivers/infiniband/core/user_mad.c | 50 +++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 22 deletions(-)

diff --git a/drivers/infiniband/core/user_mad.c b/drivers/infiniband/core/user_mad.c
index f0d588f8859e..055893b870ee 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);
@@ -814,9 +806,16 @@ static int ib_umad_open(struct inode *inode, struct file *filp)
 	list_add_tail(&file->port_list, &port->file_list);
 
 	ret = nonseekable_open(inode, filp);
+	if (ret) {
+		list_del(&file->port_list);
+		kfree(file);
+		goto out;
+	}
+
+	kref_get(&port->umad_dev->ref);
 
 out:
 	mutex_unlock(&port->file_mutex);
 	return ret;
 }
 
@@ -880,10 +880,6 @@ 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)) {
@@ -898,17 +894,27 @@ static int ib_umad_sm_open(struct inode *inode, struct file *filp)
 	}
 
 	ret = ib_modify_port(port->ib_dev, port->port_num, 0, &props);
-	if (ret) {
-		up(&port->sm_sem);
-		goto fail;
-	}
+	if (ret)
+		goto err_up_sem;
 
 	filp->private_data = port;
 
-	return nonseekable_open(inode, filp);
+	ret = nonseekable_open(inode, filp);
+	if (ret)
+		goto err_clr_sm_cap;
+
+	kref_get(&port->umad_dev->ref);
+
+	return 0;
+
+err_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);
+
+err_up_sem:
+	up(&port->sm_sem);
 
 fail:
-	kref_put(&port->umad_dev->ref, ib_umad_release_dev);
 	return ret;
 }
 
-- 
1.9.0

--
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 v3.1] IB/umad: Fix error handling

[Bart Van Assche]

On 05/20/14 10:33, Yann Droneaud wrote:
> Please find a slightly modified version of your patch to simplify
> a bit the error paths (no backward goto's) and to reduces the amount
> of lines touched.
> 
> I wasn't able to explain it clearly enough in the previous patch review,
> so I've made the changes directly in the file and propose the modified
> patch for you to review.
> 
> As it's only suggestion, feel free to integrate the changes in your
> patch or discard them.

Hello Yann,

It seems like our opinions about backward goto's are different :-) I
thought these are common at the end of error handling code in the Linux
kernel. Anyway, what matters to me is that a fix gets upstream, not
which fix. But please do not expect me to spend more time testing a
patch that has been reworked only because of the coding style aspects
mentioned in your e-mail.

Bart.

--
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 v3.1] IB/umad: Fix error handling

[Yann Droneaud]

Hi,

Le mardi 20 mai 2014 à 13:25 +0200, Bart Van Assche a écrit :
> On 05/20/14 10:33, Yann Droneaud wrote:
> > Please find a slightly modified version of your patch to simplify
> > a bit the error paths (no backward goto's) and to reduces the amount
> > of lines touched.
> > 
> > I wasn't able to explain it clearly enough in the previous patch review,
> > so I've made the changes directly in the file and propose the modified
> > patch for you to review.
> > 
> > As it's only suggestion, feel free to integrate the changes in your
> > patch or discard them.
> 
> Hello Yann,
> 
> It seems like our opinions about backward goto's are different :-) I
> thought these are common at the end of error handling code in the Linux
> kernel. Anyway, what matters to me is that a fix gets upstream, not
> which fix. But please do not expect me to spend more time testing a
> patch that has been reworked only because of the coding style aspects
> mentioned in your e-mail.
> 

Having a shorter diff might help to get the patch applied upstream.
YMMV.

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

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

[Yann Droneaud]

From: Bart Van Assche <bvanassche@acm.org>

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@acm.org>
Reviewed-by: Yann Droneaud <ydroneaud@opteya.com>
Cc: Alex Chiang <achiang@canonical.com>
Cc: <stable@vger.kernel.org> # 3.x: 8ec0a0e6b58: IB/umad: Fix error handling
---
Hi Bart and Roland,

It seems this one was not rolled in your latest katamari[1].
Since the original patch didn't apply on top of commit
8ec0a0e6b58 ('IB/umad: Fix error handling'), I've applied it
manually and resend it for review and inclusion.

I hope having correctly set the Cc: stable@vger.kernel.org
tag to state the dependency on earlier patch.

[1] this term is actually used by Xorg to team to describe
    the process of gathering each little tiny piece needed
    for a release :) By the way, it's also a video game.

Regards.

 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 9bdf576cd4ab..1acb99100556 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) :
@@ -812,7 +816,7 @@ static int ib_umad_open(struct inode *inode, struct file *filp)
 		goto out;
 	}
 
-	kref_get(&port->umad_dev->ref);
+	kobject_get(&port->umad_dev->kobj);
 
 out:
 	mutex_unlock(&port->file_mutex);
@@ -851,7 +855,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;
 }
@@ -902,7 +906,7 @@ static int ib_umad_sm_open(struct inode *inode, struct file *filp)
 	if (ret)
 		goto err_clr_sm_cap;
 
-	kref_get(&port->umad_dev->ref);
+	kobject_get(&port->umad_dev->kobj);
 
 	return 0;
 
@@ -932,7 +936,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;
 }
@@ -1000,6 +1004,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;
@@ -1032,6 +1037,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;
@@ -1050,6 +1056,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;
@@ -1143,7 +1150,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;
@@ -1151,7 +1158,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;
 	}
 
@@ -1163,7 +1171,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)
@@ -1177,7 +1185,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.9.3

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

[Roland Dreier]

On Fri, Jun 6, 2014 at 9:25 AM, Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org> wrote:
> It seems this one was not rolled in your latest katamari[1].


Thanks, picked it up this time around.

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