Re: [PATCH 2/3] soc: apple: Add driver for Apple PMGR misc controls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect check for devm_platform_ioremap_resource_byname() return value leads to ERR_PTR dereference
- [High] Missing NULL check for of_device_get_match_data() causes NULL pointer dereference
--

commit 144dd044ce23e2ad7967536cb164bb8942aedf81
Author: Hector Martin <marcan@marcan.st>

soc: apple: Add driver for Apple PMGR misc controls
    
This commit adds a driver for managing miscellaneous PMGR controls on Apple
SoCs, implementing fabric and memory controller power state switching on system
standby/s2idle to save power.

> diff --git a/drivers/soc/apple/apple-pmgr-misc.c b/drivers/soc/apple/apple-pmgr-misc.c
> new file mode 100644
> index 0000000000000..d487a87ecddb0
> --- /dev/null
> +++ b/drivers/soc/apple/apple-pmgr-misc.c
[ ... ]
> +static bool apple_pmgr_init_device(struct apple_pmgr_misc *misc,
> +				   const struct apple_pmgr_misc_hw *hw,
> +				   enum sys_device dev,
> +				   const char *device_name)
> +{
> +	void __iomem *base;
> +	char name[32];
> +	u32 val;
> +
> +	snprintf(name, sizeof(name), "%s-ps", device_name);
> +
> +	base = devm_platform_ioremap_resource_byname(
> +		to_platform_device(misc->dev), name);
> +	if (!base)

[Severity: High]
Does this code handle the return value of 
devm_platform_ioremap_resource_byname() correctly?

The function returns an error pointer on failure, not NULL. A failure here
would bypass this check and could lead to an ERR_PTR being dereferenced by
the readl_relaxed() call below.

> +		return false;
> +
> +	val = readl_relaxed(base + APPLE_CLKGEN_PSTATE);
> +
> +	misc->devices[dev].base = base;
> +	misc->devices[dev].active_state =
> +		FIELD_GET(APPLE_CLKGEN_PSTATE_DESIRED, val);
> +	misc->devices[dev].suspend_state = hw->dev_min_ps[dev];

[Severity: High]
Can hw be NULL here?

If the driver is bound manually via sysfs without a matching device tree node,
of_device_get_match_data() in apple_pmgr_misc_probe() below returns NULL. This
would lead to a NULL pointer dereference.

> +
> +	return true;
> +}
> +
> +static int apple_pmgr_misc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	const struct apple_pmgr_misc_hw *hw;
> +	struct apple_pmgr_misc *misc;
> +	int ret = -ENODEV;
> +
> +	misc = devm_kzalloc(dev, sizeof(*misc), GFP_KERNEL);
> +	if (!misc)
> +		return -ENOMEM;
> +
> +	misc->dev = dev;
> +	hw = of_device_get_match_data(dev);
> +
> +	if (apple_pmgr_init_device(misc, hw, DEV_FABRIC, "fabric"))

[Severity: High]
Is it possible for hw to be NULL here?

Passing an unchecked hw pointer to apple_pmgr_init_device() causes the
unconditional dereference of hw->dev_min_ps[dev] shown above.

> +		ret = 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-pmgr-misc-v1-0-4f075a3a95c1@chaosmail.tech?part=2