Fix a use-after-free in ib_umad
v2 cover Fix a use-after-free in ib_umad
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH v2 1/2 IB/umad: Fix error handling
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.
--- drivers/infiniband/core/user_mad.c | 58 ++++++++++++++++++++++---------------- 1 file changed, 33 insertions(+), 25 deletions(-)
drivers/infiniband/core/user_mad.c +33 -25
@@ -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)
Signature · 6 lines
1.8.4.5 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 2/2 IB/umad: Fix a use-after-free
In reply to a message not in the archiveAvoid 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
--- drivers/infiniband/core/user_mad.c | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-)
drivers/infiniband/core/user_mad.c +22 -11
@@ -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)
Signature · 6 lines
1.8.4.5 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH v2 2/2] IB/umad: Fix a use-after-free
Hi,
Quoted 106 lines · Bart Van Assche
Avoid that closing /dev/infiniband/umad<n> or /dev/infiniband/issm<n> triggers a use-after-free.
__fput()infs/file_table.cinvokesf_op->release()before it invokescdev_put(). Make sure that theib_umad_devicestructure is freed by thecdev_put()call instead off_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/0x17Reference: https://bugzilla.kernel.org/show_bug.cgi?id=75051
Signed-off-by Bart Van Assche <[email protected]>Cc Alex Chiang <[email protected]>Cc Yann Droneaud <[email protected]>Cc <[email protected]>--- drivers/infiniband/core/user_mad.c | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-)drivers/infiniband/core/user_mad.c +12
-7@@ -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;
drivers/infiniband/core/user_mad.c static int ib_umad_init_port(struct ib_device *device, int port_num, · line 1077
Quoted 5 lines
drivers/infiniband/core/user_mad.c +2
-0@@ -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.
drivers/infiniband/core/user_mad.c static int ib_umad_init_port(struct ib_device *device, int port_num, · line 1080
Quoted 3 lines
drivers/infiniband/core/user_mad.c +3
-0+ 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().
Quoted 32 lines
drivers/infiniband/core/user_mad.c +4
-3return 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)
drivers/infiniband/core/user_mad.c static void ib_umad_remove_one(struct ib_device *device) · line 1197
Quoted 8 lines
drivers/infiniband/core/user_mad.c +1
-1@@ -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.
Signature · 8 lines
Yann Droneaud OPTEYA -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html