methodmissing commented on code in PR #20538:
URL: https://github.com/apache/nuttx/pull/20538#discussion_r4241585515


##########
openamp/0020-rpmsg_virtio-do-not-trust-lengths-written-by-the-remote.patch:
##########
@@ -0,0 +1,62 @@
+From 6710e18a7b9631f5da2bdc8010ba0a79ac0e4317 Mon Sep 17 00:00:00 2001
+From: Lourens Naude <[email protected]>
+Date: Thu, 8 Oct 2026 22:14:33 +0100
+Subject: [PATCH 20/20] rpmsg_virtio: do not trust lengths written by the
+ remote
+
+On the driver side every TX buffer is h2r_buf_size long and every RX
+buffer r2h_buf_size long; used->len and rpmsg_hdr.len come from the
+remote.  A used->len of 0 turned into a 4 GB payload capacity, a header
+length larger than the buffer let endpoint callbacks read past it.
+
+Signed-off-by: Lourens Naude <[email protected]>
+---
+ lib/rpmsg/rpmsg_virtio.c | 16 ++++++++++++++--
+ 1 file changed, 14 insertions(+), 2 deletions(-)
+
+diff --git a/lib/rpmsg/rpmsg_virtio.c open-amp/lib/rpmsg/rpmsg_virtio.c
+index 9913d87..45783ab 100644
+--- a/lib/rpmsg/rpmsg_virtio.c
++++ open-amp/lib/rpmsg/rpmsg_virtio.c
+@@ -198,9 +198,11 @@ static void *rpmsg_virtio_get_tx_buffer(struct 
rpmsg_virtio_device *rvdev,
+               if (!data && rvdev->svq->vq_free_cnt) {
+                       data = rpmsg_virtio_shm_pool_get_buffer(rvdev->shpool,
+                                       rvdev->config.h2r_buf_size);
+-                      *len = rvdev->config.h2r_buf_size;
+                       *idx = 0;
+               }
++              /* used->len was written by the remote; the buffer is ours */
++              if (data)
++                      *len = rvdev->config.h2r_buf_size;

Review Comment:
   Correct, an OpenAMP remote echoes `desc->len`, so for that peer this patch 
changes nothing.
   
   But the used ring is `device-written`: a non-OpenAMP or buggy remote can put 
anything there, and 0 is the spec value. The driver already holds the capacity 
in `rvdev->config.h2r_buf_size`, so it should not size its own buffer from a 
peer-written field. Same argument for the `rp_hdr->len` checks.
   
   I acknowledge the risk surface is very small and this discussion makes more 
sense @ OpenAMP upstream.
   
   Thoughts on closing the PR here instead?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to