| Message ID | 20231201110631.669085-1-quic_deesin@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [V1] rpmsg: glink: smem: validate index before fifo read write | expand |
On Fri, Dec 01, 2023 at 04:36:31PM +0530, Deepak Kumar Singh wrote: > Fifo head and tail index can be modified with wrong values from > untrusted remote procs. Glink smem is not validating these index > before using to read or write fifo. This can result in out of > bound memory access if head and tail have incorrect values. > > Add check for validation of head and tail index. This check will > put index within fifo boundaries, so that no invalid memory access > is made. Further this may result in certain packet drops unless > glink finds a valid packet header in fifo again and recovers. > > Crash signature and calltrace with wrong head and tail values: > > Internal error: Oops: 96000007 [#1] PREEMPT SMP > pc : __memcpy_fromio+0x34/0xb4 > lr : glink_smem_rx_peak+0x68/0x94 > > __memcpy_fromio+0x34/0xb4 > glink_smem_rx_peak+0x68/0x94 > qcom_glink_native_intr+0x90/0x888 > > Signed-off-by: Deepak Kumar Singh <quic_deesin@quicinc.com> > --- > drivers/rpmsg/qcom_glink_smem.c | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/drivers/rpmsg/qcom_glink_smem.c b/drivers/rpmsg/qcom_glink_smem.c > index 7a982c60a8dd..9eba0aaae916 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; > } > > static void glink_smem_rx_peek(struct qcom_glink_pipe *np, > @@ -99,6 +104,10 @@ static void glink_smem_rx_peek(struct qcom_glink_pipe *np, > u32 tail; > > tail = le32_to_cpu(*pipe->tail); > + > + if (WARN_ON_ONCE(tail > pipe->native.length)) > + return; Just returning here will leave the caller with garbage in @data, which they will act upon. It does avoid the out of bounds read, but I'm not confident in what happens next. > + > tail += offset; > if (tail >= pipe->native.length) > tail -= pipe->native.length; > @@ -121,7 +130,7 @@ static void glink_smem_rx_advance(struct qcom_glink_pipe *np, > > tail += count; > if (tail >= pipe->native.length) > - tail -= pipe->native.length; > + tail %= pipe->native.length; If @tail had a bogus value before we incremented then we now have a completely random value. The next time the FIFO is read these values will be OK and we will return some random values to the caller. > > *pipe->tail = cpu_to_le32(tail); > } > @@ -146,6 +155,9 @@ static size_t glink_smem_tx_avail(struct qcom_glink_pipe *np) > else > avail -= FIFO_FULL_RESERVE + TX_BLOCKED_CMD_RESERVE; > > + if (WARN_ON_ONCE(avail > pipe->native.length)) > + avail = 0; > + > return avail; > } > > @@ -155,6 +167,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; As above, but with less probability, this might end up adjusting pipe->head (in glink_smem_tx_write()) to a random position within the FIFO - which then upon next access will corrupt the data. This shouldn't cause any direct issues on the Linux side though, we will just corrupt the outgoing FIFO (which probably don't matter given that things are already broken). Regards, Bjorn > + > len = min_t(size_t, count, pipe->native.length - head); > if (len) > memcpy(pipe->fifo + head, data, len); > -- > 2.34.1 >
diff --git a/drivers/rpmsg/qcom_glink_smem.c b/drivers/rpmsg/qcom_glink_smem.c index 7a982c60a8dd..9eba0aaae916 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; } static void glink_smem_rx_peek(struct qcom_glink_pipe *np, @@ -99,6 +104,10 @@ static void glink_smem_rx_peek(struct qcom_glink_pipe *np, u32 tail; tail = le32_to_cpu(*pipe->tail); + + if (WARN_ON_ONCE(tail > pipe->native.length)) + return; + tail += offset; if (tail >= pipe->native.length) tail -= pipe->native.length; @@ -121,7 +130,7 @@ static void glink_smem_rx_advance(struct qcom_glink_pipe *np, tail += count; if (tail >= pipe->native.length) - tail -= pipe->native.length; + tail %= pipe->native.length; *pipe->tail = cpu_to_le32(tail); } @@ -146,6 +155,9 @@ static size_t glink_smem_tx_avail(struct qcom_glink_pipe *np) else avail -= FIFO_FULL_RESERVE + TX_BLOCKED_CMD_RESERVE; + if (WARN_ON_ONCE(avail > pipe->native.length)) + avail = 0; + return avail; } @@ -155,6 +167,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; + len = min_t(size_t, count, pipe->native.length - head); if (len) memcpy(pipe->fifo + head, data, len);
Fifo head and tail index can be modified with wrong values from untrusted remote procs. Glink smem is not validating these index before using to read or write fifo. This can result in out of bound memory access if head and tail have incorrect values. Add check for validation of head and tail index. This check will put index within fifo boundaries, so that no invalid memory access is made. Further this may result in certain packet drops unless glink finds a valid packet header in fifo again and recovers. Crash signature and calltrace with wrong head and tail values: Internal error: Oops: 96000007 [#1] PREEMPT SMP pc : __memcpy_fromio+0x34/0xb4 lr : glink_smem_rx_peak+0x68/0x94 __memcpy_fromio+0x34/0xb4 glink_smem_rx_peak+0x68/0x94 qcom_glink_native_intr+0x90/0x888 Signed-off-by: Deepak Kumar Singh <quic_deesin@quicinc.com> --- drivers/rpmsg/qcom_glink_smem.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)