lsi53c895a: check message length value

PATCH In review P J P 4 participants 9 messages Oct 25, 2018 – Oct 26, 2018

lore

Share & tools

lore

P J P

author

2018-10-25 20:15 UTC #0 lore raw

PATCH 1/1 lsi53c895a: check message length value

From: Prasad J Pandit <[email protected]>

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

Reported-by Ameya More <[email protected]>

Signed-off-by Prasad J Pandit <[email protected]>

---
 hw/scsi/lsi53c895a.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

hw/scsi/lsi53c895a.c +6 -3

index d1e6534311..a266c5a113 100644
@@ -205,7 +205,7 @@ typedef struct {
     /* Action to take at the end of a MSG IN phase.
        0 = COMMAND, 1 = disconnect, 2 = DATA OUT, 3 = DATA IN.  */
     int msg_action;
-    int msg_len;
+    uint8_t msg_len;
     uint8_t msg[LSI_MAX_MSGIN_LEN];
     /* 0 if SCRIPTS are running or stopped.
      * 1 if a Wait Reselect instruction has been issued.

@@ -861,12 +861,15 @@ 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;
+    if (len > LSI_MAX_MSGIN_LEN) {
+        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];

@@ -2114,7 +2117,7 @@ static const VMStateDescription vmstate_lsi_scsi = {
         VMSTATE_INT32(carry, LSIState),
         VMSTATE_INT32(status, LSIState),
         VMSTATE_INT32(msg_action, LSIState),
-        VMSTATE_INT32(msg_len, LSIState),
+        VMSTATE_UINT8(msg_len, LSIState),
         VMSTATE_BUFFER(msg, LSIState),
         VMSTATE_INT32(waiting, LSIState),

Signature · 1 line

2.17.2

Ameya More

2018-10-25 20:36 UTC #1 lore raw

Hi Prasad,

Thanks for following up on this. While Mark and I reported this issue to you, it was actually discovered by Dejvau Security and they should receive credit for reporting this issue.
http://www.dejavusecurity.com

Thanks,
Ameya

Quoted 50 lines · P J P

From: Prasad J Pandit <[email protected]>

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

Reported-by Ameya More <[email protected]>

Signed-off-by Prasad J Pandit <[email protected]>

---
  hw/scsi/lsi53c895a.c | 9 ++++++---
  1 file changed, 6 insertions(+), 3 deletions(-)

hw/scsi/lsi53c895a.c +6 -3

index d1e6534311..a266c5a113 100644
@@ -205,7 +205,7 @@ typedef struct {
      /* Action to take at the end of a MSG IN phase.
         0 = COMMAND, 1 = disconnect, 2 = DATA OUT, 3 = DATA IN.  */
      int msg_action;
-    int msg_len;
+    uint8_t msg_len;
      uint8_t msg[LSI_MAX_MSGIN_LEN];
      /* 0 if SCRIPTS are running or stopped.
       * 1 if a Wait Reselect instruction has been issued.

@@ -861,12 +861,15 @@ 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;
+    if (len > LSI_MAX_MSGIN_LEN) {
+        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];

@@ -2114,7 +2117,7 @@ static const VMStateDescription vmstate_lsi_scsi = {
          VMSTATE_INT32(carry, LSIState),
          VMSTATE_INT32(status, LSIState),
          VMSTATE_INT32(msg_action, LSIState),
-        VMSTATE_INT32(msg_len, LSIState),
+        VMSTATE_UINT8(msg_len, LSIState),
          VMSTATE_BUFFER(msg, LSIState),
          VMSTATE_INT32(waiting, LSIState),

P J P

author

2018-10-26 09:25 UTC #2 lore raw

+-- On Thu, 25 Oct 2018, Ameya More wrote --+
| While Mark and I reported this issue to you, it was actually discovered by 
| Dejvau Security and they should receive credit for reporting this issue. 
| http://www.dejavusecurity.com

I see; Would it be possible to share email-id of the original reporter to include in the commit log message?

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

Mark Kanda

2018-10-26 14:01 UTC #3 lore raw

Quoted 7 lines · P J P

+-- On Thu, 25 Oct 2018, Ameya More wrote --+
| While Mark and I reported this issue to you, it was actually discovered by
| Dejvau Security and they should receive credit for reporting this issue.
| http://www.dejavusecurity.com
I see; Would it be possible to share email-id of the original reporter to include in the commit log message?

Deja vu requested that we include the following text in the commit message:

Discovered by Deja vu Security. Reported by Oracle.

Would that be acceptable?

Thanks,

-Mark

P J P

author

2018-10-26 18:37 UTC #4 lore raw

+-- On Fri, 26 Oct 2018, Mark Kanda wrote --+
| Deja vu requested that we include the following text in the commit message:
| 
| 	Discovered by Deja vu Security. Reported by Oracle.
| 
| Would that be acceptable?

Generally an email-id is used/preferred in the commit log message. We could use above for acknowledgement and avoid Reported-by in the commit log message if that suits Deja vu team.

Please let me know your/their preference.

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

Mark Kanda

2018-10-26 18:45 UTC #5 lore raw

Quoted 7 lines · P J P

+-- On Fri, 26 Oct 2018, Mark Kanda wrote --+
| Deja vu requested that we include the following text in the commit message:
|
| 	Discovered by Deja vu Security. Reported by Oracle.
|
| Would that be acceptable?

Quoted 6 lines

Generally an email-id is used/preferred in the commit log message. We could use above for acknowledgement and avoid Reported-by in the commit log message if that suits Deja vu team.
Please let me know your/their preference.

Yes, please use that acknowledgement text in lieu of a 'Reported-by' line.

Thanks,

-Mark

P J P

author

2018-10-26 19:09 UTC #6 lore raw

+-- On Fri, 26 Oct 2018, Mark Kanda wrote --+
| Yes, please use that acknowledgement text in lieu of a 'Reported-by' line.

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

Paolo Bonzini

2018-10-25 23:18 UTC #7 lore raw

In reply to P J P

Quoted 16 lines · P J P

From: Prasad J Pandit <[email protected]>
While writing a message in 'lsi_do_msgin', message length value in msg_len could be invalid, add check to avoid OOB access issue.
Reported-by Ameya More <[email protected]>
Signed-off-by Prasad J Pandit <[email protected]>
---
 hw/scsi/lsi53c895a.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
hw/scsi/lsi53c895a.c +0 -0
index d1e6534311..a266c5a113 100644

hw/scsi/lsi53c895a.c typedef struct { · line 208

Quoted 5 lines

hw/scsi/lsi53c895a.c +1 -1

@@ -205,7 +205,7 @@ typedef struct {
     /* Action to take at the end of a MSG IN phase.
        0 = COMMAND, 1 = disconnect, 2 = DATA OUT, 3 = DATA IN.  */
     int msg_action;
-    int msg_len;
+    uint8_t msg_len;

Not wrong per se, but it's also not clear why it's needed. I understand that you want to switch from signed to unsigned, but it is not mentioned in the commit message.

The switch to 8-bit, and below from VMSTATE_INT32 to VMSTATE_UINT8, is wrong because it changes the format of the live migration stream.

Quoted 7 lines

hw/scsi/lsi53c895a.c +0 -0

     uint8_t msg[LSI_MAX_MSGIN_LEN];
     /* 0 if SCRIPTS are running or stopped.
      * 1 if a Wait Reselect instruction has been issued.

@@ -861,12 +861,15 @@ static void lsi_do_status(LSIState *s)
 
 static void lsi_do_msgin(LSIState *s)
 {

hw/scsi/lsi53c895a.c static void lsi_do_status(LSIState *s) · line 872

Quoted 10 lines

hw/scsi/lsi53c895a.c +4 -1

-    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;
+    if (len > LSI_MAX_MSGIN_LEN) {
+        len = LSI_MAX_MSGIN_LEN;
+    }

I'm not sure it's appropriate to check for out of bounds reads here, because if s->msg_len is greater than LSI_MAX_MSGIN_LEN, then this test doesn't exclude you've already had an out of bounds write before. Indeed the msg_len is checked in lsi_add_msg_byte in order to avoid out of bounds accesses in either lsi_add_msg_byte or lsi_do_msgin. You could assert here that the variable is in range, I guess.

However, the out of bounds s->msg_len can actually happen in one other
case: namely, if a malicious live migration stream includes a bogus s->msg_len. Such live migration stream should be rejected; the fix for that is to add a lsi_post_load function, point to it in vmstate_lsi_scsi, and check there for s->msg_len <= LSI_MAX_MSGIN_LEN.

Thanks,

Paolo

Quoted 13 lines

hw/scsi/lsi53c895a.c +1 -1

     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];

@@ -2114,7 +2117,7 @@ static const VMStateDescription vmstate_lsi_scsi = {
         VMSTATE_INT32(carry, LSIState),
         VMSTATE_INT32(status, LSIState),
         VMSTATE_INT32(msg_action, LSIState),
-        VMSTATE_INT32(msg_len, LSIState),
+        VMSTATE_UINT8(msg_len, LSIState),
         VMSTATE_BUFFER(msg, LSIState),
         VMSTATE_INT32(waiting, LSIState),

P J P

author

2018-10-26 09:36 UTC #8 lore raw

+-- On Fri, 26 Oct 2018, Paolo Bonzini wrote --+
| > -    int msg_len;
| > +    uint8_t msg_len;
| 
| Not wrong per se, but it's also not clear why it's needed.  I understand
| that you want to switch from signed to unsigned, but it is not mentioned
| in the commit message.

Changed to uint8_t because IIUC 'msg_len' is likely be < LSI_MAX_MSGIN_LEN=8. And uint32_t seems rather large for it.

| The switch to 8-bit, and below from VMSTATE_INT32 to VMSTATE_UINT8, is | wrong because it changes the format of the live migration stream.

I see.

| I'm not sure it's appropriate to check for out of bounds reads here,
| because if s->msg_len is greater than LSI_MAX_MSGIN_LEN, then this test
| doesn't exclude you've already had an out of bounds write before.
| Indeed the msg_len is checked in lsi_add_msg_byte in order to avoid out
| of bounds accesses in either lsi_add_msg_byte or lsi_do_msgin. You
| could assert here that the variable is in range, I guess.

Okay.

| However, the out of bounds s->msg_len can actually happen in one other
| case: namely, if a malicious live migration stream includes a bogus
| s->msg_len.  Such live migration stream should be rejected; the fix for
| that is to add a lsi_post_load function, point to it in
| vmstate_lsi_scsi, and check there for s->msg_len <= LSI_MAX_MSGIN_LEN.

Okay, sending a revised patch v1 in a bit.

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