[Qemu-devel] [PATCH v1] lsi53c895a: check message length value is valid

[Qemu-devel] [PATCH v1] lsi53c895a: check message length value is valid

[P J P]

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 | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

Update v1: add .post_load routine and an assert() call
  -> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg05730.html

diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index d1e6534311..3a40e62853 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -861,12 +861,13 @@ 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;
     if (len > s->dbc)
         len = s->dbc;
+    assert(len <= LSI_MAX_MSGIN_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];
@@ -2103,11 +2104,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

Re: [Qemu-devel] [PATCH v1] lsi53c895a: check message length value is valid

[Peter Maydell]

On 26 October 2018 at 20:43, P J P <ppandit@redhat.com> 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>
> ---
>  hw/scsi/lsi53c895a.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> Update v1: add .post_load routine and an assert() call
>   -> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg05730.html
>
> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> index d1e6534311..3a40e62853 100644
> --- a/hw/scsi/lsi53c895a.c
> +++ b/hw/scsi/lsi53c895a.c
> @@ -861,12 +861,13 @@ 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;
>      if (len > s->dbc)
>          len = s->dbc;
> +    assert(len <= LSI_MAX_MSGIN_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];

Is it possible to get here with len == 0 ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1] lsi53c895a: check message length value is valid

[Paolo Bonzini]

On 26/10/2018 22:55, Peter Maydell wrote:
>> +    assert(len <= LSI_MAX_MSGIN_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];
> Is it possible to get here with len == 0 ?

No, all calls to

    lsi_set_phase(s, PHASE_MI);

are followed or preceded by lsi_add_msg_byte.  But an assertion is good
to add.  What do you think of squashing this on top:

diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index 3a40e62853..72d85c42dd 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -865,9 +865,9 @@ static void lsi_do_msgin(LSIState *s)
     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;
-    assert(len <= LSI_MAX_MSGIN_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];
@@ -1706,8 +1706,10 @@
         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 */


Paolo

Re: [Qemu-devel] [PATCH v1] lsi53c895a: check message length value is valid

[Paolo Bonzini]

On 29/10/2018 18:56, Paolo Bonzini wrote:
> On 26/10/2018 22:55, Peter Maydell wrote:
>>> +    assert(len <= LSI_MAX_MSGIN_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];
>> Is it possible to get here with len == 0 ?
> 
> No, all calls to
> 
>     lsi_set_phase(s, PHASE_MI);
> 
> are followed or preceded by lsi_add_msg_byte.  But an assertion is good
> to add.  What do you think of squashing this on top:
> 
> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> index 3a40e62853..72d85c42dd 100644
> --- a/hw/scsi/lsi53c895a.c
> +++ b/hw/scsi/lsi53c895a.c
> @@ -865,9 +865,9 @@ static void lsi_do_msgin(LSIState *s)
>      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);

Ahem, len > 0.  Is there a CVE number?

Paolo

>      if (len > s->dbc)
>          len = s->dbc;
> -    assert(len <= LSI_MAX_MSGIN_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];
> @@ -1706,8 +1706,10 @@
>          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 */
> 
> 
> Paolo
> 

Re: [Qemu-devel] [PATCH v1] lsi53c895a: check message length value is valid

[P J P]

+-- On Mon, 29 Oct 2018, Paolo Bonzini wrote --+
| On 29/10/2018 18:56, Paolo Bonzini wrote:
| > On 26/10/2018 22:55, Peter Maydell wrote:
| >>> +    assert(len <= LSI_MAX_MSGIN_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];
| >> Is it possible to get here with len == 0 ?
| > 
| > No, all calls to
| > 
| >     lsi_set_phase(s, PHASE_MI);
| > 
| > are followed or preceded by lsi_add_msg_byte.  But an assertion is good
| > to add.  What do you think of squashing this on top:
| > 
| > diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
| > index 3a40e62853..72d85c42dd 100644
| > --- a/hw/scsi/lsi53c895a.c
| > +++ b/hw/scsi/lsi53c895a.c
| > @@ -865,9 +865,9 @@ static void lsi_do_msgin(LSIState *s)
| >      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);
| 
| Ahem, len > 0.  Is there a CVE number?

Sent revised patch v2. I'll request CVE once patch is reviewed/approved here.

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