| Message ID | 20181030062842.9792-1-ppandit@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] lsi53c895a: check message length value is valid | expand |
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
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),