Re: [RFC PATCH 02/14] platform/x86: add backend for macsmc in intel macs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [Critical] platform/x86: macsmc-acpi: uncanceled work on teardown causes use-after-free
- [High] platform/x86: macsmc-acpi: premature IRQ registration before init in probe
- [High] platform/x86: macsmc-acpi: off-by-one MMIO offsets in get_key_info()
- [Medium] platform/x86: macsmc-acpi: MMIO mapping from ioremap() leaked on teardown
- [Medium] platform/x86: macsmc-acpi: broken status check in apple_smc_acpi_irq()
- [Medium] platform/x86: macsmc-acpi: unlocked SMC write in apple_smc_acpi_resume()
- [Medium] platform/x86: macsmc-acpi: return -EIO instead of negated SMC status byte
- [Medium] platform/x86: macsmc-acpi: broken SENSORS_APPLESMC exclusion and I/O claim
- [Low] platform/x86: macsmc-acpi: duplicate MODULE_DEVICE_TABLE() breaks module build
- [Low] platform/x86: macsmc-acpi: wrong %lX format specifier for size_t len
--

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

platform/x86: add backend for macsmc in intel macs

This commit adds an ACPI backend driver (macsmc-acpi) for the Apple System
Management Controller on Intel Macs, supporting both I/O port and MMIO
access as well as interrupt-driven command completion and event handling.

> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 9ce384e0475ea..ce21277407fca 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -202,6 +202,16 @@ config ADV_SWBUTTON
>  	  To compile this driver as a module, choose M here. The module will
>  	  be called adv_swbutton.
>  
> +config MACSMC_ACPI
> +	tristate "Apple SMC (System Management Controller) ACPI Backend"
> +	depends on MFD_MACSMC_CORE && ACPI && !SENSORS_APPLESMC

[Severity: Medium]
Because SENSORS_APPLESMC is a tristate symbol in drivers/hwmon/Kconfig,
!SENSORS_APPLESMC evaluates to m when CONFIG_SENSORS_APPLESMC=m, which
allows both CONFIG_SENSORS_APPLESMC=m and CONFIG_MACSMC_ACPI=m to be built
in the same configuration.

When both modules are present on an Intel Mac, udev can auto-load both
applesmc (via DMI modalias) and macsmc-acpi (via the APP0001 ACPI
modalias). Because apple_smc_acpi_walk_resources() and
apple_smc_acpi_probe() in drivers/platform/x86/macsmc-acpi.c record
smc->port_base without reserving the I/O port range via
devm_request_region(), applesmc_init()'s request_region(0x300, 32,
"applesmc") will not detect a conflict, allowing both drivers to access the
SMC ports concurrently without shared locking.

Should this dependency be SENSORS_APPLESMC=n, and should macsmc-acpi also
reserve its I/O port and MMIO regions via devm_request_region() and
devm_request_mem_region()?

[ ... ]

> diff --git a/drivers/platform/x86/macsmc-acpi.c b/drivers/platform/x86/macsmc-acpi.c
> new file mode 100644
> index 0000000000000..a06ec921ac320
> --- /dev/null
> +++ b/drivers/platform/x86/macsmc-acpi.c

[ ... ]

