Re: [PATCH 2/5] power: supply: add Qualcomm QGauge (qcom_qg) fuel gauge driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] power: supply: qcom_qg: UAF of batt_info in qcom_qg_probe()
  teardown
- [Medium] power: supply: qcom_qg: temperature unit mismatch in
  qg_get_capacity()
- [Medium] power: supply: qcom_qg: broken temperature threshold checks
  in qg_get_health()
- [Medium] power: supply: qcom_qg: unhandled -EINVAL design voltage
  limits in health/props
--

commit 23cbeedd29303e2809a8e496524a3641be8ea9bc
Author: Marc Lainez <marc.lainez@gmail.com>

power: supply: add Qualcomm QGauge (qcom_qg) fuel gauge driver

This commit adds a power_supply driver for the Qualcomm QGauge (QG) fuel
gauge hardware block found in SPMI PMICs such as PMI632 and PM6150.

> diff --git a/drivers/power/supply/qcom_qg.c b/drivers/power/supply/qcom_qg.c
> new file mode 100644
> index 0000000000000..071e29d4c2991
> --- /dev/null
> +++ b/drivers/power/supply/qcom_qg.c

[ ... ]

> +static int qg_get_capacity(struct qcom_qg *qg, int *capacity)
> +{
> +	int temp_decidegc;
> +	int cap, ret;
> +
> +	if (!qg->batt_info)
> +		return -ENODATA;
> +
> +	/*
> +	 * Without OCV, it's impossible to calculate state-of-charge.
> +	 * Well, you could try, but you'd get *massive* load swings.
> +	 */
> +	if (!qg->ocv_uv)
> +		return -ENODATA;
> +
> +	ret = qg_read_temperature(qg, &temp_decidegc);
> +	if (ret)
> +		temp_decidegc = 250; /* default 25.0 degC */
> +
> +	cap = power_supply_batinfo_ocv2cap(qg->batt_info, qg->ocv_uv,
> +					   temp_decidegc);

[Severity: Medium]
Does passing temp_decidegc to power_supply_batinfo_ocv2cap() use the wrong
temperature unit?

qg_read_temperature() and the fallback value return tenths of a degree
Celsius (e.g. 250 for 25.0 degC), whereas power_supply_get_battery_info()
populates info->ocv_temp[] from ocv-capacity-celsius in whole degrees
Celsius.  When multiple temperature OCV tables are defined,
power_supply_find_ocv2cap_table() compares decidegrees against degrees and
selects the wrong table.

[ ... ]

> +static int qg_get_health(struct qcom_qg *qg, int *health)
> +{
> +	struct power_supply_battery_info *info = qg->batt_info;
> +	int temp_decidegc;
> +	int ret;
> +
> +	if (!qg->batt_present) {
> +		*health = POWER_SUPPLY_HEALTH_UNKNOWN;
> +		return 0;
> +	}
> +
> +	if (info) {
> +		ret = qg_read_temperature(qg, &temp_decidegc);
> +
> +		/* No thermistor is not a health fault */
> +		if (ret && ret != -ENODEV)
> +			return ret;
> +
> +		if (!ret) {
> +			/* Unset thresholds stay 0; 0 degC is a valid reading. */
> +			if (info->temp_alert_max > 0 &&
> +			    temp_decidegc >= info->temp_alert_max) {
> +				*health = POWER_SUPPLY_HEALTH_HOT;
> +				return 0;
> +			}
> +			if (info->temp_alert_min > 0 &&
> +			    temp_decidegc <= info->temp_alert_min) {
> +				*health = POWER_SUPPLY_HEALTH_COLD;
> +				return 0;
> +			}
> +			if (info->temp_max > 0 &&
> +			    temp_decidegc >= info->temp_max) {
> +				*health = POWER_SUPPLY_HEALTH_WARM;
> +				return 0;
> +			}
> +			if (info->temp_min > 0 &&
> +			    temp_decidegc <= info->temp_min) {
> +				*health = POWER_SUPPLY_HEALTH_COOL;
> +				return 0;
> +			}
> +		}
> +	}

[Severity: Medium]
Are the temperature threshold checks in qg_get_health() comparing
decidegrees against whole degrees and using the wrong sentinel values?

power_supply_get_battery_info() initializes unset info->temp_alert_min and
info->temp_min to INT_MIN and unset info->temp_alert_max and info->temp_max
to INT_MAX, and populates them from alert-celsius and
operating-range-celsius in whole degrees Celsius.

Because temp_decidegc is in tenths of a degree Celsius and unset max limits
are INT_MAX (which satisfies > 0), any room-temperature reading (e.g. 250
for 25.0 degC) is compared directly against a whole-degree limit (e.g. 45)
and falsely reports POWER_SUPPLY_HEALTH_HOT.  Checking > 0 on
temp_alert_min and temp_min also ignores any cold threshold at or below
0 degC.  In addition, temp_min and temp_max represent the outer operating
range (COLD/HOT) while temp_alert_min and temp_alert_max are the inner
alert thresholds (COOL/WARM), so their health states are swapped here.

> +
> +	*health = POWER_SUPPLY_HEALTH_GOOD;
> +
> +	if (info && qg->vbat_uv) {
> +		if (qg->vbat_uv < info->voltage_min_design_uv)
> +			*health = POWER_SUPPLY_HEALTH_DEAD;
> +		else if (qg->vbat_uv > info->voltage_max_design_uv)
> +			*health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +	case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
> +		if (!qg->batt_info)
> +			return -ENODATA;
> +		val->intval = qg->batt_info->voltage_max_design_uv;
> +		return 0;
> +
> +	case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
> +		if (!qg->batt_info)
> +			return -ENODATA;
> +		val->intval = qg->batt_info->voltage_min_design_uv;
> +		return 0;

[Severity: Medium]
What happens in qg_get_health() and qg_get_property() when
voltage-min-design-microvolt or voltage-max-design-microvolt is omitted from
the battery node?

power_supply_get_battery_info() initializes info->voltage_min_design_uv and
info->voltage_max_design_uv to -EINVAL.  In qg_get_health(), comparing
unsigned int qg->vbat_uv against signed int info->voltage_min_design_uv
promotes -EINVAL to UINT_MAX - 21, so qg->vbat_uv <
info->voltage_min_design_uv is always true and reports
POWER_SUPPLY_HEALTH_DEAD.  In qg_get_property(),
POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN and
POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN return 0 with val->intval = -22 instead
of returning -ENODATA when those fields are negative (unlike
POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN just below).

[ ... ]

> +static void qg_batt_info_cleanup(void *data)
> +{
> +	struct qcom_qg *qg = data;
> +
> +	power_supply_put_battery_info(qg->psy, qg->batt_info);
> +}
> +
> +static int qcom_qg_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	qg->psy = devm_power_supply_register(dev, &qg_psy_desc, &psy_cfg);
> +	if (IS_ERR(qg->psy))
> +		return dev_err_probe(dev, PTR_ERR(qg->psy),
> +				     "Failed to register power supply");
> +
> +	ret = power_supply_get_battery_info(qg->psy, &qg->batt_info);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get battery info");
> +
> +	ret = devm_add_action_or_reset(dev, qg_batt_info_cleanup, qg);
> +	if (ret)
> +		return ret;

[Severity: High]
Can qg_get_property() dereference qg->batt_info after it has been freed
during driver unbind or probe unwind?

devm_add_action_or_reset(dev, qg_batt_info_cleanup, qg) is registered on
dev after devm_power_supply_register().  Because devres releases resources
in reverse order, on driver unbind or if any of the subsequent
qg_get_optional_irq() calls fails, qg_batt_info_cleanup() frees
qg->batt_info and its ocv_table[] arrays while qg->psy is still registered
and qg->batt_info remains non-NULL.  Any sysfs read or pending uevent work
flushed during power_supply_unregister() then calls qg_get_property() and
reads freed memory.  Calling power_supply_get_battery_info() after
devm_power_supply_register() also leaves qg->batt_info NULL while qg->psy
is first exposed to userspace and thermal/hwmon consumers.

Note that because qg_psy_desc.type is POWER_SUPPLY_TYPE_BATTERY,
__power_supply_register() already populates psy->battery_info before
device_add() and frees it when psy->dev is unregistered, or if
qg->batt_info is kept, its cleanup action only runs safely if unregistered
after qg->psy (or if qg->batt_info is allocated on &qg->psy->dev without a
separate dev action that runs before power_supply_unregister()).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112170841.2189322-1-val@packett.cool?part=2