| Message ID | 20181025200944.12113-1-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | lsi53c895a: check message length value | expand |
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 On 10/25/2018 03:09 PM, 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. > > Reported-by: Ameya More <ameya.more@oracle.com> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > --- > hw/scsi/lsi53c895a.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c > index d1e6534311..a266c5a113 100644 > --- a/hw/scsi/lsi53c895a.c > +++ b/hw/scsi/lsi53c895a.c > @@ -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), >
On 25/10/2018 22:09, 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. > > Reported-by: Ameya More <ameya.more@oracle.com> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > --- > hw/scsi/lsi53c895a.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c > index d1e6534311..a266c5a113 100644 > --- a/hw/scsi/lsi53c895a.c > +++ b/hw/scsi/lsi53c895a.c > @@ -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. > 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; > + } 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 > 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), > >
+-- 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
+-- 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
On 10/26/2018 4:25 AM, P J P wrote: > +-- 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
+-- 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
On 10/26/2018 1:37 PM, P J P wrote: > +-- 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. > Yes, please use that acknowledgement text in lieu of a 'Reported-by' line. Thanks, -Mark
+-- 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
diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c index d1e6534311..a266c5a113 100644 --- a/hw/scsi/lsi53c895a.c +++ b/hw/scsi/lsi53c895a.c @@ -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),