[PATCH 1/2] nfit: fix _FIT evaluation memory leak

[PATCH 1/2] nfit: fix _FIT evaluation memory leak

[Dan Williams]

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;
+	} 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;

Re: [PATCH 1/2] nfit: fix _FIT evaluation memory leak

[joeyli]

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

Re: [PATCH 1/2] nfit: fix _FIT evaluation memory leak

[Dan Williams]

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.

Re: [PATCH 1/2] nfit: fix _FIT evaluation memory leak

[Xiao Guangrong]

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);

Re: [PATCH 1/2] nfit: fix _FIT evaluation memory leak

[Haozhong Zhang]

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;
> 

Re: [PATCH 1/2] nfit: fix _FIT evaluation memory leak

[Haozhong Zhang]

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

Re: [PATCH 1/2] nfit: fix _FIT evaluation memory leak

[Dan Williams]

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.

Re: [PATCH 1/2] nfit: fix _FIT evaluation memory leak

[Dan Williams]

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!