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

[Mathieu Poirier <mathieu.poirier@linaro.org>]

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
>