Re: [PATCH v16 5/5] arm64: dts: imx8ulp: Add rpmsg node under imx_rproc

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] gpio: lost EOI due to race between set_bit() and bitmap_zero()
- [High] gpio: out-of-bounds read in rpmsg_gpio_channel_callback()
--

commit aea7aeffb6bed8da7df793b582974e6352af17db
Author: Shenwei Wang <shenwei.wang@nxp.com>

gpio: rpmsg: add generic rpmsg GPIO driver

On an AMP platform, the system may include multiple processors communicating
via the RPMSG protocol. This driver implements the standard GPIO interface,
allowing the Linux side to control GPIO controllers on the remote processor.

> 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 void gpio_rpmsg_eoi_irq(struct irq_data *d)
> +{
> +	struct rpmsg_gpio_port *port = irq_data_get_irq_chip_data(d);
> +
> +	set_bit(d->hwirq, port->pending_eoi);
> +	schedule_work(&port->eoi_work);
> +}

[ ... ]

> +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]
Does this code lose EOI events in port->pending_eoi?

In gpio_rpmsg_eoi_irq(), set_bit() is used to atomically record an EOI for a
specific GPIO line in port->pending_eoi. However, bitmap_zero() performs a
non-atomic memory clear. 

If set_bit() executes after the workqueue reads the bitmap in bitmap_copy(),
but before or during the non-atomic bitmap_zero(), won't the newly set bit
be overwritten with zero and 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);

[ ... ]

> +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: High]
Does this code read beyond the data buffer?

The callback casts the data pointer to struct rpmsg_gpio_response and
immediately accesses msg->type, or copies the entire struct.

Since the len parameter is not validated, if a truncated packet with a length
smaller than sizeof(struct rpmsg_gpio_response) is received, couldn't this
result in an out-of-bounds read?

> +		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;
> +}

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