Re: [PATCH v01] mailbox/pcc.c: add query channel function

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Medium] Uninitialized `mchan` field in the user-provided output structure.
- [Low] Dead code in `IS_ERR(chan)` check provides a false sense of security.
- [Low] Commented-out `#include` directives left in a public header file.

Pre-existing issues:
- [High] Use-After-Free (UAF) vulnerabilities due to missing driver teardown on unbind.
--

commit a70542fb899e92a284eaf8ce05841d6088a0dece
Author: Adam Young <admiyo@os.amperecomputing.com>

mailbox/pcc.c: add query channel function

This commit adds a new function to allow drivers to query PCC channel
information before creating a channel. It also exposes the PCC channel type
to the client by adding it to the channel records.

> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 636879ae1db76..30577c6beefaa 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
[ ... ]
> +int
> +pcc_mbox_query_channel(struct pcc_mbox_chan *q_chan, int subspace_id)
> +{
> +	struct pcc_mbox_chan *pcc_mchan;
> +	struct pcc_chan_info *pchan;
> +	struct mbox_chan *chan;
> +
> +	if (subspace_id < 0 || subspace_id >= pcc_chan_count)
> +		return -ENOENT;
> +	pchan = chan_info + subspace_id;
> +	chan = pchan->chan.mchan;
> +	if (IS_ERR(chan)) {

[Severity: Low]
Is this check effective? Since chan is initialized in pcc_mbox_probe() as the
address of an element within a devm_kcalloc allocated array
(&pcc_mbox_channels[i]), it seems it can never be an ERR_PTR. Also, IS_ERR()
would evaluate to false for a NULL pointer.

> +		pr_err("Channel not found for idx: %d\n", subspace_id);
> +		return -EBUSY;
> +	}
> +	pcc_mchan = &pchan->chan;
> +
> +	q_chan->shmem_base_addr = pcc_mchan->shmem_base_addr;
> +	q_chan->shmem = NULL;
> +	q_chan->shmem_size = pcc_mchan->shmem_size;
> +	q_chan->latency = pcc_mchan->latency;
> +	q_chan->max_access_rate = pcc_mchan->max_access_rate;
> +	q_chan->min_turnaround_time = pcc_mchan->min_turnaround_time;
> +	q_chan->type = pcc_mchan->type;

[Severity: Medium]
Does this code leave the mchan field uninitialized? If a caller allocates
q_chan on the stack without zero-initializing it, the mchan field will retain
uninitialized stack memory.

If the caller subsequently checks mchan or mistakenly passes the structure to
pcc_mbox_free_channel(), could this bypass null-checks and cause arbitrary
pointer dereferences?

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(pcc_mbox_query_channel);

[Severity: High]
This is a pre-existing issue, but could this lead to a use-after-free on
driver unbind?

The platform driver lacks a .remove callback and does not set
.suppress_bind_attrs = true:

static struct platform_driver pcc_mbox_driver = {
	.probe = pcc_mbox_probe,
	.driver = {
		.name = "PCCT",
	},
};

If the driver is unbound via sysfs, the driver core automatically frees all
devm_-managed memory, including pcc_mbox_ctrl, pcc_mbox_channels, and
chan_info.

However, pcc_mbox_probe() uses mbox_controller_register() instead of
devm_mbox_controller_register():

	rc = mbox_controller_register(pcc_mbox_ctrl);

This leaves the controller registered in the mailbox framework. Subsequent
attempts to request or query a channel would then access the freed
chan_info or pcc_mbox_ctrl.

[ ... ]
> diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
> index 840bfc95bae33..8d0fada6e31fb 100644
> --- a/include/acpi/pcc.h
> +++ b/include/acpi/pcc.h
> @@ -8,6 +8,10 @@
>  
>  #include <linux/mailbox_controller.h>
>  #include <linux/mailbox_client.h>
> +#include <linux/acpi.h>
> +//#include <acpi/actypes.h>
> +//#include <acpi/actbl.h>
> +//#include <acpi/actbl2.h>

[Severity: Low]
Are these commented-out include directives intended to be kept? Leaving dead
code in a public header file can clutter the API.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604203749.168752-1-admiyo@os.amperecomputing.com?part=1