| Message ID | 146855333714.573.13934675433503265133.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Dan, On Thu, Jul 14, 2016 at 08:28:57PM -0700, Dan Williams wrote: > acpi_evaluate_object() allocates memory. Free the buffer allocated > during acpi_nfit_add(). > > 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> > --- > drivers/acpi/nfit.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c > index 0497175ee6cb..008dbaaa2b75 100644 > --- a/drivers/acpi/nfit.c > +++ b/drivers/acpi/nfit.c > @@ -2414,12 +2414,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); > - } > + kfree(buf.pointer); > + acpi_desc->nfit = NULL; Looks "acpi_desc->nfit = NULL" statement will be removed in [PATCH 2/2] immediately. Why add it in PATCH 1? > + } 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; > Other parts are no problem to me. Thanks a lot! Joey Lee
On Thu, Jul 14, 2016 at 10:15 PM, joeyli <jlee@suse.com> wrote: > Hi Dan, > > On Thu, Jul 14, 2016 at 08:28:57PM -0700, Dan Williams wrote: >> acpi_evaluate_object() allocates memory. Free the buffer allocated >> during acpi_nfit_add(). >> >> 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> >> --- >> drivers/acpi/nfit.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c >> index 0497175ee6cb..008dbaaa2b75 100644 >> --- a/drivers/acpi/nfit.c >> +++ b/drivers/acpi/nfit.c >> @@ -2414,12 +2414,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); >> - } >> + kfree(buf.pointer); >> + acpi_desc->nfit = NULL; > > Looks "acpi_desc->nfit = NULL" statement will be removed in [PATCH 2/2] > immediately. Why add it in PATCH 1? I was debating it, but for code readability of -stable kernels (where patch2 will not be included) I want to make it clear that nothing uses the value of ->nfit outside of acpi_nfit_init(). > >> + } 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; >> > > Other parts are no problem to me. Thanks.
On 07/15/2016 11:28 AM, Dan Williams wrote: > acpi_evaluate_object() allocates memory. Free the buffer allocated > during acpi_nfit_add(). > Dan, thanks for your fix. Another one is the use-after-free issue in acpi_nfit_notify(): /* Evaluate _FIT */ status = acpi_evaluate_object(adev->handle, "_FIT", NULL, &buf); ... acpi_desc->nfit = (struct acpi_nfit_header *)obj->buffer.pointer; ... kfree(buf.pointer);
On 07/14/16 20:28, Dan Williams wrote: > acpi_evaluate_object() allocates memory. Free the buffer allocated > during acpi_nfit_add(). > > 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> > --- > drivers/acpi/nfit.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c > index 0497175ee6cb..008dbaaa2b75 100644 > --- a/drivers/acpi/nfit.c > +++ b/drivers/acpi/nfit.c > @@ -2414,12 +2414,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); 'rc' is not set in this path, so it maybe used uninitialized by 'if (rc)' below. Should we set it to a non-zero value in this path? > - } > + kfree(buf.pointer); > + acpi_desc->nfit = NULL; I notice the following code in acpi_nfit_notify(): 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; dev_err(dev, "failed to merge updated NFIT\n"); } ... If we set acpi_desc->nfit to NULL in acpi_nfit_add() and acpi_nfit_init() in acpi_nfit_notify() fails, it will be impossible to restore the old nfit, because nfit_saved is NULL. Thanks, Haozhong > + } 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; >
On 07/15/16 15:55, Haozhong Zhang wrote: > On 07/14/16 20:28, Dan Williams wrote: > > acpi_evaluate_object() allocates memory. Free the buffer allocated > > during acpi_nfit_add(). > > > > 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> > > --- > > drivers/acpi/nfit.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c > > index 0497175ee6cb..008dbaaa2b75 100644 > > --- a/drivers/acpi/nfit.c > > +++ b/drivers/acpi/nfit.c > > @@ -2414,12 +2414,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); > > 'rc' is not set in this path, so it maybe used uninitialized by 'if (rc)' below. > Should we set it to a non-zero value in this path? 'rc' should be set to 0 here, as what patch 2 does. Sorry for my mistake. Haozhong
On Thu, Jul 14, 2016 at 10:47 PM, Xiao Guangrong <guangrong.xiao@intel.com> wrote: > > > On 07/15/2016 11:28 AM, Dan Williams wrote: >> >> acpi_evaluate_object() allocates memory. Free the buffer allocated >> during acpi_nfit_add(). >> > > Dan, thanks for your fix. > > Another one is the use-after-free issue in acpi_nfit_notify(): > > /* Evaluate _FIT */ > status = acpi_evaluate_object(adev->handle, "_FIT", NULL, &buf); > ... > acpi_desc->nfit = > (struct acpi_nfit_header *)obj->buffer.pointer; > ... > kfree(buf.pointer); grep for acpi_desc->nfit usages, there are no usages after acpi_nfit_init(). We go through the hassle of setting up nfit_saved for no reason.
On Fri, Jul 15, 2016 at 1:12 AM, Haozhong Zhang <haozhong.zhang@intel.com> wrote: > On 07/15/16 15:55, Haozhong Zhang wrote: >> On 07/14/16 20:28, Dan Williams wrote: >> > acpi_evaluate_object() allocates memory. Free the buffer allocated >> > during acpi_nfit_add(). >> > >> > 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> >> > --- >> > drivers/acpi/nfit.c | 7 +++++-- >> > 1 file changed, 5 insertions(+), 2 deletions(-) >> > >> > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c >> > index 0497175ee6cb..008dbaaa2b75 100644 >> > --- a/drivers/acpi/nfit.c >> > +++ b/drivers/acpi/nfit.c >> > @@ -2414,12 +2414,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); >> >> 'rc' is not set in this path, so it maybe used uninitialized by 'if (rc)' below. >> Should we set it to a non-zero value in this path? > > 'rc' should be set to 0 here, as what patch 2 does. Sorry for my mistake. No, this is good feedback because patch1 is targeted for -stable. Will fix, thanks!
diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c index 0497175ee6cb..008dbaaa2b75 100644 --- a/drivers/acpi/nfit.c +++ b/drivers/acpi/nfit.c @@ -2414,12 +2414,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); - } + kfree(buf.pointer); + acpi_desc->nfit = NULL; + } 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;
acpi_evaluate_object() allocates memory. Free the buffer allocated during acpi_nfit_add(). 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> --- drivers/acpi/nfit.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)