| Message ID | 20170217152651.GA18275@localhost.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 17, 2017 at 10:26:51AM -0500, Keith Busch wrote: > On Fri, Feb 17, 2017 at 01:59:41PM +0100, Christoph Hellwig wrote: > > @@ -1789,7 +1789,8 @@ static void nvme_reset_work(struct work_struct *work) > > if (result) > > goto out; > > > > - if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) { > > + kfree(dev->ctrl.opal_dev); > > + if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) { > > dev->ctrl.opal_dev = > > init_opal_dev(&dev->ctrl, &nvme_sec_submit); > > } > > A couple things. > > This has a use-after-free in opal_unlock_from_suspend if the nvme > device had an opal_dev before, but no longer support the capability > after resume. So you'd want to set ctrl.opal_dev to NULL after the free. > > But we don't want to unconditionally free it anyway during resume > since opal_unlock_from_suspend requires the exisiting opal_dev state > information saved in the 'unlk_list'. > > Something like this instead: > > --- > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c > index ddc51ad..8fa6be9 100644 > --- a/drivers/nvme/host/pci.c > +++ b/drivers/nvme/host/pci.c > @@ -1789,13 +1789,17 @@ static void nvme_reset_work(struct work_struct *work) > if (result) > goto out; > > - if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) { > - dev->ctrl.opal_dev = > - init_opal_dev(&dev->ctrl, &nvme_sec_submit); > + if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) > + if (was_suspend && dev->ctrl.opal_dev) > + opal_unlock_from_suspend(dev->ctrl.opal_dev); > + else if (!dev->ctrl.opal_dev) > + dev->ctrl.opal_dev = > + init_opal_dev(&dev->ctrl, &nvme_sec_submit); > + } else { > + kfree(dev->ctrl.opal_dev); > + dev->ctrl.opal_dev = NULL; Keith's comments made me realize something even deeper as well. Assuming the firmware changed and we no longer support security commands we need to free the opal_dev structure like we're doing but there is a possiblity that there were saved ranges in the structure that we need to free as well. If the user had previously told the kernel to unlock 5 ranges coming out of a suspend there are 5 structures we need to free inside the opal_dev before we free the opal dev. We'll need to re-introduce free_opal_dev() in the opal code like we had a while back.
On Fri, Feb 17, 2017 at 06:01:28PM +0100, Christoph Hellwig wrote: > On Fri, Feb 17, 2017 at 10:26:51AM -0500, Keith Busch wrote: > > On Fri, Feb 17, 2017 at 01:59:41PM +0100, Christoph Hellwig wrote: > > > @@ -1789,7 +1789,8 @@ static void nvme_reset_work(struct work_struct *work) > > > if (result) > > > goto out; > > > > > > - if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) { > > > + kfree(dev->ctrl.opal_dev); > > > + if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) { > > > dev->ctrl.opal_dev = > > > init_opal_dev(&dev->ctrl, &nvme_sec_submit); > > > } > > > > A couple things. > > > > This has a use-after-free in opal_unlock_from_suspend if the nvme > > device had an opal_dev before, but no longer support the capability > > after resume. So you'd want to set ctrl.opal_dev to NULL after the free. > > > > But we don't want to unconditionally free it anyway during resume > > since opal_unlock_from_suspend requires the exisiting opal_dev state > > information saved in the 'unlk_list'. > > > > Something like this instead: > > Yes, that looks fine to me. We'll probably also need the additional > fixup Scott pointed out. I'm working on it now. Do you want a diff like Keith did or a separate patch?
On Fri, Feb 17, 2017 at 10:26:51AM -0500, Keith Busch wrote: > On Fri, Feb 17, 2017 at 01:59:41PM +0100, Christoph Hellwig wrote: > > @@ -1789,7 +1789,8 @@ static void nvme_reset_work(struct work_struct *work) > > if (result) > > goto out; > > > > - if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) { > > + kfree(dev->ctrl.opal_dev); > > + if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) { > > dev->ctrl.opal_dev = > > init_opal_dev(&dev->ctrl, &nvme_sec_submit); > > } > > A couple things. > > This has a use-after-free in opal_unlock_from_suspend if the nvme > device had an opal_dev before, but no longer support the capability > after resume. So you'd want to set ctrl.opal_dev to NULL after the free. > > But we don't want to unconditionally free it anyway during resume > since opal_unlock_from_suspend requires the exisiting opal_dev state > information saved in the 'unlk_list'. > > Something like this instead: Yes, that looks fine to me. We'll probably also need the additional fixup Scott pointed out.
On Fri, Feb 17, 2017 at 09:55:51AM -0700, Scott Bauer wrote:
> I'm working on it now. Do you want a diff like Keith did or a separate patch?
Please make it a proper patch that applies on top of my patches 1-3 (where
3 really is yours anyway). I'll respin 4 with the updates from Keith on
top of 1-3 + your patch then.
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index ddc51ad..8fa6be9 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -1789,13 +1789,17 @@ static void nvme_reset_work(struct work_struct *work) if (result) goto out; - if ((dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) && !dev->ctrl.opal_dev) { - dev->ctrl.opal_dev = - init_opal_dev(&dev->ctrl, &nvme_sec_submit); + if (dev->ctrl.oacs & NVME_CTRL_OACS_SEC_SUPP) + if (was_suspend && dev->ctrl.opal_dev) + opal_unlock_from_suspend(dev->ctrl.opal_dev); + else if (!dev->ctrl.opal_dev) + dev->ctrl.opal_dev = + init_opal_dev(&dev->ctrl, &nvme_sec_submit); + } else { + kfree(dev->ctrl.opal_dev); + dev->ctrl.opal_dev = NULL; } - if (was_suspend) - opal_unlock_from_suspend(dev->ctrl.opal_dev); result = nvme_setup_io_queues(dev); if (result)