On Wed, Jun 17, 2026 at 07:07:14PM +0800, Chunkai Deng wrote:
> The FIFO read/write helpers assume the head and tail indices stay within
> [0, pipe->native.length) and use them directly as offsets into the
> mapped FIFO region. If that invariant is ever broken, the subsequent
> memcpy or memcpy_fromio would access memory outside the FIFO.
>
> Add WARN_ON_ONCE checks in these helpers so a broken invariant is
> caught and reported once, and the out-of-bounds access is skipped
> instead of proceeding silently.
>
> Fixes: caf989c350e8 ("rpmsg: glink: Introduce glink smem based transport")
> Cc: [email protected]
> Signed-off-by: Chunkai Deng <[email protected]>
> ---
> drivers/rpmsg/qcom_glink_smem.c | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/rpmsg/qcom_glink_smem.c b/drivers/rpmsg/qcom_glink_smem.c
> index edab912557ac..42ad315d7910 100644
> --- a/drivers/rpmsg/qcom_glink_smem.c
> +++ b/drivers/rpmsg/qcom_glink_smem.c
> @@ -86,9 +86,14 @@ static size_t glink_smem_rx_avail(struct qcom_glink_pipe
> *np)
> tail = le32_to_cpu(*pipe->tail);
>
> if (head < tail)
> - return pipe->native.length - tail + head;
> + len = pipe->native.length - tail + head;
> else
> - return head - tail;
> + len = head - tail;
> +
> + if (WARN_ON_ONCE(len > pipe->native.length))
> + len = 0;
> +
> + return len;
Looks good.
> }
>
> static void glink_smem_rx_peek(struct qcom_glink_pipe *np,
> @@ -103,6 +108,9 @@ static void glink_smem_rx_peek(struct qcom_glink_pipe *np,
> if (tail >= pipe->native.length)
> tail -= pipe->native.length;
>
> + if (WARN_ON_ONCE(tail >= pipe->native.length))
> + return;
> +
Wouldn't it be preferable to check the original "tail", before the
addition and subtraction?
Perhaps also validate that `offset + count < pipe->native.length`?
> len = min_t(size_t, count, pipe->native.length - tail);
> if (len)
> memcpy_fromio(data, pipe->fifo + tail, len);
> @@ -141,6 +149,9 @@ static size_t glink_smem_tx_avail(struct qcom_glink_pipe
> *np)
> else
> avail = tail - head;
>
> + if (WARN_ON_ONCE(avail > pipe->native.length))
> + avail = 0;
`head - tail < length` does not guarantee that head and tail are valid
offsets within the fifo, so I think you should check both of them
instead of the difference.
> +
> if (avail < (FIFO_FULL_RESERVE + TX_BLOCKED_CMD_RESERVE))
> avail = 0;
> else
> @@ -155,6 +166,9 @@ static unsigned int glink_smem_tx_write_one(struct
> glink_smem_pipe *pipe,
> {
> size_t len;
>
> + if (WARN_ON_ONCE(head >= pipe->native.length))
> + return head;
This makes glink_smem_tx_write_one() do nothing, twice. But then we
return to glink_smem_tx_write() which will adjust and update pipe->head;
possible subtract head into the valid range.
I think it would be better to move this check up to
glink_smem_tx_write().
And also check that `hlen + dlen < pipe->native.length`?
Regards,
Bjorn
> +
> len = min_t(size_t, count, pipe->native.length - head);
> if (len)
> memcpy(pipe->fifo + head, data, len);
>
> --
> 2.34.1
>
