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

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

[Dan Williams]

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:

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH v2 2/2] nfit: cleanup acpi_nfit_init calling convention

[Dan Williams]

Pass the nfit buffer as a parameter rather than hanging it off of
acpi_desc.

Reviewed-by: "Lee, Chun-Yi" <jlee@suse.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
Changes since v1:

* Reflowed based on fixups done to patch1, no other changes.

 drivers/acpi/nfit.c              |   48 +++++++++++++-------------------------
 drivers/acpi/nfit.h              |    3 +-
 tools/testing/nvdimm/test/nfit.c |    7 +++---
 3 files changed, 22 insertions(+), 36 deletions(-)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index cbdbe13bdbe8..4173cf0033c2 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -2237,12 +2237,11 @@ static int acpi_nfit_check_deletions(struct acpi_nfit_desc *acpi_desc,
 	return 0;
 }
 
-int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, acpi_size sz)
+int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, void *data, acpi_size sz)
 {
 	struct device *dev = acpi_desc->dev;
 	struct nfit_table_prev prev;
 	const void *end;
-	u8 *data;
 	int rc;
 
 	mutex_lock(&acpi_desc->init_mutex);
@@ -2267,7 +2266,6 @@ int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, acpi_size sz)
 	list_cut_position(&prev.flushes, &acpi_desc->flushes,
 				acpi_desc->flushes.prev);
 
-	data = (u8 *) acpi_desc->nfit;
 	end = data + sz;
 	while (!IS_ERR_OR_NULL(data))
 		data = add_table(acpi_desc, &prev, data, end);
@@ -2407,40 +2405,30 @@ static int acpi_nfit_add(struct acpi_device *adev)
 	if (!acpi_desc->nvdimm_bus)
 		return -ENOMEM;
 
-	/*
-	 * Save the acpi header for later and then skip it,
-	 * making nfit point to the first nfit table header.
-	 */
+	/* Save the acpi header for exporting the revision via sysfs */
 	acpi_desc->acpi_header = *tbl;
-	acpi_desc->nfit = (void *) tbl + sizeof(struct acpi_table_nfit);
-	sz -= sizeof(struct acpi_table_nfit);
 
 	/* Evaluate _FIT and override with that if present */
 	status = acpi_evaluate_object(adev->handle, "_FIT", NULL, &buf);
 	if (ACPI_SUCCESS(status) && buf.length > 0) {
-		union acpi_object *obj;
-		/*
-		 * Adjust for the acpi_object header of the _FIT
-		 */
-		obj = buf.pointer;
-		if (obj->type == ACPI_TYPE_BUFFER) {
-			acpi_desc->nfit =
-				(struct acpi_nfit_header *)obj->buffer.pointer;
-			sz = obj->buffer.length;
-			rc = acpi_nfit_init(acpi_desc, sz);
-		} else
+		union acpi_object *obj = buf.pointer;
+
+		if (obj->type == ACPI_TYPE_BUFFER)
+			rc = acpi_nfit_init(acpi_desc, obj->buffer.pointer,
+					obj->buffer.length);
+		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);
+		/* skip over the lead-in header table */
+		rc = acpi_nfit_init(acpi_desc, (void *) tbl
+				+ sizeof(struct acpi_table_nfit),
+				sz - sizeof(struct acpi_table_nfit));
 
-	if (rc) {
+	if (rc)
 		nvdimm_bus_unregister(acpi_desc->nvdimm_bus);
-		return rc;
-	}
-	return 0;
+	return rc;
 }
 
 static int acpi_nfit_remove(struct acpi_device *adev)
@@ -2457,8 +2445,8 @@ 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 };
-	union acpi_object *obj;
 	struct device *dev = &adev->dev;
+	union acpi_object *obj;
 	acpi_status status;
 	int ret;
 
@@ -2496,14 +2484,12 @@ static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
 
 	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);
+		ret = acpi_nfit_init(acpi_desc, obj->buffer.pointer,
+				obj->buffer.length);
 		if (ret)
 			dev_err(dev, "failed to merge updated NFIT\n");
 	} else
 		dev_err(dev, "Invalid _FIT\n");
-	acpi_desc->nfit = NULL;
 	kfree(buf.pointer);
 
  out_unlock:
diff --git a/drivers/acpi/nfit.h b/drivers/acpi/nfit.h
index 80fb2c0ac8bf..5179eba97a47 100644
--- a/drivers/acpi/nfit.h
+++ b/drivers/acpi/nfit.h
@@ -135,7 +135,6 @@ struct nfit_mem {
 struct acpi_nfit_desc {
 	struct nvdimm_bus_descriptor nd_desc;
 	struct acpi_table_header acpi_header;
-	struct acpi_nfit_header *nfit;
 	struct mutex init_mutex;
 	struct list_head memdevs;
 	struct list_head flushes;
@@ -201,6 +200,6 @@ static inline struct acpi_nfit_desc *to_acpi_desc(
 }
 
 const u8 *to_nfit_uuid(enum nfit_uuids id);
-int acpi_nfit_init(struct acpi_nfit_desc *nfit, acpi_size sz);
+int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, void *nfit, acpi_size sz);
 void acpi_nfit_desc_init(struct acpi_nfit_desc *acpi_desc, struct device *dev);
 #endif /* __NFIT_H__ */
diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c
index ff09a28890ed..52df3c20231d 100644
--- a/tools/testing/nvdimm/test/nfit.c
+++ b/tools/testing/nvdimm/test/nfit.c
@@ -1458,7 +1458,6 @@ static int nfit_test_probe(struct platform_device *pdev)
 	nfit_test->setup(nfit_test);
 	acpi_desc = &nfit_test->acpi_desc;
 	acpi_nfit_desc_init(acpi_desc, &pdev->dev);
-	acpi_desc->nfit = nfit_test->nfit_buf;
 	acpi_desc->blk_do_io = nfit_test_blk_do_io;
 	nd_desc = &acpi_desc->nd_desc;
 	nd_desc->provider_name = NULL;
@@ -1467,7 +1466,8 @@ static int nfit_test_probe(struct platform_device *pdev)
 	if (!acpi_desc->nvdimm_bus)
 		return -ENXIO;
 
-	rc = acpi_nfit_init(acpi_desc, nfit_test->nfit_size);
+	rc = acpi_nfit_init(acpi_desc, nfit_test->nfit_buf,
+			nfit_test->nfit_size);
 	if (rc) {
 		nvdimm_bus_unregister(acpi_desc->nvdimm_bus);
 		return rc;
@@ -1479,7 +1479,8 @@ static int nfit_test_probe(struct platform_device *pdev)
 	nfit_test->setup_hotplug = 1;
 	nfit_test->setup(nfit_test);
 
-	rc = acpi_nfit_init(acpi_desc, nfit_test->nfit_size);
+	rc = acpi_nfit_init(acpi_desc, nfit_test->nfit_buf,
+			nfit_test->nfit_size);
 	if (rc) {
 		nvdimm_bus_unregister(acpi_desc->nvdimm_bus);
 		return rc;

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

[Xiao Guangrong]

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.

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

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

[Dan Williams]

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.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm