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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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@xxxxxxxxxxxxxxx>
Cc: Vishal Verma <vishal.l.verma@xxxxxxxxx>
Reported-by: Xiao Guangrong <guangrong.xiao@xxxxxxxxx>
Reported-by: Haozhong Zhang <haozhong.zhang@xxxxxxxxx>
Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
---
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.

--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]