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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] Writing to the `.name` field of the immutable `gpio_rpmsg_irq_chip` structure attempts to modify read-only memory, causing a kernel panic.
- [High] Host-endian integers are directly assigned to little-endian protocol fields, breaking communication on big-endian architectures.
- [High] An RPMSG timeout permanently desynchronizes the request/response state machine because the driver lacks transaction IDs to discard late replies.
- [High] The RPMSG callback blindly accesses the incoming message without validating that the payload length is sufficient.
- [High] The driver's GPIO callbacks can sleep, but it fails to set `gc->can_sleep = true`, incorrectly allowing them to be called from atomic contexts.
- [High] Interrupts will be permanently masked after firing once because the driver incorrectly relies on `irq_bus_sync_unlock` to send the unmask command during normal IRQ flow.
- [Medium] The driver leaks a reference to the `rproc` parent device node.
- [Medium] `rpmsg_gpio_direction_output()` configures the GPIO line as an output before setting the desired logical value, potentially causing a signal glitch.
--

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

This commit adds a generic GPIO driver to support controlling GPIO
controllers that reside on a remote processor. It translates standard
Linux GPIO calls into RPMSG protocol messages sent to the remote side.

> diff --git a/drivers/gpio/gpio-rpmsg.c b/drivers/gpio/gpio-rpmsg.c
> new file mode 100644
> index 0000000000000..332e2925a8309
> --- /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]
If rpmsg_send times out, can this permanently desynchronize the request and
response state machine?

If a timeout occurs, rpmsg_gpio_send_message() returns -ETIMEDOUT but leaves
the channel active. Since the rpmsg_gpio_response doesn't appear to contain a
transaction ID, if the remote processor sends a delayed reply to this timed-out
command, will rpmsg_gpio_channel_callback() mistakenly accept it as the
completion for the next unrelated command sent by the host?