> +static int apple_smc_acpi_port_read(struct apple_smc_acpi *smc, u8 cmd, smc_key key,
> +				void *buffer, size_t len)
> +{
> +	int ret;
> +
> +	ret = apple_smc_acpi_port_valid(smc);
> +	if (ret)
> +		return ret;
> +
> +	if (apple_smc_acpi_port_write_cmd(smc, cmd) || apple_smc_acpi_port_write_key(smc, key)) {
> +		dev_dbg(smc->dev, "port/read: read argument failed, key: 0x%X, cmd: 0x%X\n",
> +				key, cmd);
> +		return -EIO;
> +	}
> +
> +	if (apple_smc_acpi_port_write_byte(smc, len)) {
> +		dev_dbg(smc->dev, "port/read: read len failed, key: 0x%X, len: 0x%lX\n", key, len);
> +		return -EIO;
> +	}

[ ... ]

> +static int apple_smc_acpi_port_write(struct apple_smc_acpi *smc, smc_key key, const void *buffer,
> +		size_t len)
> +{
> +	int ret;
> +
> +	ret = apple_smc_acpi_port_valid(smc);
> +	if (ret)
> +		return ret;
> +
> +	if (apple_smc_acpi_port_write_cmd(smc, SMC_MSG_WRITE_KEY) ||
> +			apple_smc_acpi_port_write_key(smc, key)) {
> +		dev_dbg(smc->dev, "port/write: write argument failed, key: 0x%X\n", key);
> +		return -EIO;
> +	}
> +
> +	if (apple_smc_acpi_port_write_byte(smc, len)) {
> +		dev_dbg(smc->dev, "port/write: write len failed, key: 0x%X, len: 0x%lX\n",
> +				key, len);
> +		return -EIO;
> +	}

[Severity: Low]
In both apple_smc_acpi_port_read() and apple_smc_acpi_port_write(), len has
type size_t, which is unsigned int on 32-bit x86 (CONFIG_X86_32). Will
formatting len with 0x%lX instead of 0x%zX trigger a -Wformat warning on
32-bit builds?

[ ... ]

> +static int apple_smc_acpi_mmio_read(struct apple_smc_acpi *smc, u8 cmd, smc_key key,
> +		void *buffer, size_t len)
> +{
> +	u8 ret;
> +
> +	reinit_completion(&smc->cmd_done);
> +	apple_smc_acpi_mmio_clear(smc);
> +	iowrite32be(key, smc->iomem_base + SMC_MMIO_KEY_NAME);
> +	iowrite8(0, smc->iomem_base + SMC_MMIO_KEY_SMC_ID);
> +	iowrite8(cmd, smc->iomem_base + SMC_MMIO_KEY_CMD);
> +
> +	if (apple_smc_acpi_mmio_wait(smc))
> +		return -EIO;
> +
> +	ret = ioread8(smc->iomem_base + SMC_MMIO_KEY_CMD);
> +	if (ret) {
> +		dev_dbg(smc->dev, "mmio/read: read failed, ret: 0x%X, key: 0x%X\n", ret, key);
> +		return -ret;
> +	}
> +
> +	memcpy_fromio(buffer, smc->iomem_base + SMC_MMIO_KEY_DATA, len);
> +	return 0;
> +}
> +
> +static int apple_smc_acpi_mmio_write(struct apple_smc_acpi *smc, smc_key key, const void *buffer,
> +		size_t len)
> +{
> +	u8 ret;
> +
> +	reinit_completion(&smc->cmd_done);
> +	apple_smc_acpi_mmio_clear(smc);
> +	iowrite32be(key, smc->iomem_base + SMC_MMIO_KEY_NAME);
> +	memcpy_toio(smc->iomem_base + SMC_MMIO_KEY_DATA, buffer, len);
> +	iowrite8(len, smc->iomem_base + SMC_MMIO_KEY_DATA_LEN);
> +	iowrite8(0, smc->iomem_base + SMC_MMIO_KEY_SMC_ID);
> +	iowrite8(SMC_MSG_WRITE_KEY, smc->iomem_base + SMC_MMIO_KEY_CMD);
> +
> +	if (apple_smc_acpi_mmio_wait(smc))
> +		return -EIO;
> +
> +	ret = ioread8(smc->iomem_base + SMC_MMIO_KEY_CMD);
> +	if (ret) {
> +		dev_dbg(smc->dev, "mmio/write: write failed, ret: 0x%X, key: 0x%X\n", ret, key);
> +		return -ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int apple_smc_acpi_mmio_get_key_info(struct apple_smc_acpi *smc, smc_key key,
> +					struct apple_smc_key_info *info)
> +{
> +	u8 ret;
> +
> +	reinit_completion(&smc->cmd_done);
> +	apple_smc_acpi_mmio_clear(smc);
> +	iowrite32be(key, smc->iomem_base + SMC_MMIO_KEY_NAME);
> +	iowrite8(0, smc->iomem_base + SMC_MMIO_KEY_SMC_ID);
> +	iowrite8(SMC_MSG_GET_KEY_INFO, smc->iomem_base + SMC_MMIO_KEY_CMD);
> +
> +	if (apple_smc_acpi_mmio_wait(smc))
> +		return -EIO;
> +
> +	ret = ioread8(smc->iomem_base + SMC_MMIO_KEY_CMD);
> +	if (ret) {
> +		dev_dbg(smc->dev, "mmio/get_key_info failed, ret: 0x%X, key: 0x%X\n", ret, key);
> +		return -ret;
> +	}

[Severity: Medium]
In apple_smc_acpi_mmio_read(), apple_smc_acpi_mmio_write(), and
apple_smc_acpi_mmio_get_key_info(), ret is the 8-bit hardware SMC status
code read from SMC_MMIO_KEY_CMD (for example, 0x84 for key not found).

Returning -ret propagates a negated hardware status code rather than a
standard Linux errno value to callers of apple_smc_read(),
apple_smc_write(), apple_smc_get_key_by_index(), and
apple_smc_get_key_info(). Should these paths return -EIO on non-zero SMC
status, matching the PIO helpers and apple_smc_rtkit_cmd_locked()?

> +
> +
> +	if (!info)
> +		return 0;
> +	info->type_code = cpu_to_be32(ioread32(smc->iomem_base + SMC_MMIO_KEY_DATA));
> +	info->size = ioread8(smc->iomem_base + 5);
> +	info->flags = ioread8(smc->iomem_base + 6);
> +
> +	return 0;
> +}

[Severity: High]
In apple_smc_acpi_mmio_get_key_info(), ioread32(smc->iomem_base +
SMC_MMIO_KEY_DATA) reads the 4-byte type_code from offsets 0 through 3.

Doesn't reading info->size from smc->iomem_base + 5 and info->flags from
smc->iomem_base + 6 skip offset 4, placing the flags byte into info->size
and an uninitialized byte from offset 6 into info->flags? Shouldn't these
reads use offsets 4 and 5?

[ ... ]

> +static irqreturn_t apple_smc_acpi_irq(int irq, void *data)
> +{
> +	struct apple_smc_acpi *smc = data;
> +	uint8_t status = 0, int_id = 0;
> +	unsigned long flags;
> +
> +	if (smc->is_mmio) {
> +		status = ioread8(smc->iomem_base + SMC_MMIO_KEY_INTERRUPT_STATUS);
> +		if ((status & 0x20) == 0 && status != 0)
> +			return IRQ_HANDLED;
> +	} else {
> +		status = inb(smc->port_base + SMC_IO_CMD_PORT);
> +		if ((status & 0x40) == 0 && status != 0)
> +			return IRQ_HANDLED;
> +	}
> +
> +	int_id = inb(smc->port_base + 0x1F);

[Severity: Medium]
In apple_smc_acpi_irq(), if status reads as 0 (no interrupt pending), the
condition ((status & 0x20) == 0 && status != 0) evaluates to false and
falls through to read inb(smc->port_base + 0x1F). Also, when the interrupt
status bit is not set, returning IRQ_HANDLED reports an unhandled interrupt
as handled, and in PIO mode checking (status & 0x40) == 0 can drop command
completion interrupts where bit 0x40 is not set.

Should apple_smc_acpi_irq() return IRQ_NONE when no SMC interrupt is
pending?

[ ... ]

> +static acpi_status apple_smc_acpi_walk_resources(struct acpi_resource *res,
> +	void *data)
> +{
> +	struct apple_smc_acpi *smc = data;
> +	int ret = 0;
> +
> +	switch (res->type) {

[ ... ]

> +	case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
> +		if (!smc->is_mmio) {
> +			if (res->data.fixed_memory32.address_length < SMC_MMIO_MIN_SIZE) {
> +				dev_dbg(smc->dev, "found mmio support but its too small: %u\n",
> +					res->data.fixed_memory32.address_length);
> +				return AE_OK;
> +			}
> +			smc->iomem_base_addr = res->data.fixed_memory32.address;
> +			smc->iomem_base_size = res->data.fixed_memory32.address_length;
> +			smc->is_mmio = true;
> +		}
> +		return AE_OK;
> +
> +	case ACPI_RESOURCE_TYPE_IRQ:
> +		ret = devm_request_irq(smc->dev, res->data.irq.interrupts[0],
> +					apple_smc_acpi_irq,
> +					0, "macsmc-acpi", smc);
> +		if (ret < 0) {
> +			dev_dbg(smc->dev, "Unable to register interrupts\n");
> +			smc->has_interrupt = false;
> +			return AE_OK;
> +		}
> +		smc->has_interrupt = true;
> +		return AE_OK;

[Severity: High]
During apple_smc_acpi_probe() -> acpi_walk_resources() ->
apple_smc_acpi_walk_resources(), ACPI_RESOURCE_TYPE_FIXED_MEMORY32 sets
smc->is_mmio = true while smc->iomem_base is still NULL, and
ACPI_RESOURCE_TYPE_IRQ immediately registers and enables
apple_smc_acpi_irq() via devm_request_irq().

At that point in apple_smc_acpi_probe(), apple_smc_acpi_try_enable_mmio()
has not yet mapped smc->iomem_base, and smc->event_handlers,
smc->shutdown_work, smc->notifier_work, smc->event_lock, and
smc->event_fifo have not been initialized yet:

apple_smc_acpi_probe()
  acpi_walk_resources(..., apple_smc_acpi_walk_resources, smc)
    smc->is_mmio = true
    devm_request_irq(..., apple_smc_acpi_irq, ...)
  apple_smc_acpi_try_enable_mmio(smc)
  BLOCKING_INIT_NOTIFIER_HEAD(&smc->event_handlers)
  INIT_WORK(&smc->shutdown_work, ...)
  INIT_WORK(&smc->notifier_work, ...)
  spin_lock_init(&smc->event_lock)
  INIT_KFIFO(smc->event_fifo)

Similarly, if MMIO validation fails in apple_smc_acpi_try_enable_mmio(),
iounmap(smc->iomem_base) is called before smc->is_mmio is set to false.

If an interrupt fires during this window (for example, if NTOK was left
enabled across a warm reboot/module reload, or during the LDKN read in
apple_smc_acpi_try_enable_mmio()), won't apple_smc_acpi_irq() dereference a
NULL or unmapped smc->iomem_base and access uninitialized spinlock, kfifo,
and work_struct fields?

[ ... ]

> +static void apple_smc_acpi_try_enable_mmio(struct apple_smc_acpi *smc)
> +{
> +	if (!smc->is_mmio)
> +		return;
> +
> +	u8 ldkn;
> +
> +	smc->iomem_base = ioremap(smc->iomem_base_addr, smc->iomem_base_size);
> +	if (!smc->iomem_base)
> +		goto out;
> +
> +	if (ioread8(smc->iomem_base + SMC_MMIO_KEY_STATUS) == 0xFF) {
> +		dev_dbg(smc->dev, "mmio enable failed: init status is 0xff\n");
> +		goto out_iomem;
> +	}
> +
> +	if (apple_smc_acpi_mmio_read(smc, SMC_MSG_READ_KEY, SMC_KEY(LDKN), &ldkn, 1)) {
> +		dev_dbg(smc->dev, "mmio enable failed: ldkn read failed\n");
> +		goto out_iomem;
> +	}
> +
> +	if (ldkn < 2) {
> +		dev_dbg(smc->dev, "mmio enable failed: ldkn version less than 2\n");
> +		goto out_iomem;
> +	}
> +	return;
> +
> +out_iomem:
> +	iounmap(smc->iomem_base);
> +out:
> +	smc->is_mmio = false;
> +}

[Severity: Medium]
When apple_smc_acpi_try_enable_mmio() succeeds, smc->iomem_base remains
mapped with unmanaged ioremap(), and nothing calls iounmap() if
apple_smc_probe() subsequently fails or when the driver is unbound.

Does this leak the smc->iomem_base MMIO mapping on teardown, and should
devm_ioremap() (with devm_iounmap() on fallback) be used instead?

[ ... ]

> +static int apple_smc_acpi_probe(struct platform_device *pdev)
> +{
> +	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> +	struct apple_smc_acpi *smc;
> +	acpi_status status;
> +
> +	smc = devm_kzalloc(&pdev->dev, sizeof(*smc), GFP_KERNEL);
> +	if (!smc)
> +		return -ENOMEM;
> +
> +	smc->dev = &pdev->dev;
> +	init_completion(&smc->cmd_done);
> +	status = acpi_walk_resources(adev->handle, METHOD_NAME__CRS,
> +				apple_smc_acpi_walk_resources, smc);
> +
> +	if (ACPI_FAILURE(status))
> +		return -ENODEV;
> +
> +	apple_smc_acpi_try_enable_mmio(smc);
> +	BLOCKING_INIT_NOTIFIER_HEAD(&smc->event_handlers);
> +	INIT_WORK(&smc->shutdown_work, apple_smc_acpi_shutdown_work);
> +	INIT_WORK(&smc->notifier_work, apple_smc_acpi_notifier_work);
> +	spin_lock_init(&smc->event_lock);
> +	INIT_KFIFO(smc->event_fifo);
> +	apple_smc_acpi_try_enable_interrupts(smc);
> +
> +	return apple_smc_probe(smc->dev, &apple_smc_acpi_be_ops, smc, &smc->event_handlers, true);
> +}

[Severity: Critical]
In apple_smc_acpi_probe(), smc->shutdown_work and smc->notifier_work are
initialized with INIT_WORK(), and apple_smc_acpi_irq() schedules them via
schedule_work(). However, neither work item is ever cancelled if
apple_smc_probe() fails or when the device is unbound (and if
apple_smc_probe() fails before registering apple_smc_disable_notifications,
NTOK remains enabled).

Once devres frees struct apple_smc_acpi on teardown, can a pending or
running apple_smc_acpi_notifier_work() access freed memory via
smc->event_lock, smc->event_fifo, and smc->event_handlers? Could
devm_work_autocancel() be used here, and should interrupts be disabled and
synchronized before cancelling work on teardown?

> +
> +static int apple_smc_acpi_resume(struct device *dev)
> +{
> +	struct apple_smc *smc = dev_get_drvdata(dev);
> +	struct apple_smc_acpi *smc_acpi = smc->be_cookie;
> +
> +	/* NTOK may be set to 0 on resume */
> +	if (smc_acpi->interrupt_enabled) {
> +		smc_acpi->interrupt_enabled = 0;
> +		apple_smc_acpi_try_enable_interrupts(smc_acpi);
> +	}
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops apple_smc_acpi_ops = {
> +	.resume = apple_smc_acpi_resume,
> +};

[Severity: Medium]
In apple_smc_acpi_resume(), apple_smc_acpi_try_enable_interrupts() writes
the NTOK key to the SMC hardware and updates smc_acpi->interrupt_enabled
without holding smc->mutex.

If a child device or worker performs an SMC transaction concurrently during
resume, can these unsynchronized accesses race on the MMIO/PIO registers
and smc_acpi->cmd_done? Also, should apple_smc_acpi_ops use
DEFINE_SIMPLE_DEV_PM_OPS() with pm_sleep_ptr() so hibernation restore and
CONFIG_PM_SLEEP=n builds are handled properly?

> +
> +static const struct acpi_device_id apple_smc_acpi_ids[] = {
> +	{"APP0001", 0},
> +	{"", 0},
> +};
> +MODULE_DEVICE_TABLE(acpi, apple_smc_acpi_ids);
> +
> +static struct platform_driver apple_smc_acpi_driver = {
> +	.probe = apple_smc_acpi_probe,
> +	.driver = {
> +		.name = "macsmc-acpi",
> +		.acpi_match_table = apple_smc_acpi_ids,
> +		.pm = &apple_smc_acpi_ops,
> +	},
> +};
> +module_platform_driver(apple_smc_acpi_driver);
> +
> +MODULE_AUTHOR("Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>");
> +MODULE_LICENSE("Dual MIT/GPL");
> +MODULE_DESCRIPTION("Apple SMC ACPI backend");
> +MODULE_DEVICE_TABLE(acpi, apple_smc_acpi_ids);

[Severity: Low]
MODULE_DEVICE_TABLE(acpi, apple_smc_acpi_ids) is invoked twice in
drivers/platform/x86/macsmc-acpi.c (and linux/module.h is not explicitly
included). When building with CONFIG_MACSMC_ACPI=m, doesn't the duplicate
MODULE_DEVICE_TABLE() macro fail to compile due to redefining
__mod_device_table__kmod_macsmc_acpi__acpi__apple_smc_acpi_ids?

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