* [PATCH v2 1/2] nfit: fix _FIT evaluation memory leak @ 2016-07-15 19:32 Dan Williams 2016-07-18 5:57 ` Xiao Guangrong 0 siblings, 1 reply; 3+ messages in thread From: Dan Williams @ 2016-07-15 19:32 UTC (permalink / raw) To: linux-nvdimm Cc: Vishal Verma, linux-acpi, stable, Xiao Guangrong, Haozhong Zhang acpi_evaluate_object() allocates memory. Free the buffer allocated during acpi_nfit_add(). Also, make it clear that ->nfit is not used outside of acpi_nfit_init() context. Cc: <stable@vger.kernel.org> Cc: Vishal Verma <vishal.l.verma@intel.com> Reported-by: Xiao Guangrong <guangrong.xiao@intel.com> Reported-by: Haozhong Zhang <haozhong.zhang@intel.com> Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- Change since v1: * Fix unitialized use of 'rc' (Haozhong) * Clarify that their is no use-after-free problem in acpi_nfit_notify() (Xiao) drivers/acpi/nfit.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c index d89a02d9ed10..cbdbe13bdbe8 100644 --- a/drivers/acpi/nfit.c +++ b/drivers/acpi/nfit.c @@ -2390,7 +2390,7 @@ static int acpi_nfit_add(struct acpi_device *adev) struct acpi_table_header *tbl; acpi_status status = AE_OK; acpi_size sz; - int rc; + int rc = 0; status = acpi_get_table_with_size(ACPI_SIG_NFIT, 0, &tbl, &sz); if (ACPI_FAILURE(status)) { @@ -2427,12 +2427,15 @@ static int acpi_nfit_add(struct acpi_device *adev) acpi_desc->nfit = (struct acpi_nfit_header *)obj->buffer.pointer; sz = obj->buffer.length; + rc = acpi_nfit_init(acpi_desc, sz); } else dev_dbg(dev, "%s invalid type %d, ignoring _FIT\n", __func__, (int) obj->type); - } + acpi_desc->nfit = NULL; + kfree(buf.pointer); + } else + rc = acpi_nfit_init(acpi_desc, sz); - rc = acpi_nfit_init(acpi_desc, sz); if (rc) { nvdimm_bus_unregister(acpi_desc->nvdimm_bus); return rc; @@ -2454,7 +2457,6 @@ static void acpi_nfit_notify(struct acpi_device *adev, u32 event) { struct acpi_nfit_desc *acpi_desc = dev_get_drvdata(&adev->dev); struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL }; - struct acpi_nfit_header *nfit_saved; union acpi_object *obj; struct device *dev = &adev->dev; acpi_status status; @@ -2492,21 +2494,16 @@ static void acpi_nfit_notify(struct acpi_device *adev, u32 event) goto out_unlock; } - nfit_saved = acpi_desc->nfit; obj = buf.pointer; if (obj->type == ACPI_TYPE_BUFFER) { acpi_desc->nfit = (struct acpi_nfit_header *)obj->buffer.pointer; ret = acpi_nfit_init(acpi_desc, obj->buffer.length); - if (ret) { - /* Merge failed, restore old nfit, and exit */ - acpi_desc->nfit = nfit_saved; + if (ret) dev_err(dev, "failed to merge updated NFIT\n"); - } - } else { - /* Bad _FIT, restore old nfit */ + } else dev_err(dev, "Invalid _FIT\n"); - } + acpi_desc->nfit = NULL; kfree(buf.pointer); out_unlock: ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2 1/2] nfit: fix _FIT evaluation memory leak 2016-07-15 19:32 [PATCH v2 1/2] nfit: fix _FIT evaluation memory leak Dan Williams @ 2016-07-18 5:57 ` Xiao Guangrong 2016-07-18 17:28 ` Dan Williams 0 siblings, 1 reply; 3+ messages in thread From: Xiao Guangrong @ 2016-07-18 5:57 UTC (permalink / raw) To: Dan Williams, linux-nvdimm Cc: Vishal Verma, linux-acpi, stable, Haozhong Zhang On 07/16/2016 03:32 AM, Dan Williams wrote: > acpi_evaluate_object() allocates memory. Free the buffer allocated > during acpi_nfit_add(). Also, make it clear that ->nfit is not used > outside of acpi_nfit_init() context. > > Cc: <stable@vger.kernel.org> > Cc: Vishal Verma <vishal.l.verma@intel.com> > Reported-by: Xiao Guangrong <guangrong.xiao@intel.com> > Reported-by: Haozhong Zhang <haozhong.zhang@intel.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > Change since v1: > > * Fix unitialized use of 'rc' (Haozhong) > * Clarify that their is no use-after-free problem in acpi_nfit_notify() > (Xiao) > No... This is a real problem, please seem below. > drivers/acpi/nfit.c | 21 +++++++++------------ > 1 file changed, 9 insertions(+), 12 deletions(-) > > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c > index d89a02d9ed10..cbdbe13bdbe8 100644 > --- a/drivers/acpi/nfit.c > +++ b/drivers/acpi/nfit.c > @@ -2390,7 +2390,7 @@ static int acpi_nfit_add(struct acpi_device *adev) > struct acpi_table_header *tbl; > acpi_status status = AE_OK; > acpi_size sz; > - int rc; > + int rc = 0; > > status = acpi_get_table_with_size(ACPI_SIG_NFIT, 0, &tbl, &sz); > if (ACPI_FAILURE(status)) { > @@ -2427,12 +2427,15 @@ static int acpi_nfit_add(struct acpi_device *adev) > acpi_desc->nfit = > (struct acpi_nfit_header *)obj->buffer.pointer; > sz = obj->buffer.length; > + rc = acpi_nfit_init(acpi_desc, sz); > } else > dev_dbg(dev, "%s invalid type %d, ignoring _FIT\n", > __func__, (int) obj->type); > - } > + acpi_desc->nfit = NULL; > + kfree(buf.pointer); > + } else > + rc = acpi_nfit_init(acpi_desc, sz); > > - rc = acpi_nfit_init(acpi_desc, sz); > if (rc) { > nvdimm_bus_unregister(acpi_desc->nvdimm_bus); > return rc; > @@ -2454,7 +2457,6 @@ static void acpi_nfit_notify(struct acpi_device *adev, u32 event) > { > struct acpi_nfit_desc *acpi_desc = dev_get_drvdata(&adev->dev); > struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL }; > - struct acpi_nfit_header *nfit_saved; > union acpi_object *obj; > struct device *dev = &adev->dev; > acpi_status status; > @@ -2492,21 +2494,16 @@ static void acpi_nfit_notify(struct acpi_device *adev, u32 event) > goto out_unlock; > } > > - nfit_saved = acpi_desc->nfit; > obj = buf.pointer; > if (obj->type == ACPI_TYPE_BUFFER) { > acpi_desc->nfit = > (struct acpi_nfit_header *)obj->buffer.pointer; > ret = acpi_nfit_init(acpi_desc, obj->buffer.length); The issue is in acpi_nfit_init(), there are some info constructing nfit_spa is directly from acpi_desc->nfit, for example: acpi_nfit_init() -> add_table() -> add_spa(): static bool add_spa(struct acpi_nfit_desc *acpi_desc, struct nfit_table_prev *prev, struct acpi_nfit_system_address *spa) { ... list_for_each_entry(nfit_spa, &prev->spas, list) { if (memcmp(nfit_spa->spa, spa, length) == 0) { // A list_move_tail(&nfit_spa->list, &acpi_desc->spas); return true; } } ... return false; INIT_LIST_HEAD(&nfit_spa->list); nfit_spa->spa = spa; // B list_add_tail(&nfit_spa->list, &acpi_desc->spas); ... } Note at point B, @spa is from acpi_desc->nfit. At point A, this @spa will be used to check if it has already existed if hotplug event happens later. ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2 1/2] nfit: fix _FIT evaluation memory leak 2016-07-18 5:57 ` Xiao Guangrong @ 2016-07-18 17:28 ` Dan Williams 0 siblings, 0 replies; 3+ messages in thread From: Dan Williams @ 2016-07-18 17:28 UTC (permalink / raw) To: Xiao Guangrong Cc: linux-nvdimm@lists.01.org, Vishal Verma, Linux ACPI, stable@vger.kernel.org, Haozhong Zhang On Sun, Jul 17, 2016 at 10:57 PM, Xiao Guangrong <guangrong.xiao@intel.com> wrote: > > > On 07/16/2016 03:32 AM, Dan Williams wrote: >> >> acpi_evaluate_object() allocates memory. Free the buffer allocated >> during acpi_nfit_add(). Also, make it clear that ->nfit is not used >> outside of acpi_nfit_init() context. >> >> Cc: <stable@vger.kernel.org> >> Cc: Vishal Verma <vishal.l.verma@intel.com> >> Reported-by: Xiao Guangrong <guangrong.xiao@intel.com> >> Reported-by: Haozhong Zhang <haozhong.zhang@intel.com> >> Signed-off-by: Dan Williams <dan.j.williams@intel.com> >> --- >> Change since v1: >> >> * Fix unitialized use of 'rc' (Haozhong) >> * Clarify that their is no use-after-free problem in acpi_nfit_notify() >> (Xiao) >> > > No... This is a real problem, please seem below. > > >> drivers/acpi/nfit.c | 21 +++++++++------------ >> 1 file changed, 9 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c >> index d89a02d9ed10..cbdbe13bdbe8 100644 >> --- a/drivers/acpi/nfit.c >> +++ b/drivers/acpi/nfit.c >> @@ -2390,7 +2390,7 @@ static int acpi_nfit_add(struct acpi_device *adev) >> struct acpi_table_header *tbl; >> acpi_status status = AE_OK; >> acpi_size sz; >> - int rc; >> + int rc = 0; >> >> status = acpi_get_table_with_size(ACPI_SIG_NFIT, 0, &tbl, &sz); >> if (ACPI_FAILURE(status)) { >> @@ -2427,12 +2427,15 @@ static int acpi_nfit_add(struct acpi_device *adev) >> acpi_desc->nfit = >> (struct acpi_nfit_header >> *)obj->buffer.pointer; >> sz = obj->buffer.length; >> + rc = acpi_nfit_init(acpi_desc, sz); >> } else >> dev_dbg(dev, "%s invalid type %d, ignoring >> _FIT\n", >> __func__, (int) obj->type); >> - } >> + acpi_desc->nfit = NULL; >> + kfree(buf.pointer); >> + } else >> + rc = acpi_nfit_init(acpi_desc, sz); >> >> - rc = acpi_nfit_init(acpi_desc, sz); >> if (rc) { >> nvdimm_bus_unregister(acpi_desc->nvdimm_bus); >> return rc; >> @@ -2454,7 +2457,6 @@ static void acpi_nfit_notify(struct acpi_device >> *adev, u32 event) >> { >> struct acpi_nfit_desc *acpi_desc = dev_get_drvdata(&adev->dev); >> struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL }; >> - struct acpi_nfit_header *nfit_saved; >> union acpi_object *obj; >> struct device *dev = &adev->dev; >> acpi_status status; >> @@ -2492,21 +2494,16 @@ static void acpi_nfit_notify(struct acpi_device >> *adev, u32 event) >> goto out_unlock; >> } >> >> - nfit_saved = acpi_desc->nfit; >> obj = buf.pointer; >> if (obj->type == ACPI_TYPE_BUFFER) { >> acpi_desc->nfit = >> (struct acpi_nfit_header *)obj->buffer.pointer; >> ret = acpi_nfit_init(acpi_desc, obj->buffer.length); > > > The issue is in acpi_nfit_init(), there are some info constructing nfit_spa > is directly from acpi_desc->nfit, for example: > acpi_nfit_init() -> add_table() -> add_spa(): > > static bool add_spa(struct acpi_nfit_desc *acpi_desc, > struct nfit_table_prev *prev, > struct acpi_nfit_system_address *spa) > { > ... > list_for_each_entry(nfit_spa, &prev->spas, list) { > if (memcmp(nfit_spa->spa, spa, length) == 0) { // A > list_move_tail(&nfit_spa->list, &acpi_desc->spas); > return true; > } > } > ... > return false; > INIT_LIST_HEAD(&nfit_spa->list); > nfit_spa->spa = spa; // B > list_add_tail(&nfit_spa->list, &acpi_desc->spas); > ... > } > > Note at point B, @spa is from acpi_desc->nfit. At point A, this @spa will be > used > to check if it has already existed if hotplug event happens later. > Argh, yes! We need a unit test for this case. Another problem is that the old and new nfit entries have different lifetimes, especially if we ever want to support delete. Another problem looking at this code is that the sizing of the idt and flush entries is wrong. We should not allow those entries to change size during hotplug. ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-07-18 17:28 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-07-15 19:32 [PATCH v2 1/2] nfit: fix _FIT evaluation memory leak Dan Williams 2016-07-18 5:57 ` Xiao Guangrong 2016-07-18 17:28 ` Dan Williams
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).