| Message ID | 20200513192540.1583887-3-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Megasas: fix OOB access and NULL dereference issues | expand |
On 200514 0055, P J P wrote: > From: Prasad J Pandit <pjp@fedoraproject.org> > > While in megasas_handle_frame(), megasas_enqueue_frame() may > set a NULL frame into MegasasCmd object for a given 'frame_addr' > address. Add check to avoid a NULL pointer dereference issue. > > Reported-by: Alexander Bulekov <alxndr@bu.edu> > Fixes: https://bugs.launchpad.net/qemu/+bug/1878259 > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> Acked-by: Alexander Bulekov <alxndr@bu.edu> > --- > hw/scsi/megasas.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c > index 6ce598cd69..b531d88a9b 100644 > --- a/hw/scsi/megasas.c > +++ b/hw/scsi/megasas.c > @@ -504,7 +504,7 @@ static MegasasCmd *megasas_enqueue_frame(MegasasState *s, > cmd->pa = frame; > /* Map all possible frames */ > cmd->frame = pci_dma_map(pcid, frame, &frame_size_p, 0); > - if (frame_size_p != frame_size) { > + if (!cmd->frame || frame_size_p != frame_size) { > trace_megasas_qf_map_failed(cmd->index, (unsigned long)frame); > if (cmd->frame) { > megasas_unmap_frame(s, cmd); > -- > 2.25.4 >
On Thursday, 2020-05-14 at 00:55:39 +0530, P J P wrote: > From: Prasad J Pandit <pjp@fedoraproject.org> > > While in megasas_handle_frame(), megasas_enqueue_frame() may > set a NULL frame into MegasasCmd object for a given 'frame_addr' > address. Add check to avoid a NULL pointer dereference issue. > > Reported-by: Alexander Bulekov <alxndr@bu.edu> > Fixes: https://bugs.launchpad.net/qemu/+bug/1878259 > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> Reviewed-by: Darren Kenny <darren.kenny@oracle.com> > --- > hw/scsi/megasas.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c > index 6ce598cd69..b531d88a9b 100644 > --- a/hw/scsi/megasas.c > +++ b/hw/scsi/megasas.c > @@ -504,7 +504,7 @@ static MegasasCmd *megasas_enqueue_frame(MegasasState *s, > cmd->pa = frame; > /* Map all possible frames */ > cmd->frame = pci_dma_map(pcid, frame, &frame_size_p, 0); > - if (frame_size_p != frame_size) { > + if (!cmd->frame || frame_size_p != frame_size) { > trace_megasas_qf_map_failed(cmd->index, (unsigned long)frame); > if (cmd->frame) { > megasas_unmap_frame(s, cmd); > -- > 2.25.4
On 13/05/20 21:25, P J P wrote: > From: Prasad J Pandit <pjp@fedoraproject.org> > > While in megasas_handle_frame(), megasas_enqueue_frame() may > set a NULL frame into MegasasCmd object for a given 'frame_addr' > address. Add check to avoid a NULL pointer dereference issue. > > Reported-by: Alexander Bulekov <alxndr@bu.edu> > Fixes: https://bugs.launchpad.net/qemu/+bug/1878259 > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > --- > hw/scsi/megasas.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c > index 6ce598cd69..b531d88a9b 100644 > --- a/hw/scsi/megasas.c > +++ b/hw/scsi/megasas.c > @@ -504,7 +504,7 @@ static MegasasCmd *megasas_enqueue_frame(MegasasState *s, > cmd->pa = frame; > /* Map all possible frames */ > cmd->frame = pci_dma_map(pcid, frame, &frame_size_p, 0); > - if (frame_size_p != frame_size) { > + if (!cmd->frame || frame_size_p != frame_size) { > trace_megasas_qf_map_failed(cmd->index, (unsigned long)frame); > if (cmd->frame) { > megasas_unmap_frame(s, cmd); > -- 2.25.4 > I think the code here was expecting frame_size_p to be 0 if cmd->frame is NULL. Can you check why this is not the case, or whether it ever was the case? Thanks, Paolo
Hello,
+-- On Thu, 21 May 2020, Paolo Bonzini wrote --+
| I think the code here was expecting frame_size_p to be 0 if cmd->frame is
| NULL. Can you check why this is not the case, or whether it ever was the
| case?
static MegasasCmd *megasas_enqueue_frame(MegasasState *s, hwaddr frame,
...
int frame_size = MEGASAS_MAX_SGE * sizeof(union mfi_sgl);
hwaddr frame_size_p = frame_size; <== = 128 * 16 = 2048
so 'frame_size_p' always starts with value '2048'
...
cmd->frame = pci_dma_map(pcid, frame, &frame_size_p, 0);
-> pci_dma_map
-> dma_memory_map
-> address_space_map
mr = flatview_translate(fv, addr, &xlat, &l, is_write, attrs);
...
if (atomic_xchg(&bounce.in_use, true)) {
return NULL; <== NULL is returned from here
}
Later when address_space_map() returns 'NULL' above, '*plen' is not set to
zero.
diff --git a/exec.c b/exec.c
index 5162f0d12f..4eea84bf66 100644
--- a/exec.c
+++ b/exec.c
@@ -3538,6 +3538,7 @@ void *address_space_map(AddressSpace *as,
if (!memory_access_is_direct(mr, is_write)) {
if (atomic_xchg(&bounce.in_use, true)) {
+ *plen = 0;
return NULL;
}
I'll send a revised patch above.
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
On 26/05/20 09:18, P J P wrote: > Later when address_space_map() returns 'NULL' above, '*plen' is not set to > zero. > > diff --git a/exec.c b/exec.c > index 5162f0d12f..4eea84bf66 100644 > --- a/exec.c > +++ b/exec.c > @@ -3538,6 +3538,7 @@ void *address_space_map(AddressSpace *as, > > if (!memory_access_is_direct(mr, is_write)) { > if (atomic_xchg(&bounce.in_use, true)) { > + *plen = 0; > return NULL; > } > > I'll send a revised patch above. Great, this looks good. Paolo
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c index 6ce598cd69..b531d88a9b 100644 --- a/hw/scsi/megasas.c +++ b/hw/scsi/megasas.c @@ -504,7 +504,7 @@ static MegasasCmd *megasas_enqueue_frame(MegasasState *s, cmd->pa = frame; /* Map all possible frames */ cmd->frame = pci_dma_map(pcid, frame, &frame_size_p, 0); - if (frame_size_p != frame_size) { + if (!cmd->frame || frame_size_p != frame_size) { trace_megasas_qf_map_failed(cmd->index, (unsigned long)frame); if (cmd->frame) { megasas_unmap_frame(s, cmd);