| Message ID | 5375F108.20608@acm.org (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
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@acm.org> > Cc: Alex Chiang <achiang@canonical.com> > Cc: Yann Droneaud <ydroneaud@opteya.com> > Cc: <stable@vger.kernel.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@opteya.com>
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)
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> Cc: Alex Chiang <achiang@canonical.com> Cc: Yann Droneaud <ydroneaud@opteya.com> Cc: <stable@vger.kernel.org> --- drivers/infiniband/core/user_mad.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-)