> +	}
> +
> +	if (unlikely(port->recv_msg->status != VIRTIO_GPIO_STATUS_OK)) {
> +		dev_err(&port->rpdev->dev, "remote core replies an error: cmd=%d!\n",
> +			port->send_msg->type);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct virtio_gpio_request *
> +rpmsg_gpio_msg_prepare(struct rpmsg_gpio_port *port, u16 line, u16 cmd, u32 val)
> +{
> +	struct virtio_gpio_request *msg = port->send_msg;
> +
> +	msg->type = cmd;
> +	msg->gpio = line;
> +	msg->value = val;

[Severity: High]
Are these values being assigned with the correct endianness?

The virtio_gpio_request structure defines these fields as __le16 and __le32,
but this assigns native host-endian integers to them directly. On big-endian
architectures, could this result in the remote processor misinterpreting the
commands?

> +
> +	return msg;
> +}

[ ... ]

> +static int rpmsg_gpio_direction_output(struct gpio_chip *gc, unsigned int line, int val)
> +{
> +	struct rpmsg_gpio_port *port = gpiochip_get_data(gc);
> +	int ret;
> +
> +	guard(mutex)(&port->lock);
> +
> +	rpmsg_gpio_msg_prepare(port, line, VIRTIO_GPIO_MSG_SET_DIRECTION,
> +			       VIRTIO_GPIO_DIRECTION_OUT);
> +
> +	ret = rpmsg_gpio_send_message(port);
> +	if (ret)
> +		return ret;
> +
> +	rpmsg_gpio_msg_prepare(port, line, VIRTIO_GPIO_MSG_SET_VALUE, val);

[Severity: Medium]
Could configuring the direction before setting the value cause a signal
glitch?

rpmsg_gpio_direction_output() sends VIRTIO_GPIO_MSG_SET_DIRECTION to the
remote processor before VIRTIO_GPIO_MSG_SET_VALUE. If the remote hardware
applies the direction immediately, could this drive the line to its default
or previous state for a short window before the intended new value is applied?

> +
> +	return rpmsg_gpio_send_message(port);
> +}

[ ... ]

> +static void gpio_rpmsg_unmask_irq(struct irq_data *d)
> +{
> +	struct rpmsg_gpio_port *port = irq_data_get_irq_chip_data(d);
> +	u32 line = d->hwirq;
> +
> +	port->lines[line].irq_unmask = 1;
> +}

[Severity: High]
Will interrupts remain permanently masked on the remote processor after they
fire once?

gpio_rpmsg_unmask_irq() updates local memory but does not send an RPMSG
command to unmask the interrupt remotely. While gpio_rpmsg_irq_bus_sync_unlock()
does send the command, the core IRQ subsystem (like handle_level_irq) masks
and unmasks interrupts without invoking bus lock/unlock callbacks during normal
interrupt dispatch. Does this mean the unmask command is never actually sent
after an interrupt is handled?

[ ... ]

> +static int rpmsg_gpiochip_register(struct rpmsg_device *rpdev,
> +				   struct device_node *np, const char *name)
> +{
> +	struct rpmsg_gpio_port *port;
> +	struct gpio_irq_chip *girq;
> +	struct gpio_chip *gc;
> +	int ret;
> +
> +	port = devm_kzalloc(&rpdev->dev, sizeof(*port), GFP_KERNEL);
> +	if (!port)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_u32(np, "reg", &port->idx);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_mutex_init(&rpdev->dev, &port->lock);
> +	if (ret)
> +		return ret;
> +
> +	ret = of_property_read_u32(np, "ngpios", &port->ngpios);
> +	if (ret || port->ngpios > GPIOS_PER_PORT_DEFAULT)
> +		port->ngpios = GPIOS_PER_PORT_DEFAULT;
> +
> +	port->send_msg = devm_kzalloc(&rpdev->dev,
> +				      sizeof(*port->send_msg),
> +				      GFP_KERNEL);
> +
> +	port->recv_msg = devm_kzalloc(&rpdev->dev,
> +				      sizeof(*port->recv_msg),
> +				      GFP_KERNEL);
> +	if (!port->send_msg || !port->recv_msg)
> +		return -ENOMEM;
> +
> +	init_completion(&port->cmd_complete);
> +	port->rpdev = rpdev;
> +
> +	gc = &port->gc;
> +	gc->owner = THIS_MODULE;
> +	gc->parent = &rpdev->dev;
> +	gc->fwnode = of_fwnode_handle(np);
> +	gc->ngpio = port->ngpios;
> +	gc->base = -1;
> +	gc->label = devm_kasprintf(&rpdev->dev, GFP_KERNEL, "%s-gpio%d",
> +				   name, port->idx);

[Severity: High]
Should gc->can_sleep be explicitly set to true for this gpio_chip?

The .set and .get callbacks in this driver acquire a mutex and wait for
completion, which will sleep. Without setting gc->can_sleep = true, the
gpiolib framework treats these GPIOs as atomic. If consumer drivers call
these GPIO operations from within an interrupt handler or a spinlock,
could this trigger a "scheduling while atomic" panic?

> +
> +	gc->direction_input = rpmsg_gpio_direction_input;
> +	gc->direction_output = rpmsg_gpio_direction_output;
> +	gc->get_direction = rpmsg_gpio_get_direction;
> +	gc->get = rpmsg_gpio_get;
> +	gc->set = rpmsg_gpio_set;
> +
> +	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->chip->name = devm_kstrdup(&rpdev->dev, gc->label, GFP_KERNEL);

[Severity: Critical]
Does this attempt to write to read-only memory?

gpio_irq_chip_set_chip() assigns the const structure gpio_rpmsg_irq_chip to
girq->chip. Modifying the .name field of a const structure placed in the
.rodata section will trigger a write-protection page fault and crash the
kernel. Should the irq_chip be dynamically allocated, or should the name be
set differently?

> +
> +	dev_set_drvdata(&rpdev->dev, port);
> +
> +	return devm_gpiochip_add_data(&rpdev->dev, gc, port);
> +}

[ ... ]

> +static struct device_node *
> +rpmsg_get_channel_ofnode(struct rpmsg_device *rpdev, const char *compat, u32 idx)
> +{
> +	struct device_node *np_chan = NULL, *np;
> +	struct rproc *rproc;
> +
> +	rproc = rproc_get_by_child(&rpdev->dev);
> +	if (!rproc)
> +		return NULL;
> +
> +	np = of_node_get(rproc->dev.of_node);
> +	if (!np && rproc->dev.parent)
> +		np = of_node_get(rproc->dev.parent->of_node);
> +
> +	if (np)
> +		np_chan = rpmsg_find_child_by_compat_reg(np, compat, idx);
> +
> +	return np_chan;

[Severity: Medium]
Does this leak the reference to the parent device node?

rpmsg_get_channel_ofnode() acquires a reference to np using of_node_get(),
but it appears to return without calling of_node_put(np) to release it.

> +}

[ ... ]

> +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;
> +
> +	port = dev_get_drvdata(&rpdev->dev);
> +
> +	if (!port) {
> +		dev_err(&rpdev->dev, "port is null\n");
> +		return -EINVAL;
> +	}
> +
> +	if (msg->type == GPIO_RPMSG_REPLY) {

[Severity: High]
Is there a risk of an out-of-bounds read here if the message is truncated?

rpmsg_gpio_channel_callback() receives a payload of size len, but blindly
dereferences the data pointer as struct rpmsg_gpio_response without
verifying that len >= sizeof(struct rpmsg_gpio_response). Could this read
out-of-bounds memory if the remote processor sends a malformed or small
message?

> +		*port->recv_msg = *msg;
> +		complete(&port->cmd_complete);
> +	} else if (msg->type == GPIO_RPMSG_NOTIFY) {
> +		generic_handle_domain_irq_safe(port->gc.irq.domain, msg->line);
> +	} else {
> +		dev_err(&rpdev->dev, "wrong message type (0x%x)\n", msg->type);
> +	}
> +
> +	return 0;
> +}

[ ... ]

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