drivers: sbefifo and occ-hwmon: Fix memory corruption

PATCH Applied Eddie James 2 participants 4 messages Oct 26, 2017

Subsystems (2)

Hardware monitoring FSI

lore

Share & tools

lore

Eddie James

author

2017-10-26 20:15 UTC #0 lore raw

cover drivers: sbefifo and occ-hwmon: Fix memory corruption

From: "Edward A. James" <[email protected]>

There was some potential for bad memory access in the sbefifo driver. List iteration will access the current position iterator, which in some cases in the sbefifo driver was freed. Use a safe loop and delete the member from the list first.

The second, more severe, problem was an incorrect amount of memory allocated for the extended sensor attributes in the occ-hwmon driver. That was my mistake in initially adding the extended attributes. I suppose we haven't hit much memory corruption because the occ doesn't normally return extended sensors.

Edward A. James (2):
drivers (fsi): sbefifo: Fix xfr deletion in lists drivers (hwmon): occ: Fix memory allocation for extended sensors

 drivers/fsi/fsi-sbefifo.c  | 19 ++++++++++++-------
 drivers/hwmon/occ/common.c |  2 +-
 2 files changed, 13 insertions(+), 8 deletions(-)

Signature · 1 line

1.8.3.1

Eddie James

author

2017-10-26 20:15 UTC #1 lore raw

PATCH 1/2 drivers (fsi): sbefifo: Fix xfr deletion in lists

From: "Edward A. James" <[email protected]>

Deleting an element of a list while iterating means that you'll access recently freed memory when getting the next list element.

Signed-off-by Edward A. James <[email protected]>

---
 drivers/fsi/fsi-sbefifo.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

drivers/fsi/fsi-sbefifo.c +12 -7

index f756822..088172c 100644
@@ -316,14 +316,15 @@ static void sbefifo_client_release(struct kref *kref)
 {
 	struct sbefifo *sbefifo;
 	struct sbefifo_client *client;
-	struct sbefifo_xfr *xfr;
+	struct sbefifo_xfr *xfr, *tmp;
 
 	client = container_of(kref, struct sbefifo_client, kref);
 	sbefifo = client->dev;
 
 	if (!READ_ONCE(sbefifo->rc)) {
-		list_for_each_entry(xfr, &client->xfrs, client) {
+		list_for_each_entry_safe(xfr, tmp, &client->xfrs, client) {
 			if (test_bit(SBEFIFO_XFR_COMPLETE, &xfr->flags)) {
+				list_del(&xfr->client);
 				kfree(xfr);
 				continue;
 			}

@@ -376,7 +377,7 @@ static void sbefifo_poll_timer(unsigned long data)
 	static const unsigned long EOT_MASK = 0x000000ff;
 	struct sbefifo *sbefifo = (void *)data;
 	struct sbefifo_buf *rbuf, *wbuf;
-	struct sbefifo_xfr *xfr = NULL;
+	struct sbefifo_xfr *xfr, *tmp;
 	struct sbefifo_buf drain;
 	size_t devn, bufn;
 	int eot = 0;

@@ -491,8 +492,10 @@ static void sbefifo_poll_timer(unsigned long data)
 		sbefifo->rc = ret;
 		dev_err(&sbefifo->fsi_dev->dev,
 			"Fatal bus access failure: %d\n", ret);
-		list_for_each_entry(xfr, &sbefifo->xfrs, xfrs)
+		list_for_each_entry_safe(xfr, tmp, &sbefifo->xfrs, xfrs) {
+			list_del(&xfr->xfrs);
 			kfree(xfr);
+		}
 		INIT_LIST_HEAD(&sbefifo->xfrs);
 
 	} else if (eot && sbefifo_next_xfr(sbefifo)) {

@@ -620,8 +623,8 @@ static ssize_t sbefifo_read_common(struct sbefifo_client *client,
 			if (mod_timer(&client->dev->poll_timer, jiffies))
 				sbefifo_put(sbefifo);
 		} else {
-			kfree(xfr);
 			list_del(&xfr->client);
+			kfree(xfr);
 			wake_up_interruptible(&sbefifo->wait);
 		}
 	}

@@ -940,13 +943,15 @@ static int sbefifo_probe(struct device *dev)
 static int sbefifo_remove(struct device *dev)
 {
 	struct sbefifo *sbefifo = dev_get_drvdata(dev);
-	struct sbefifo_xfr *xfr;
+	struct sbefifo_xfr *xfr, *tmp;
 
 	spin_lock(&sbefifo->lock);
 
 	WRITE_ONCE(sbefifo->rc, -ENODEV);
-	list_for_each_entry(xfr, &sbefifo->xfrs, xfrs)
+	list_for_each_entry_safe(xfr, tmp, &sbefifo->xfrs, xfrs) {
+		list_del(&xfr->xfrs);
 		kfree(xfr);
+	}
 
 	INIT_LIST_HEAD(&sbefifo->xfrs);

Signature · 1 line

1.8.3.1

Eddie James

author

2017-10-26 20:15 UTC #2 lore raw

PATCH 2/2 drivers (hwmon): occ: Fix memory allocation for extended sensors

In reply to Eddie James

From: "Edward A. James" <[email protected]>

If extended sensors are present, the driver is doing bad memory access since we weren't allocating enough memory for the extended sensors. Fix this with proper allocation for extended sensor attributes.

Signed-off-by Edward A. James <[email protected]>

---
 drivers/hwmon/occ/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/hwmon/occ/common.c +1 -1

index 3b9adf4..3175e7b 100644
@@ -872,7 +872,7 @@ int occ_setup_sensor_attrs(struct occ *occ)
 
 	switch (sensors->extended.version) {
 	case 1:
-		occ->num_attrs += sensors->extended.num_sensors;
+		occ->num_attrs += (sensors->extended.num_sensors * 3);
 		break;
 	default:
 		sensors->extended.num_sensors = 0;

Signature · 1 line

1.8.3.1

Andrew Jeffery

Applied

2017-10-26 23:14 UTC #3 lore raw

In reply to Eddie James

Quoted 7 lines · Eddie James

Quoted 1 line
From: "Edward A. James" <[email protected]>
There was some potential for bad memory access in the sbefifo driver. List iteration will access the current position iterator, which in some cases in the sbefifo driver was freed. Use a safe loop and delete the member from the list first.

Quoted 8 lines

The second, more severe, problem was an incorrect amount of memory allocated for the extended sensor attributes in the occ-hwmon driver. That was my mistake in initially adding the extended attributes. I suppose we haven't hit much memory corruption because the occ doesn't normally return extended sensors.
Edward A. James (2):
drivers (fsi): sbefifo: Fix xfr deletion in lists drivers (hwmon): occ: Fix memory allocation for extended sensors

Thanks, applied to dev-4.10.

Andrew

Quoted 5 lines

drivers/fsi/fsi-sbefifo.c | 19 ++++++++++++------- drivers/hwmon/occ/common.c | 2 +-
 2 files changed, 13 insertions(+), 8 deletions(-)