| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c76fe3f8-8884-4769-af69-a9a527d2a202@kernel.org>
Date: Sun, 8 Jun 2025 22:16:06 +0200
From: Hans de Goede <hansg@...nel.org>
To: Sebastian Reichel <sebastian.reichel@...labora.com>,
Sebastian Reichel <sre@...nel.org>, Mark Brown <broonie@...nel.org>,
Linus Walleij <linus.walleij@...aro.org>
Cc: Liam Girdwood <lgirdwood@...il.com>, Vinod Koul <vkoul@...nel.org>,
Kishon Vijay Abraham I <kishon@...nel.org>, Chen-Yu Tsai <wens@...e.org>,
Jernej Skrabec <jernej.skrabec@...il.com>,
Samuel Holland <samuel@...lland.org>,
Matti Vaittinen <mazziesaccount@...il.com>, Pali Rohár
<pali@...nel.org>, Krzysztof Kozlowski <krzk@...nel.org>,
AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/5] power: supply: core: battery-info: fully switch to
fwnode
Hi,
On 8-Jun-25 9:57 PM, Hans de Goede wrote:
> Hi Sebastian,
> On 30-Apr-25 12:54 AM, Sebastian Reichel wrote:
>> Also use fwnode based parsing for "ocv-capacity-celsius" and
>> "resistance-temp-table", so that any DT specific bits are
>> removed from the power-supply core.
>>
>> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
>> Signed-off-by: Sebastian Reichel <sebastian.reichel@...labora.com>
>
> I have been testing this converting the ug3105 driver to
> use power_supply_batinfo_ocv2cap(), replacing the hardcoded
> ocv -> capacity table in that driver.
>
> While testing I hit a bug and while looking closer at this
> patch it needs more work on top of fixing that bug.
>
> See comments inline, also I've attached 3 fixup patches
> which can be squashed into this patch to address the remarks.
>
>> ---
>> drivers/power/supply/power_supply_core.c | 109 ++++++++++++++++++-------------
>> 1 file changed, 63 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
>> index 89947f1fe610d8a75756e1e4e5339b06349f9ab8..a8d1fe66e2486a833ccaa3ed77b861c6e52c5760 100644
>> --- a/drivers/power/supply/power_supply_core.c
>> +++ b/drivers/power/supply/power_supply_core.c
>> @@ -585,32 +585,19 @@ int power_supply_get_battery_info(struct power_supply *psy,
>> {
>> struct power_supply_resistance_temp_table *resist_table;
>> struct power_supply_battery_info *info;
>> - struct device_node *battery_np = NULL;
>> - struct fwnode_reference_args args;
>> - struct fwnode_handle *fwnode = NULL;
>> + struct fwnode_handle *srcnode, *fwnode;
>> const char *value;
>> - int err, len, index;
>> - const __be32 *list;
>> + int err, len, index, proplen;
>> + u32 *propdata;
>
> propname which is also a local-variable for a temporary
> malloc-ed buffer uses __free(kfree) instead of explicit
> kfree() calls IMHO it would be good to do that here too.
>
> This requires declaring it inside the
> "for (index = 0; index < len; index++)" loop like how
> this is done for propname, so that it gets freed on
> every loop iteration.
>
>
>> u32 min_max[2];
>>
>> - if (psy->dev.of_node) {
>> - battery_np = of_parse_phandle(psy->dev.of_node, "monitored-battery", 0);
>> - if (!battery_np)
>> - return -ENODEV;
>> + srcnode = dev_fwnode(&psy->dev);
>> + if (!srcnode && psy->dev.parent)
>> + srcnode = dev_fwnode(psy->dev.parent);
>>
>> - fwnode = fwnode_handle_get(of_fwnode_handle(battery_np));
>> - } else if (psy->dev.parent) {
>> - err = fwnode_property_get_reference_args(
>> - dev_fwnode(psy->dev.parent),
>> - "monitored-battery", NULL, 0, 0, &args);
>> - if (err)
>> - return err;
>> -
>> - fwnode = args.fwnode;
>> - }
>> -
>> - if (!fwnode)
>> - return -ENOENT;
>> + fwnode = fwnode_find_reference(srcnode, "monitored-battery", 0);
>> + if (IS_ERR(fwnode))
>> + return PTR_ERR(fwnode);
>>
>> err = fwnode_property_read_string(fwnode, "compatible", &value);
>> if (err)
>> @@ -740,15 +727,7 @@ int power_supply_get_battery_info(struct power_supply *psy,
>> info->temp_max = min_max[1];
>> }
>>
>> - /*
>> - * The below code uses raw of-data parsing to parse
>> - * /schemas/types.yaml#/definitions/uint32-matrix
>> - * data, so for now this is only support with of.
>> - */
>> - if (!battery_np)
>> - goto out_ret_pointer;
>> -
>> - len = of_property_count_u32_elems(battery_np, "ocv-capacity-celsius");
>> + len = fwnode_property_count_u32(fwnode, "ocv-capacity-celsius");
>> if (len < 0 && len != -EINVAL) {
>> err = len;
>> goto out_put_node;
>> @@ -757,13 +736,13 @@ int power_supply_get_battery_info(struct power_supply *psy,
>> err = -EINVAL;
>> goto out_put_node;
>> } else if (len > 0) {
>> - of_property_read_u32_array(battery_np, "ocv-capacity-celsius",
>> + fwnode_property_read_u32_array(fwnode, "ocv-capacity-celsius",
>> info->ocv_temp, len);
>> }
>>
>> for (index = 0; index < len; index++) {
>> struct power_supply_battery_ocv_table *table;
>> - int i, tab_len, size;
>> + int i, tab_len;
>>
>> char *propname __free(kfree) = kasprintf(GFP_KERNEL, "ocv-capacity-table-%d",
>> index);
>> @@ -772,60 +751,98 @@ int power_supply_get_battery_info(struct power_supply *psy,
>> err = -ENOMEM;
>> goto out_put_node;
>> }
>> - list = of_get_property(battery_np, propname, &size);
>> - if (!list || !size) {
>> + proplen = fwnode_property_count_u32(fwnode, propname);
>> + if (proplen < 0 || proplen % 2 != 0) {
>> dev_err(&psy->dev, "failed to get %s\n", propname);
>> power_supply_put_battery_info(psy, info);
>> err = -EINVAL;
>> goto out_put_node;
>> }
>> + propdata = kcalloc(proplen, sizeof(*propdata), GFP_KERNEL);
>
> As mentioned above I suggest to use the following here instead:
>
> u32 *propdata __free(kfree) = kcalloc(proplen, sizeof(*propdata), GFP_KERNEL);
>
>
>> + if (!propdata) {
>> + kfree(propname);
>
> propname must NOT be free-ed here since it is marked __free(kfree),
> freeing it here will cause a double-free bug.
>
>> + power_supply_put_battery_info(psy, info);
>> + err = -EINVAL;
>> + goto out_put_node;
>> + }
>> + err = fwnode_property_read_u32_array(fwnode, propname, propdata, proplen);
>> + if (err < 0) {
>> + dev_err(&psy->dev, "failed to get %s\n", propname);
>> + kfree(propname);
>
> same as above.
>
>> + kfree(propdata);
>
> with the suggested "u32 *propdata __free(kfree)" this can and must be dropped.
>
>> + power_supply_put_battery_info(psy, info);
>> + goto out_put_node;
>> + }
>>
>> - tab_len = size / (2 * sizeof(__be32));
>> + tab_len = proplen / 2;
>> info->ocv_table_size[index] = tab_len;
>>
>> info->ocv_table[index] = table =
>> devm_kcalloc(&psy->dev, tab_len, sizeof(*table), GFP_KERNEL);
>> if (!info->ocv_table[index]) {
>> + kfree(propdata);
>
> with the suggested "u32 *propdata __free(kfree)" this can and must be dropped.
>
>> power_supply_put_battery_info(psy, info);
>> err = -ENOMEM;
>> goto out_put_node;
>> }
>>
>> for (i = 0; i < tab_len; i++) {
>> - table[i].ocv = be32_to_cpu(*list);
>> - list++;
>> - table[i].capacity = be32_to_cpu(*list);
>> - list++;
>> + table[i].ocv = propdata[i*2];
>> + table[i].capacity = propdata[i*2+1];
>> }
>> +
>> + kfree(propdata);
>
> with the suggested "u32 *propdata __free(kfree)" this can and must be dropped.
>
>> }
>>
>> - list = of_get_property(battery_np, "resistance-temp-table", &len);
>> - if (!list || !len)
>> + proplen = fwnode_property_count_u32(fwnode, "resistance-temp-table");
>
> This will return -EINVAL when the property does not exist, making
> power_supply_get_battery_info() always fail when there is no
> "resistance-temp-table" in the battery fwnode. See the attached fixup
> patch for a suggested fix.
>
>> + if (proplen < 0 || proplen % 2 != 0) {
>> + power_supply_put_battery_info(psy, info);
>> + err = -ENOMEM;
>
> -ENOMEM is the wrong error code here.
>
>> goto out_ret_pointer;
>
> This should be "goto out_put_node" since this is an error path.
>
>> + } else if (proplen == 0) {
>> + goto out_ret_pointer;
>> + }
>>
>> - info->resist_table_size = len / (2 * sizeof(__be32));
>> + propdata = kcalloc(proplen, sizeof(*propdata), GFP_KERNEL);
>
> As mentioned above I suggest to use the following here instead:
>
> u32 *propdata __free(kfree) = kcalloc(proplen, sizeof(*propdata), GFP_KERNEL);
Ok, so this does not work, because we jump over this causing
propdata to not get initialized, but the __free(kfree) still
triggers.
Lesson learned: do not use modern C style mixing of declarations
and statements together with __free().
Attached are updated fixup patches which do not make things crash.
Regards,
Hans
>
>> + if (!propdata) {
>> + power_supply_put_battery_info(psy, info);
>> + err = -ENOMEM;
>> + goto out_ret_pointer;
>
> This should be "goto out_put_node" since this is an error path.
>
>> + }
>> +
>> + err = fwnode_property_read_u32_array(fwnode, "resistance-temp-table",
>> + propdata, proplen);
>> + if (err < 0) {
>> + kfree(propdata);
>
> with the suggested "u32 *propdata __free(kfree)" this can and must be dropped.
>
>> + power_supply_put_battery_info(psy, info);
>> + goto out_put_node;
>> + }
>> +
>> + info->resist_table_size = proplen / 2;
>> info->resist_table = resist_table = devm_kcalloc(&psy->dev,
>> info->resist_table_size,
>> sizeof(*resist_table),
>> GFP_KERNEL);
>> if (!info->resist_table) {
>> + kfree(propdata);
>
> with the suggested "u32 *propdata __free(kfree)" this can and must be dropped.
>
>> power_supply_put_battery_info(psy, info);
>> err = -ENOMEM;
>> goto out_put_node;
>> }
>>
>> for (index = 0; index < info->resist_table_size; index++) {
>> - resist_table[index].temp = be32_to_cpu(*list++);
>> - resist_table[index].resistance = be32_to_cpu(*list++);
>> + resist_table[index].temp = propdata[index*2];
>> + resist_table[index].resistance = propdata[index*2+1];
>> }
>>
>> + kfree(propdata);
>
> with the suggested "u32 *propdata __free(kfree)" this can and must be dropped.
>
>> +
>> out_ret_pointer:
>> /* Finally return the whole thing */
>> *info_out = info;
>>
>> out_put_node:
>> fwnode_handle_put(fwnode);
>> - of_node_put(battery_np);
>> return err;
>> }
>> EXPORT_SYMBOL_GPL(power_supply_get_battery_info);
>>
>
> Regards,
>
> Hans
>
>
View attachment "0004-fixup-power-supply-core-battery-info-fully-switch-to.patch" of type "text/x-patch" (2103 bytes)
View attachment "0005-fixup-power-supply-core-battery-info-fully-switch-to.patch" of type "text/x-patch" (3277 bytes)
Powered by blists - more mailing lists