| From 932d47448c3caa0fa99e84d7f5bc302aa286efd8 Mon Sep 17 00:00:00 2001 |
| From: "H. Nikolaus Schaller" <hns@goldelico.com> |
| Date: Tue, 26 Jun 2018 15:28:29 +0200 |
| Subject: power: generic-adc-battery: fix out-of-bounds write when copying channel properties |
| |
| From: H. Nikolaus Schaller <hns@goldelico.com> |
| |
| commit 932d47448c3caa0fa99e84d7f5bc302aa286efd8 upstream. |
| |
| 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 |
| Fixes: e60fea794e6e ("power: battery: Generic battery driver using IIO") |
| Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| |
| --- |
| drivers/power/supply/generic-adc-battery.c | 14 ++++---------- |
| 1 file changed, 4 insertions(+), 10 deletions(-) |
| |
| --- a/drivers/power/supply/generic-adc-battery.c |
| +++ b/drivers/power/supply/generic-adc-battery.c |
| @@ -243,10 +243,9 @@ static int gab_probe(struct platform_dev |
| 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) { |
| @@ -280,8 +279,6 @@ static int gab_probe(struct platform_dev |
| } |
| |
| 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 |
| @@ -295,15 +292,12 @@ static int gab_probe(struct platform_dev |
| 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; |
| } |
| @@ -314,7 +308,7 @@ static int gab_probe(struct platform_dev |
| * 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)) { |