Re: [RFC PATCH 10/14] power/supply: add support for x86 macs in macsmc-power

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] power: supply: macsmc: NULL deref in macsmc_power_remove() without battery
- [High] power: supply: macsmc: notify handler UAF and ref leak in macsmc_power_probe()
- [High] power: supply: macsmc: unchecked power supplies in macsmc_power_acpi_notify()
- [Medium] power: supply: macsmc: uninitialized adapter_status in macsmc_power_probe()
- [Medium] power: supply: macsmc: missing swab16() for B0TE on ACPI Macs
- [Medium] power: supply: macsmc: missing else in macsmc_power_probe() battery check
- [Medium] power: supply: macsmc: 32-bit overflow in macsmc_battery_acpi_read()
- [Low] power: supply: macsmc: build failure when CONFIG_ACPI is disabled
--

commit 14a9bd57ae09c9ca5072f486dd3745277286ab3b
Author: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>

power/supply: add support for x86 macs in macsmc-power

This commit adds support for Intel Macs to the macsmc-power driver,
exposing battery and AC adapter sensors as well as charge limit controls
on ACPI-based Apple systems.

> diff --git a/drivers/power/supply/macsmc-power.c b/drivers/power/supply/macsmc-power.c
> index ced37a7019981..1d64d8185ed7b 100644
> --- a/drivers/power/supply/macsmc-power.c
> +++ b/drivers/power/supply/macsmc-power.c

[ ... ]

> @@ -322,6 +362,198 @@ static int macsmc_battery_get_capacity_level(struct macsmc_power *power)

[ ... ]

