Message ID | 1506990743-1455-1-git-send-email-jk@ozlabs.org |
---|---|
State | Accepted, archived |
Headers | show |
Series | fsi/master: Clarify master lifetimes & fix use-after-free in hub master | expand |
Tested-by: Christopher Bostic <cbostic@linux.vnet.ibm.com> On 10/2/17 7:32 PM, Jeremy Kerr wrote: > Once we call fsi_master_unregister, the core will put_device, > potentially freeing the hub master. This change adds a comment > explaining the lifetime of an allocated fsi_master. > > We then add a reference from the driver to the hub master, so it stays > around until we've finished ->remove(). > > Signed-off-by: Jeremy Kerr <jk@ozlabs.org> > --- > drivers/fsi/fsi-master-hub.c | 19 ++++++++++++++++--- > drivers/fsi/fsi-master.h | 15 +++++++++++++++ > 2 files changed, 31 insertions(+), 3 deletions(-) > > diff --git a/drivers/fsi/fsi-master-hub.c b/drivers/fsi/fsi-master-hub.c > index 5e4cd31..614d5f5 100644 > --- a/drivers/fsi/fsi-master-hub.c > +++ b/drivers/fsi/fsi-master-hub.c > @@ -288,10 +288,19 @@ static int hub_master_probe(struct device *dev) > hub_master_init(hub); > > rc = fsi_master_register(&hub->master); > - if (!rc) > - return 0; > + if (rc) > + goto err_release; > + > + /* At this point, fsi_master_register performs the device_initialize(), > + * and holds the sole reference on master.dev. This means the device > + * will be freed (via ->release) during any subsequent call to > + * fsi_master_unregister. We add our own reference to it here, so we > + * can perform cleanup (in _remove()) without it being freed before > + * we're ready. > + */ > + get_device(&hub->master.dev); > + return 0; > > - kfree(hub); > err_release: > fsi_slave_release_range(fsi_dev->slave, FSI_HUB_LINK_OFFSET, > FSI_HUB_LINK_SIZE * links); > @@ -306,6 +315,10 @@ static int hub_master_remove(struct device *dev) > fsi_slave_release_range(hub->upstream->slave, hub->addr, hub->size); > of_node_put(hub->master.dev.of_node); > > + /* master.dev will likely be ->release()ed after this, which free()s > + * the hub */ > + put_device(&hub->master.dev); > + > return 0; > } > > diff --git a/drivers/fsi/fsi-master.h b/drivers/fsi/fsi-master.h > index 546257d..1530ab2 100644 > --- a/drivers/fsi/fsi-master.h > +++ b/drivers/fsi/fsi-master.h > @@ -37,6 +37,21 @@ struct fsi_master { > > #define dev_to_fsi_master(d) container_of(d, struct fsi_master, dev) > > +/** > + * fsi_master registration & lifetime: the fsi_master_register() and > + * fsi_master_unregister() functions will take ownership of the master, and > + * ->dev in particular. The registration path performs a get_device(), which > + * takes the first reference on the device. Similarly, the unregistration path > + * performs a put_device(), which may well drop the last reference. > + * > + * This means that master implementations *may* need to hold their own > + * reference (via get_device()) on master->dev. In particular, if the device's > + * ->release callback frees the fsi_master, then fsi_master_unregister will > + * invoke this free if no other reference is held. > + * > + * The same applies for the error path of fsi_master_register; if the call > + * fails, dev->release will have been invoked. > + */ > extern int fsi_master_register(struct fsi_master *master); > extern void fsi_master_unregister(struct fsi_master *master); >
On Tue, Oct 3, 2017 at 10:41 AM, Christopher Bostic <cbostic@linux.vnet.ibm.com> wrote: > On 10/2/17 7:32 PM, Jeremy Kerr wrote: >> >> Once we call fsi_master_unregister, the core will put_device, >> potentially freeing the hub master. This change adds a comment >> explaining the lifetime of an allocated fsi_master. >> >> We then add a reference from the driver to the hub master, so it stays >> around until we've finished ->remove(). >> >> Signed-off-by: Jeremy Kerr <jk@ozlabs.org> > Tested-by: Christopher Bostic <cbostic@linux.vnet.ibm.com> Applied to dev-4.10. We should also create a tree with these fixes for upstream inclusion. Cheers, Joel
diff --git a/drivers/fsi/fsi-master-hub.c b/drivers/fsi/fsi-master-hub.c index 5e4cd31..614d5f5 100644 --- a/drivers/fsi/fsi-master-hub.c +++ b/drivers/fsi/fsi-master-hub.c @@ -288,10 +288,19 @@ static int hub_master_probe(struct device *dev) hub_master_init(hub); rc = fsi_master_register(&hub->master); - if (!rc) - return 0; + if (rc) + goto err_release; + + /* At this point, fsi_master_register performs the device_initialize(), + * and holds the sole reference on master.dev. This means the device + * will be freed (via ->release) during any subsequent call to + * fsi_master_unregister. We add our own reference to it here, so we + * can perform cleanup (in _remove()) without it being freed before + * we're ready. + */ + get_device(&hub->master.dev); + return 0; - kfree(hub); err_release: fsi_slave_release_range(fsi_dev->slave, FSI_HUB_LINK_OFFSET, FSI_HUB_LINK_SIZE * links); @@ -306,6 +315,10 @@ static int hub_master_remove(struct device *dev) fsi_slave_release_range(hub->upstream->slave, hub->addr, hub->size); of_node_put(hub->master.dev.of_node); + /* master.dev will likely be ->release()ed after this, which free()s + * the hub */ + put_device(&hub->master.dev); + return 0; } diff --git a/drivers/fsi/fsi-master.h b/drivers/fsi/fsi-master.h index 546257d..1530ab2 100644 --- a/drivers/fsi/fsi-master.h +++ b/drivers/fsi/fsi-master.h @@ -37,6 +37,21 @@ struct fsi_master { #define dev_to_fsi_master(d) container_of(d, struct fsi_master, dev) +/** + * fsi_master registration & lifetime: the fsi_master_register() and + * fsi_master_unregister() functions will take ownership of the master, and + * ->dev in particular. The registration path performs a get_device(), which + * takes the first reference on the device. Similarly, the unregistration path + * performs a put_device(), which may well drop the last reference. + * + * This means that master implementations *may* need to hold their own + * reference (via get_device()) on master->dev. In particular, if the device's + * ->release callback frees the fsi_master, then fsi_master_unregister will + * invoke this free if no other reference is held. + * + * The same applies for the error path of fsi_master_register; if the call + * fails, dev->release will have been invoked. + */ extern int fsi_master_register(struct fsi_master *master); extern void fsi_master_unregister(struct fsi_master *master);
Once we call fsi_master_unregister, the core will put_device, potentially freeing the hub master. This change adds a comment explaining the lifetime of an allocated fsi_master. We then add a reference from the driver to the hub master, so it stays around until we've finished ->remove(). Signed-off-by: Jeremy Kerr <jk@ozlabs.org> --- drivers/fsi/fsi-master-hub.c | 19 ++++++++++++++++--- drivers/fsi/fsi-master.h | 15 +++++++++++++++ 2 files changed, 31 insertions(+), 3 deletions(-)