> On 31 Aug 2016, at 09:49 AM, P J P <ppan...@redhat.com> wrote: > > From: Prasad J Pandit <p...@fedoraproject.org> > > Vmware Paravirtual SCSI emulation uses command descriptors to > process SCSI commands. These descriptors come with their ring > buffers. A guest could set the page count for these rings to > an arbitrary value, leading to infinite loop or OOB access. > Add check to avoid it. > > Reported-by: Tom Victor <vv474172...@gmail.com> > Signed-off-by: Prasad J Pandit <p...@fedoraproject.org> > --- > hw/scsi/vmw_pvscsi.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > Update: > Moved Request and Confirm rings page count check to the parent > function -> pvscsi_on_cmd_setup_rings(). > > diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c > index 5116f4a..79aa88c 100644 > --- a/hw/scsi/vmw_pvscsi.c > +++ b/hw/scsi/vmw_pvscsi.c > @@ -160,10 +160,6 @@ pvscsi_ring_init_data(PVSCSIRingInfo *m, > PVSCSICmdDescSetupRings *ri) > uint32_t req_ring_size, cmp_ring_size; > m->rs_pa = ri->ringsStatePPN << VMW_PAGE_SHIFT; > > - if ((ri->reqRingNumPages > PVSCSI_SETUP_RINGS_MAX_NUM_PAGES) > - || (ri->cmpRingNumPages > PVSCSI_SETUP_RINGS_MAX_NUM_PAGES)) { > - return -1; > - }
Hello Prasad, Why did you decide to move this logic out of pvscsi_ring_init_data()? Why not just amend existing “if" as you did in v1 of this patch? ~Dmitry > req_ring_size = ri->reqRingNumPages * PVSCSI_MAX_NUM_REQ_ENTRIES_PER_PAGE; > cmp_ring_size = ri->cmpRingNumPages * PVSCSI_MAX_NUM_CMP_ENTRIES_PER_PAGE; > txr_len_log2 = pvscsi_log2(req_ring_size - 1); > @@ -746,7 +742,7 @@ pvscsi_dbg_dump_tx_rings_config(PVSCSICmdDescSetupRings > *rc) > > trace_pvscsi_tx_rings_num_pages("Confirm Ring", rc->cmpRingNumPages); > for (i = 0; i < rc->cmpRingNumPages; i++) { > - trace_pvscsi_tx_rings_ppn("Confirm Ring", rc->reqRingPPNs[i]); > + trace_pvscsi_tx_rings_ppn("Confirm Ring", rc->cmpRingPPNs[i]); > } > } > > @@ -777,12 +773,17 @@ pvscsi_on_cmd_setup_rings(PVSCSIState *s) > PVSCSICmdDescSetupRings *rc = > (PVSCSICmdDescSetupRings *) s->curr_cmd_data; > > + if (!rc->reqRingNumPages > + || rc->reqRingNumPages > PVSCSI_SETUP_RINGS_MAX_NUM_PAGES > + || !rc->cmpRingNumPages > + || rc->cmpRingNumPages > PVSCSI_SETUP_RINGS_MAX_NUM_PAGES) { > + return PVSCSI_COMMAND_PROCESSING_FAILED; > + } > + > trace_pvscsi_on_cmd_arrived("PVSCSI_CMD_SETUP_RINGS"); > > pvscsi_dbg_dump_tx_rings_config(rc); > - if (pvscsi_ring_init_data(&s->rings, rc) < 0) { > - return PVSCSI_COMMAND_PROCESSING_FAILED; > - } > + pvscsi_ring_init_data(&s->rings, rc); > > s->rings_info_valid = TRUE; > return PVSCSI_COMMAND_PROCESSING_SUCCEEDED; > -- > 2.5.5 >