[PATCH 0/2] power: generic-adc-battery: fix out of bounds write

[PATCH 0/2] power: generic-adc-battery: fix out of bounds write

[H. Nikolaus Schaller]

This patch set addresses two bugs in the gab_probe() function which are
there since the first commit in 3.7-rc1:

1. there is an out of bounds write access by a miscalculated destination
   address for the memcpy()
2. if iio channels are already represented by default properties, they
   appear as duplicates in uevent


H. Nikolaus Schaller (2):
  power: generic-adc-battery: fix out-of-bounds write when copying
    channel properties
  power: generic-adc-battery: check for duplicate properties copied from
    iio channels

 drivers/power/supply/generic-adc-battery.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

-- 
2.12.2

[PATCH 1/2] power: generic-adc-battery: fix out-of-bounds write when copying channel properties

[H. Nikolaus Schaller]

We did have sporadic problems in the pinctrl framework during boot
where a pin group name unexpectedly became NULL leading to a NULL
dereference in strcmp.

Detailled analysis of the failing cases did reveal that there were
two devm allocated objects close to each other. The second one was
the affected group_desc in pinmux and the first one was the
psy_desc->properties buffer of the gab driver.

Review of the gab code showed that the address calculation for
one memcpy() is wrong. It does

	properties + sizeof(type) * index

but C is defined to do the index multiplication already for
pointer + integer additions. Hence the factor was applied twice
and the memcpy() does write outside of the properties buffer.
Sometimes it happened to be the pinctrl and triggered the strcmp(NULL).

Anyways, it is overkill to use a memcpy() here instead of a simple
assignment, which is easier to read and has less risk for wrong
address calculations. So we change code to a simple assignment.

If we initialize the index to the first free location, we can even
remove the local variable 'properties'.

This bug seems to exist right from the beginning in 3.7-rc1 in

commit e60fea794e6e ("power: battery: Generic battery driver using IIO")

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
Cc: stable@vger.kernel.org
---
 drivers/power/supply/generic-adc-battery.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/power/supply/generic-adc-battery.c b/drivers/power/supply/generic-adc-battery.c
index 28dc056eaafa..11f59275bff4 100644
--- a/drivers/power/supply/generic-adc-battery.c
+++ b/drivers/power/supply/generic-adc-battery.c
@@ -241,10 +241,9 @@ static int gab_probe(struct platform_device *pdev)
 	struct power_supply_desc *psy_desc;
 	struct power_supply_config psy_cfg = {};
 	struct gab_platform_data *pdata = pdev->dev.platform_data;
-	enum power_supply_property *properties;
 	int ret = 0;
 	int chan;
-	int index = 0;
+	int index = ARRAY_SIZE(gab_props);
 
 	adc_bat = devm_kzalloc(&pdev->dev, sizeof(*adc_bat), GFP_KERNEL);
 	if (!adc_bat) {
@@ -278,8 +277,6 @@ static int gab_probe(struct platform_device *pdev)
 	}
 
 	memcpy(psy_desc->properties, gab_props, sizeof(gab_props));
-	properties = (enum power_supply_property *)
-			((char *)psy_desc->properties + sizeof(gab_props));
 
 	/*
 	 * getting channel from iio and copying the battery properties
@@ -293,15 +290,12 @@ static int gab_probe(struct platform_device *pdev)
 			adc_bat->channel[chan] = NULL;
 		} else {
 			/* copying properties for supported channels only */
-			memcpy(properties + sizeof(*(psy_desc->properties)) * index,
-					&gab_dyn_props[chan],
-					sizeof(gab_dyn_props[chan]));
-			index++;
+			psy_desc->properties[index++] = gab_dyn_props[chan];
 		}
 	}
 
 	/* none of the channels are supported so let's bail out */
-	if (index == 0) {
+	if (index == ARRAY_SIZE(gab_props)) {
 		ret = -ENODEV;
 		goto second_mem_fail;
 	}
@@ -312,7 +306,7 @@ static int gab_probe(struct platform_device *pdev)
 	 * as come channels may be not be supported by the device.So
 	 * we need to take care of that.
 	 */
-	psy_desc->num_properties = ARRAY_SIZE(gab_props) + index;
+	psy_desc->num_properties = index;
 
 	adc_bat->psy = power_supply_register(&pdev->dev, psy_desc, &psy_cfg);
 	if (IS_ERR(adc_bat->psy)) {
-- 
2.12.2

[PATCH 2/2] power: generic-adc-battery: check for duplicate properties copied from iio channels

[H. Nikolaus Schaller]

If an iio channel defines a basic property, there are duplicate entries
in /sys/class/power/*/uevent.

So add a check to avoid duplicates. Since all channels may be duplicates,
we have to modify the related error check.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
Cc: stable@vger.kernel.org
---
 drivers/power/supply/generic-adc-battery.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/power/supply/generic-adc-battery.c b/drivers/power/supply/generic-adc-battery.c
index 11f59275bff4..bc462d1ec963 100644
--- a/drivers/power/supply/generic-adc-battery.c
+++ b/drivers/power/supply/generic-adc-battery.c
@@ -244,6 +244,7 @@ static int gab_probe(struct platform_device *pdev)
 	int ret = 0;
 	int chan;
 	int index = ARRAY_SIZE(gab_props);
+	bool any = false;
 
 	adc_bat = devm_kzalloc(&pdev->dev, sizeof(*adc_bat), GFP_KERNEL);
 	if (!adc_bat) {
@@ -290,12 +291,22 @@ static int gab_probe(struct platform_device *pdev)
 			adc_bat->channel[chan] = NULL;
 		} else {
 			/* copying properties for supported channels only */
-			psy_desc->properties[index++] = gab_dyn_props[chan];
+			int index2;
+
+			for (index2 = 0; index2 < index; index2++) {
+				if (psy_desc->properties[index2] ==
+				    gab_dyn_props[chan])
+					break;	/* already known */
+			}
+			if (index2 == index)	/* really new */
+				psy_desc->properties[index++] =
+					gab_dyn_props[chan];
+			any = true;
 		}
 	}
 
 	/* none of the channels are supported so let's bail out */
-	if (index == ARRAY_SIZE(gab_props)) {
+	if (!any) {
 		ret = -ENODEV;
 		goto second_mem_fail;
 	}
-- 
2.12.2

Re: [PATCH 0/2] power: generic-adc-battery: fix out of bounds write

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 483 bytes --]

Hi,

On Tue, Jun 26, 2018 at 03:28:28PM +0200, H. Nikolaus Schaller wrote:
> This patch set addresses two bugs in the gab_probe() function which are
> there since the first commit in 3.7-rc1:
> 
> 1. there is an out of bounds write access by a miscalculated destination
>    address for the memcpy()
> 2. if iio channels are already represented by default properties, they
>    appear as duplicates in uevent

Thanks, both queued into power-supply-fixes.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]