From: Prasad J Pandit <pjp@fedoraproject.org>

While writing a message in 'lsi_do_msgin', message length value
in 'msg_len' could be invalid. Add check to avoid OOB access issue.

Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/scsi/lsi53c895a.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

Update v2: modify assert()
  -> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg06273.html

diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index d1e6534311..dc39f4c3ee 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -861,10 +861,11 @@ static void lsi_do_status(LSIState *s)
 
 static void lsi_do_msgin(LSIState *s)
 {
-    int len;
+    uint8_t len;
     trace_lsi_do_msgin(s->dbc, s->msg_len);
     s->sfbr = s->msg[0];
     len = s->msg_len;
+    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);
@@ -1705,8 +1706,10 @@ static uint8_t lsi_reg_readb(LSIState *s, int offset)
         break;
     case 0x58: /* SBDL */
         /* Some drivers peek at the data bus during the MSG IN phase.  */
-        if ((s->sstat1 & PHASE_MASK) == PHASE_MI)
+        if ((s->sstat1 & PHASE_MASK) == PHASE_MI) {
+            assert(s->msg_len >= 0);
             return s->msg[0];
+        }
         ret = 0;
         break;
     case 0x59: /* SBDL high */
@@ -2103,11 +2106,23 @@ static int lsi_pre_save(void *opaque)
     return 0;
 }
 
+static int lsi_post_load(void *opaque, int version_id)
+{
+    LSIState *s = opaque;
+
+    if (s->msg_len < 0 || s->msg_len > LSI_MAX_MSGIN_LEN) {
+        return -EINVAL;
+    }
+
+    return 0;
+}
+
 static const VMStateDescription vmstate_lsi_scsi = {
     .name = "lsiscsi",
     .version_id = 0,
     .minimum_version_id = 0,
     .pre_save = lsi_pre_save,
+    .post_load = lsi_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_PCI_DEVICE(parent_obj, LSIState),
 
-- 
2.17.2

On 30/10/2018 07:28, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> While writing a message in 'lsi_do_msgin', message length value
> in 'msg_len' could be invalid. Add check to avoid OOB access issue.
> 
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

with one change below:

> ---
>  hw/scsi/lsi53c895a.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> Update v2: modify assert()
>   -> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg06273.html
> 
> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> index d1e6534311..dc39f4c3ee 100644
> --- a/hw/scsi/lsi53c895a.c
> +++ b/hw/scsi/lsi53c895a.c
> @@ -861,10 +861,11 @@ static void lsi_do_status(LSIState *s)
>  
>  static void lsi_do_msgin(LSIState *s)
>  {
> -    int len;
> +    uint8_t len;
>      trace_lsi_do_msgin(s->dbc, s->msg_len);
>      s->sfbr = s->msg[0];
>      len = s->msg_len;
> +    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);
> @@ -1705,8 +1706,10 @@ static uint8_t lsi_reg_readb(LSIState *s, int offset)
>          break;
>      case 0x58: /* SBDL */
>          /* Some drivers peek at the data bus during the MSG IN phase.  */
> -        if ((s->sstat1 & PHASE_MASK) == PHASE_MI)
> +        if ((s->sstat1 & PHASE_MASK) == PHASE_MI) {
> +            assert(s->msg_len >= 0);

should be > 0 as well.

Paolo

>              return s->msg[0];
> +        }
>          ret = 0;
>          break;
>      case 0x59: /* SBDL high */
> @@ -2103,11 +2106,23 @@ static int lsi_pre_save(void *opaque)
>      return 0;
>  }
>  
> +static int lsi_post_load(void *opaque, int version_id)
> +{
> +    LSIState *s = opaque;
> +
> +    if (s->msg_len < 0 || s->msg_len > LSI_MAX_MSGIN_LEN) {
> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}
> +
>  static const VMStateDescription vmstate_lsi_scsi = {
>      .name = "lsiscsi",
>      .version_id = 0,
>      .minimum_version_id = 0,
>      .pre_save = lsi_pre_save,
> +    .post_load = lsi_post_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_PCI_DEVICE(parent_obj, LSIState),
>  
> 

+-- On Tue, 30 Oct 2018, Paolo Bonzini wrote --+
| 
| Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
| 
| with one change below:
| 
| > +        if ((s->sstat1 & PHASE_MASK) == PHASE_MI) {
| > +            assert(s->msg_len >= 0);
| 
| should be > 0 as well.

Sent patch v3. Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F