| Message ID | 146861106687.16732.18123746119721100494.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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.
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:
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(-)