[PATCH] lsi53c895a: avoid out of bounds access to s->msg[]

[PATCH] lsi53c895a: avoid out of bounds access to s->msg[]

[Paolo Bonzini]

If no bytes are there to process in the message in phase,
the input data latch (s->sidl) is set to s->msg[-1].  Just
do nothing since no DMA is performed.

Reported-by: Chuhong Yuan <hslester96@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/lsi53c895a.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index 71f759a59dd..eb9828dd5ef 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -927,13 +927,18 @@ static void lsi_do_msgin(LSIState *s)
     assert(len > 0 && len <= LSI_MAX_MSGIN_LEN);
     if (len > s->dbc)
         len = s->dbc;
-    pci_dma_write(PCI_DEVICE(s), s->dnad, s->msg, len);
-    /* Linux drivers rely on the last byte being in the SIDL.  */
-    s->sidl = s->msg[len - 1];
-    s->msg_len -= len;
-    if (s->msg_len) {
-        memmove(s->msg, s->msg + len, s->msg_len);
-    } else {
+
+    if (len) {
+        pci_dma_write(PCI_DEVICE(s), s->dnad, s->msg, len);
+        /* Linux drivers rely on the last byte being in the SIDL.  */
+        s->sidl = s->msg[len - 1];
+        s->msg_len -= len;
+        if (s->msg_len) {
+            memmove(s->msg, s->msg + len, s->msg_len);
+        }
+    }
+
+    if (!s->msg_len) {
         /* ??? Check if ATN (not yet implemented) is asserted and maybe
            switch to PHASE_MO.  */
         switch (s->msg_action) {
-- 
2.44.0

Re: [PATCH] lsi53c895a: avoid out of bounds access to s->msg[]

[Philippe Mathieu-Daudé]

On 31/3/24 20:17, Paolo Bonzini wrote:
> If no bytes are there to process in the message in phase,
> the input data latch (s->sidl) is set to s->msg[-1].  Just
> do nothing since no DMA is performed.
> 
> Reported-by: Chuhong Yuan <hslester96@gmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/scsi/lsi53c895a.c | 19 ++++++++++++-------
>   1 file changed, 12 insertions(+), 7 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>