[PATCH] rpmsg: virtio: Reject a message shorter than its header

[PATCH] rpmsg: virtio: Reject a message shorter than its header

[Lianjie Wang]

rpmsg_recv_single() rejects a message whose msg_len exceeds len -
sizeof(struct rpmsg_hdr), where len is the used length the device
reported. Since len is an unsigned int, a used length shorter than the
header wraps the difference, and every msg_len passes. The endpoint
callback receives up to 65535 bytes from a receive buffer of 512 bytes
by default, and rpmsg_char copies them into an skb that userspace reads.
The receive and transmit buffers share one DMA allocation, so the copy
can run through the following buffers and, with small vrings, past the
end of the allocation. With vrings of 16 buffers and a header claiming
65535 bytes, KASAN reports:

  BUG: KASAN: slab-out-of-bounds in rpmsg_ept_cb+0x4d/0xb0
  Read of size 65535 at addr ffff888002484010 by task rpmsg_echo/140
  Call Trace:
   rpmsg_ept_cb+0x4d/0xb0
   rpmsg_recv_done+0x29f/0x840
   vring_interrupt+0x32a/0x540
   vp_interrupt+0x146/0x180

Compare the sum of the header size and msg_len with len. The sum cannot
overflow, since msg_len has 16 bits. Move the check ahead of the debug
output, since dynamic_hex_dump() reads sizeof(*msg) + msg_len bytes
when its dynamic debug site is enabled.

This patch is assisted by Claude Opus 5.5.

Fixes: 9648224e564a ("rpmsg: validate incoming message length before propagating")
Cc: stable@vger.kernel.org
Signed-off-by: Lianjie Wang <lian@is.s.u-tokyo.ac.jp>
---
 drivers/rpmsg/virtio_rpmsg_bus.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 23ff6c8b7a20b..a9125e81cb150 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -661,6 +661,16 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
 	unsigned int msg_len = __rpmsg16_to_cpu(little_endian, msg->len);
 	int err;
 
+	/*
+	 * We currently use fixed-sized buffers, so trivially sanitize
+	 * the reported payload length.
+	 */
+	if (len > vrp->rx_buf_size ||
+	    sizeof(struct rpmsg_hdr) + msg_len > len) {
+		dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg_len);
+		return -EINVAL;
+	}
+
 	dev_dbg(dev, "From: 0x%x, To: 0x%x, Len: %d, Flags: %d, Reserved: %d\n",
 		__rpmsg32_to_cpu(little_endian, msg->src),
 		__rpmsg32_to_cpu(little_endian, msg->dst), msg_len,
@@ -671,16 +681,6 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
 			 msg, sizeof(*msg) + msg_len, true);
 #endif
 
-	/*
-	 * We currently use fixed-sized buffers, so trivially sanitize
-	 * the reported payload length.
-	 */
-	if (len > vrp->rx_buf_size ||
-	    msg_len > (len - sizeof(struct rpmsg_hdr))) {
-		dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg_len);
-		return -EINVAL;
-	}
-
 	/* use the dst addr to fetch the callback of the appropriate user */
 	mutex_lock(&vrp->endpoints_lock);
 

base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
-- 
2.56.0

Re: [PATCH] rpmsg: virtio: Reject a message shorter than its header

[Mathieu Poirier]

On Tue, Nov 03, 2026 at 07:15:04AM +0900, Lianjie Wang wrote:
> rpmsg_recv_single() rejects a message whose msg_len exceeds len -
> sizeof(struct rpmsg_hdr), where len is the used length the device
> reported. Since len is an unsigned int, a used length shorter than the
> header wraps the difference, and every msg_len passes. The endpoint
> callback receives up to 65535 bytes from a receive buffer of 512 bytes
> by default, and rpmsg_char copies them into an skb that userspace reads.
> The receive and transmit buffers share one DMA allocation, so the copy
> can run through the following buffers and, with small vrings, past the
> end of the allocation. With vrings of 16 buffers and a header claiming
> 65535 bytes, KASAN reports:
> 
>   BUG: KASAN: slab-out-of-bounds in rpmsg_ept_cb+0x4d/0xb0
>   Read of size 65535 at addr ffff888002484010 by task rpmsg_echo/140
>   Call Trace:
>    rpmsg_ept_cb+0x4d/0xb0
>    rpmsg_recv_done+0x29f/0x840
>    vring_interrupt+0x32a/0x540
>    vp_interrupt+0x146/0x180
> 
> Compare the sum of the header size and msg_len with len. The sum cannot
> overflow, since msg_len has 16 bits. Move the check ahead of the debug
> output, since dynamic_hex_dump() reads sizeof(*msg) + msg_len bytes
> when its dynamic debug site is enabled.

I don't understand why the check is moved ahead of the debug output and the
robot generated changelog isn't helpful at all.

I agree with this fix if we don't move the check ahead of the dev_dbg()
statement.  For this patch and any future ones you may send, write your own
changelog rather than simply dumping what the robot says.  

> 
> This patch is assisted by Claude Opus 5.5.
> 
> Fixes: 9648224e564a ("rpmsg: validate incoming message length before propagating")
> Cc: stable@vger.kernel.org
> Signed-off-by: Lianjie Wang <lian@is.s.u-tokyo.ac.jp>
> ---
>  drivers/rpmsg/virtio_rpmsg_bus.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 23ff6c8b7a20b..a9125e81cb150 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -661,6 +661,16 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
>  	unsigned int msg_len = __rpmsg16_to_cpu(little_endian, msg->len);
>  	int err;
>  
> +	/*
> +	 * We currently use fixed-sized buffers, so trivially sanitize
> +	 * the reported payload length.
> +	 */
> +	if (len > vrp->rx_buf_size ||
> +	    sizeof(struct rpmsg_hdr) + msg_len > len) {
> +		dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg_len);
> +		return -EINVAL;
> +	}
> +
>  	dev_dbg(dev, "From: 0x%x, To: 0x%x, Len: %d, Flags: %d, Reserved: %d\n",
>  		__rpmsg32_to_cpu(little_endian, msg->src),
>  		__rpmsg32_to_cpu(little_endian, msg->dst), msg_len,
> @@ -671,16 +681,6 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
>  			 msg, sizeof(*msg) + msg_len, true);
>  #endif
>  
> -	/*
> -	 * We currently use fixed-sized buffers, so trivially sanitize
> -	 * the reported payload length.
> -	 */
> -	if (len > vrp->rx_buf_size ||
> -	    msg_len > (len - sizeof(struct rpmsg_hdr))) {
> -		dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg_len);
> -		return -EINVAL;
> -	}
> -
>  	/* use the dst addr to fetch the callback of the appropriate user */
>  	mutex_lock(&vrp->endpoints_lock);
>  
> 
> base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
> -- 
> 2.56.0
>