On 25/10/2018 22:09, P J P wrote: > From: Prasad J Pandit <p...@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.m...@oracle.com> > Signed-off-by: Prasad J Pandit <p...@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), > >