> +static int macsmc_battery_acpi_read(struct apple_smc *smc, smc_key key, int *val,
> +	int scale)
> +{
> +	int ret;
> +	struct apple_smc_key_info info;
> +
> +	ret = apple_smc_get_key_info(smc, key, &info);
> +	if (ret)
> +		return ret;
> +
> +	switch (info.type_code) {
> +	/* 32-bit IEEE 754 float */
> +	case __SMC_KEY('f', 'l', 't', ' '): {
> +		int flt_ = 0;
> +
> +		ret = macsmc_battery_acpi_read_f32_scaled(smc, key, &flt_, scale);
> +		if (ret)
> +			return ret;
> +
> +		*val = flt_;
> +		break;
> +	}
> +
> +	/* 3.13 fixed point decimal */
> +	case __SMC_KEY('f', 'p', '3', 'd'): {
> +		u16 fp3d;
> +
> +		ret = apple_smc_read_u16(smc, key, &fp3d);
> +		if (ret)
> +			return ret;
> +
> +		*val = mult_frac(be16_to_cpu(fp3d), scale, BIT(13));
> +		break;
> +	}

[ ... ]

> +	/* 3.12 signed fixed point decimal */
> +	case __SMC_KEY('s', 'p', '3', 'c'): {
> +		s16 sp3c;
> +
> +		ret = apple_smc_read_s16(smc, key, &sp3c);
> +		if (ret)
> +			return ret;
> +
> +		*val = mult_frac((s16)be16_to_cpu(sp3c), scale, BIT(12));
> +		break;
> +	}

[Severity: Medium]
Can the mult_frac() calls for the fp3d and sp3c types overflow a 32-bit
signed int when scale is 1000000?

Inside mult_frac(x, n, d):

    typeof(x_) q = x_ / d_;
    typeof(x_) r = x_ % d_;
    q * n_ + r * n_ / d_;

Because x is u16 or s16 and scale is int (1000000), r * n_ is evaluated as
a 32-bit signed int multiplication before being converted to unsigned long
for division by BIT(13) (8192) or BIT(12) (4096). Whenever the fractional
remainder r is 2148 or larger, r * 1000000 exceeds INT_MAX and wraps around,
producing corrupted (and often negative) current, voltage, or power readings.

[ ... ]

> @@ -359,36 +594,71 @@ static int macsmc_battery_get_property(struct power_supply *psy,
>  		break;
>  	case POWER_SUPPLY_PROP_TIME_TO_FULL_NOW:
>  		ret = apple_smc_read_u16(power->smc, SMC_KEY(B0TF), &vu16);
> -		val->intval = vu16 == 0xffff ? 0 : vu16 * 60;
> +		if (power->smc->is_acpi)
> +			val->intval = vu16 == 0xffff ? 0 : swab16(vu16) * 60;
> +		else
> +			val->intval = vu16 == 0xffff ? 0 : vu16 * 60;
>  		break;

[Severity: Medium]
In macsmc_battery_get_property(), POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW right
above this case reads SMC_KEY(B0TE) into vu16 without byte-swapping when
power->smc->is_acpi is true:

	case POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW:
		ret = apple_smc_read_u16(power->smc, SMC_KEY(B0TE), &vu16);
		val->intval = vu16 == 0xffff ? 0 : vu16 * 60;
		break;

Since macsmc_power_probe() registers POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW on
ACPI Macs when B0TE is present, should B0TE also apply swab16(vu16) on ACPI
systems just like B0TF does here?

[ ... ]

> @@ -634,6 +1003,23 @@ static int macsmc_power_event(struct notifier_block *nb, unsigned long event, vo
>  	return NOTIFY_DONE;
>  }
>  
> +static void macsmc_power_acpi_notify(acpi_handle handle, u32 event, void *context)
> +{
> +	struct macsmc_power *power = context;
> +
> +	if (event == 0x80) {
> +		acpi_status status = acpi_evaluate_integer(power->adapter->handle, "_PSR", NULL,
> +								&power->adapter_status);
> +		if (ACPI_FAILURE(status))
> +			return;
> +
> +		power->adapter_status = !!power->adapter_status;
> +		power_supply_changed(power->batt);
> +		power_supply_changed(power->ac);
> +	}
> +}

[Severity: High]
What happens in macsmc_power_acpi_notify() if either power->batt or
power->ac failed to register during macsmc_power_probe()?

In macsmc_power_probe(), if devm_power_supply_register() fails for one of
the supplies, the failed pointer is set to NULL and probe continues as long
as the other supply succeeded. Unlike macsmc_power_event(), which checks
if (power->batt) and if (power->ac), macsmc_power_acpi_notify() passes both
pointers unconditionally to power_supply_changed(), which immediately
dereferences the pointer.

> +
> +
>  static int macsmc_power_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -670,15 +1056,19 @@ static int macsmc_power_probe(struct platform_device *pdev)
>  	 * Check for battery presence.
>  	 * B0AV is a fundamental key.
>  	 */
> +	if (smc->is_acpi && apple_smc_read_flag(power->smc, SMC_KEY(BBIN), &flag) == 0 && flag)
> +		has_battery = true;
> +
>  	if (apple_smc_read_u16(power->smc, SMC_KEY(B0AV), &vu16) == 0 &&
>  	    macsmc_battery_get_status(power) > POWER_SUPPLY_STATUS_UNKNOWN)
>  		has_battery = true;

[Severity: Medium]
Should the second battery check be an else if (!smc->is_acpi && ...) branch?

As written, ACPI Macs always fall through to the second check and call
macsmc_battery_get_status(power), which queries Apple Silicon SMC keys
(CH0R, CHCE, CHCC, AC-i, BSFC, CHNC, CHSC) even when smc->is_acpi is true
and BBIN indicated no battery is present.

[ ... ]

> @@ -858,9 +1297,26 @@ static int macsmc_power_probe(struct platform_device *pdev)
>  	if (!power->batt && !power->ac)
>  		return -ENODEV;
>  
> -	power->nb.notifier_call = macsmc_power_event;
> -	blocking_notifier_chain_register(smc->event_handlers, &power->nb);
> +	if (!smc->is_acpi) {
> +		power->nb.notifier_call = macsmc_power_event;
> +		blocking_notifier_chain_register(smc->event_handlers, &power->nb);
> +	} else if (power->has_battery) {
> +		power->adapter = acpi_dev_get_first_match_dev("ACPI0003", NULL, -1);
> +		if (!power->adapter)
> +			return -ENODEV;
>  
> +		ret = acpi_dev_install_notify_handler(power->adapter, ACPI_DEVICE_NOTIFY,
> +					macsmc_power_acpi_notify, power);
> +		if (ret)
> +			return ret;
> +
> +		acpi_status status = acpi_evaluate_integer(power->adapter->handle, "_PSR", NULL,
> +					&power->adapter_status);
> +		if (ACPI_FAILURE(status))
> +			return -EIO;
> +
> +		power->adapter_status = !!power->adapter_status;
> +	}
>  	return 0;
>  }

[Severity: High]
If acpi_evaluate_integer() fails here in macsmc_power_probe(), does this
leave macsmc_power_acpi_notify() installed with a freed power context
pointer?

acpi_dev_install_notify_handler() is not devm-managed, so returning -EIO
causes devres to free power while the ACPI notify handler remains registered
on power->adapter; a subsequent 0x80 notify event will then dereference
freed memory in macsmc_power_acpi_notify(). In addition,
acpi_dev_get_first_match_dev() takes a reference on power->adapter via
get_device(), which is never released with acpi_dev_put() on these error
paths or in macsmc_power_remove().

[Severity: Medium]
Also in macsmc_power_probe(), devm_power_supply_register() is called for
both power->batt and power->ac before power->adapter is looked up and before
_PSR is evaluated into power->adapter_status.

Because __power_supply_register() queries the supply properties during
registration, won't the initial reads of POWER_SUPPLY_PROP_STATUS in
macsmc_battery_acpi_get_status() and POWER_SUPPLY_PROP_ONLINE in
macsmc_ac_get_property() always observe power->adapter_status as 0 even when
AC power is connected? Furthermore, evaluating _PSR directly into
power->adapter_status and normalizing it in place afterwards allows
concurrent property reads to observe the unnormalized value.

>  
>  static void macsmc_power_remove(struct platform_device *pdev)
>  {
>  	struct macsmc_power *power = dev_get_drvdata(&pdev->dev);
>  
> -	blocking_notifier_chain_unregister(power->smc->event_handlers, &power->nb);
> +	if (!power->smc->is_acpi)
> +		blocking_notifier_chain_unregister(power->smc->event_handlers, &power->nb);
> +	else
> +		acpi_dev_remove_notify_handler(power->adapter, ACPI_DEVICE_NOTIFY,
> +				macsmc_power_acpi_notify);
>  }

[Severity: High]
On desktop Intel Macs without a battery (where power->smc->is_acpi is true
and power->has_battery is false), power->adapter is never initialized in
macsmc_power_probe() and remains NULL.

When macsmc_power_remove() runs on those machines, won't passing
power->adapter to acpi_dev_remove_notify_handler() cause a NULL pointer
dereference when it accesses adev->handle?

[Severity: Low]
Finally, does this driver still build when CONFIG_ACPI is disabled (for
example, on arm64 Apple Silicon configs with CONFIG_ACPI=n)?

In include/acpi/acpi_bus.h, the full definition of struct acpi_device as
well as acpi_evaluate_integer(), acpi_dev_install_notify_handler(), and
acpi_dev_remove_notify_handler() are only provided under #ifdef CONFIG_ACPI
without !CONFIG_ACPI stubs, so dereferencing power->adapter->handle and
calling those functions in macsmc_power_acpi_notify(), macsmc_power_probe(),
and macsmc_power_remove() will fail to compile when CONFIG_ACPI is not
enabled.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029085504.3547-1-atharvatiwarilinuxdev@gmail.com?part=10