Re: [PATCH v16 4/5] gpio: rpmsg: add generic rpmsg GPIO driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] gpio: rpmsg: non-atomic bitmap_zero() clobbers concurrent set_bit() updates
- [High] gpio: rpmsg: premature cancel_work_sync() causes UAF on port struct
- [High] gpio: rpmsg: RPC timeouts break request-response synchronization
- [High] gpio: rpmsg: early drvdata setup causes NULL pointer dereference in probe
- [Medium] gpio: rpmsg: out-of-bounds read in callback due to missing length check
- [Medium] gpio: rpmsg: eoi_work() unmasks interrupts that were just shut down
--

commit aea7aeffb6bed8da7df793b582974e6352af17db
Author: Shenwei Wang <shenwei.wang@nxp.com>
Subject: gpio: rpmsg: add generic rpmsg GPIO driver

This commit adds a generic RPMSG GPIO driver allowing Linux to control
remote processor GPIOs via RPMSG.

> diff --git a/drivers/gpio/gpio-rpmsg.c b/drivers/gpio/gpio-rpmsg.c
> new file mode 100644
> index 0000000000000..076af781f3bfb
> --- /dev/null
> +++ b/drivers/gpio/gpio-rpmsg.c
[ ... ]
> +static int rpmsg_gpio_send_message(struct rpmsg_gpio_port *port)
> +{
> +	int ret;
> +
> +	reinit_completion(&port->cmd_complete);
> +
> +	ret = rpmsg_send(port->rpdev->ept, port->send_msg, sizeof(*port->send_msg));
> +	if (ret) {
> +		dev_err(&port->rpdev->dev, "rpmsg_send failed: cmd=%d ret=%d\n",
> +			port->send_msg->type, ret);
> +		return ret;
> +	}
> +
> +	ret = wait_for_completion_timeout(&port->cmd_complete,
> +					  msecs_to_jiffies(RPMSG_TIMEOUT));
> +	if (ret == 0) {
> +		dev_err(&port->rpdev->dev, "rpmsg_send timeout! cmd=%d\n",
> +			port->send_msg->type);
> +		return -ETIMEDOUT;
> +	}

[Severity: High]
Is it possible for a delayed reply to break the request-response
synchronization?

If wait_for_completion_timeout() times out, the driver aborts the current
request but leaves the channel active. If a new request is subsequently
sent, could a late reply from the timed-out request overwrite *port->recv_msg
in rpmsg_gpio_channel_callback() and falsely wake up the completion for the
new request?

[ ... ]
> +static void gpio_rpmsg_eoi_work(struct work_struct *work)
> +{
> +	struct rpmsg_gpio_port *port =
> +		container_of(work, struct rpmsg_gpio_port, eoi_work);
> +	unsigned long pending[BITS_TO_LONGS(GPIOS_PER_PORT_MAX)];
> +	unsigned int line;
> +
> +	guard(mutex)(&port->lock);
> +
> +	bitmap_copy(pending, port->pending_eoi, port->ngpios);
> +	bitmap_zero(port->pending_eoi, port->ngpios);

[Severity: High]
Could bitmap_zero() clobber concurrent updates?

Since bitmap_zero() expands to a non-atomic memory store, if a new
interrupt arrives and calls set_bit() in gpio_rpmsg_eoi_irq() while this
zeroing is executing, won't the new hardware interrupt get permanently lost?

> +
> +	for_each_set_bit(line, pending, port->ngpios) {
> +		rpmsg_gpio_msg_prepare(port, line, VIRTIO_GPIO_MSG_IRQ_TYPE,
> +				       port->lines[line].irq_type);

[Severity: Medium]
Does this blindly re-enable interrupts that were just shut down?

If free_irq() calls gpio_rpmsg_irq_shutdown() to mask the interrupt right
before or while this async work is executing, could this
VIRTIO_GPIO_MSG_IRQ_TYPE command override the shutdown and cause spurious
wakeups on the remote end?

> +
> +		if (rpmsg_gpio_send_message(port))
> +			dev_err(&port->rpdev->dev, "EOI error for line %u\n", line);
> +	}
> +}
[ ... ]
> +static int rpmsg_gpiochip_register(struct rpmsg_device *rpdev, u32 idx,
> +				   struct device_node *np, const char *name)
> +{
[ ... ]
> +	girq = &gc->irq;
> +	gpio_irq_chip_set_chip(girq, &gpio_rpmsg_irq_chip);
> +	girq->parent_handler = NULL;
> +	girq->num_parents = 0;
> +	girq->parents = NULL;
> +	girq->default_type = IRQ_TYPE_NONE;
> +	girq->handler = handle_fasteoi_irq;
> +
> +	dev_set_drvdata(&rpdev->dev, port);
> +
> +	return devm_gpiochip_add_data(&rpdev->dev, gc, port);
> +}

[Severity: High]
Is there a risk of a NULL pointer dereference here if an early interrupt
arrives?

The call to dev_set_drvdata() exposes the port structure to the active RPMSG
endpoint before devm_gpiochip_add_data() initializes port->gc.irq.domain.

If the remote processor sends an interrupt notification at this exact moment,
rpmsg_gpio_channel_callback() could retrieve the port and pass a still-NULL
IRQ domain to generic_handle_domain_irq_safe().

[ ... ]
> +static int rpmsg_gpio_channel_callback(struct rpmsg_device *rpdev, void *data,
> +				       int len, void *priv, u32 src)
> +{
> +	struct rpmsg_gpio_response *msg = data;
> +	struct rpmsg_gpio_port *port = NULL;
> +	u32 line;
> +
> +	port = dev_get_drvdata(&rpdev->dev);
> +
> +	if (!port) {
> +		dev_err(&rpdev->dev, "port is null\n");
> +		return -EINVAL;
> +	}
> +
> +	if (msg->type == GPIO_RPMSG_REPLY) {
> +		*port->recv_msg = *msg;

[Severity: Medium]
Is there a missing bounds check on len?

If the remote processor sends a payload smaller than
sizeof(struct rpmsg_gpio_response), this assignment will perform an
out-of-bounds memory read, potentially processing garbage data. Should we
verify len before dereferencing msg?

> +		complete(&port->cmd_complete);
> +	} else if (msg->type == GPIO_RPMSG_NOTIFY) {
> +		line = le16_to_cpu(msg->line);
> +		generic_handle_domain_irq_safe(port->gc.irq.domain, line);
> +	} else {
> +		dev_err(&rpdev->dev, "wrong message type (0x%x)\n", msg->type);
> +	}
> +
> +	return 0;
> +}
[ ... ]
> +static void rpmsg_gpio_channel_remove(struct rpmsg_device *rpdev)
> +{
> +	struct rpmsg_gpio_port *port = dev_get_drvdata(&rpdev->dev);
> +
> +	cancel_work_sync(&port->eoi_work);
> +}

[Severity: High]
Could this lead to a use-after-free on the port struct when the driver is
unbound?

While cancel_work_sync() runs here, the RPMSG endpoint remains active until
devres tears it down later. If an interrupt arrives immediately after remove()
returns:

rpmsg_gpio_channel_callback()
  -> gpio_rpmsg_eoi_irq()
     -> schedule_work(&port->eoi_work);

This would queue work on a struct that devres is about to free, leaving a
freed work_struct linked in the kernel's workqueue. Should the endpoint be
explicitly disabled before canceling the work?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923184255.299034-1-shenwei.wang@oss.nxp.com?part=4