| Message ID | 1464172291-2856-2-git-send-email-ppandit@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 05/25/2016 12:31 PM, P J P wrote: > From: Prasad J Pandit <pjp@fedoraproject.org> > > When setting MegaRAID SAS controller properties via MegaRAID > Firmware Interface(MFI) commands, a user supplied size parameter > is used to set property value. Use appropriate size value to avoid > OOB access issues. > > Reported-by: Li Qiang <liqiang6-s@360.cn> > 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 a63a581..dcbd3e1 100644 > --- a/hw/scsi/megasas.c > +++ b/hw/scsi/megasas.c > @@ -1446,7 +1446,7 @@ static int megasas_dcmd_set_properties(MegasasState *s, MegasasCmd *cmd) > dcmd_size); > return MFI_STAT_INVALID_PARAMETER; > } > - dma_buf_write((uint8_t *)&info, cmd->iov_size, &cmd->qsg); > + dma_buf_write((uint8_t *)&info, dcmd_size, &cmd->qsg); This looks odd - can dcmd_size be bigger than iov_size? Wouldn't we overwrite guest memory then? And where does dcmd_size come from? I don't see it in master. Alex > trace_megasas_dcmd_unsupported(cmd->index, cmd->iov_size); > return MFI_STAT_OK; > }
Hello Alex, +-- On Wed, 25 May 2016, Alexander Graf wrote --+ | > - dma_buf_write((uint8_t *)&info, cmd->iov_size, &cmd->qsg); | > + dma_buf_write((uint8_t *)&info, dcmd_size, &cmd->qsg); | | This looks odd - can dcmd_size be bigger than iov_size? Wouldn't we overwrite | guest memory then? And where does dcmd_size come from? I don't see it in | master. struct mfi_ctrl_props info; size_t dcmd_size = sizeof(info); -> http://git.qemu.org/?p=qemu.git;a=blob;f=hw/scsi/megasas.c;h=a63a581550a328d0326ddee4f7fe1c4ffdecc194;hb=HEAD#l1439 'dcmd_size' is same as that of 'info' object. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
On 05/25/2016 01:51 PM, P J P wrote: > Hello Alex, > > +-- On Wed, 25 May 2016, Alexander Graf wrote --+ > | > - dma_buf_write((uint8_t *)&info, cmd->iov_size, &cmd->qsg); > | > + dma_buf_write((uint8_t *)&info, dcmd_size, &cmd->qsg); > | > | This looks odd - can dcmd_size be bigger than iov_size? Wouldn't we overwrite > | guest memory then? And where does dcmd_size come from? I don't see it in > | master. > > struct mfi_ctrl_props info; > size_t dcmd_size = sizeof(info); > > -> http://git.qemu.org/?p=qemu.git;a=blob;f=hw/scsi/megasas.c;h=a63a581550a328d0326ddee4f7fe1c4ffdecc194;hb=HEAD#l1439 > > 'dcmd_size' is same as that of 'info' object. Ok, then this patch is definitely bogus. The guest may receive less than the size of the info object. So we really want to have a MIN() between the maximum allowed transfer size (sizeof(info)) and the requested size (cmd->iov_size) here. Alex
+-- On Wed, 25 May 2016, Alexander Graf wrote --+ | > http://git.qemu.org/?p=qemu.git;a=blob;f=hw/scsi/megasas.c;h=a63a581550a328d0326ddee4f7fe1c4ffdecc194;hb=HEAD#l1439 | > 'dcmd_size' is same as that of 'info' object. | | Ok, then this patch is definitely bogus. The guest may receive less than the | size of the info object. So we really want to have a MIN() between the maximum | allowed transfer size (sizeof(info)) and the requested size (cmd->iov_size) | here. There is also a check which returns an invalid parameter error if 'cmd->iov_size' is less than 'dcmd_size'. Ie. OOB access occurs when cmd->iov_size is greater than 'dcmd_size'. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
On 05/25/2016 02:15 PM, P J P wrote: > +-- On Wed, 25 May 2016, Alexander Graf wrote --+ > | > http://git.qemu.org/?p=qemu.git;a=blob;f=hw/scsi/megasas.c;h=a63a581550a328d0326ddee4f7fe1c4ffdecc194;hb=HEAD#l1439 > | > 'dcmd_size' is same as that of 'info' object. > | > | Ok, then this patch is definitely bogus. The guest may receive less than the > | size of the info object. So we really want to have a MIN() between the maximum > | allowed transfer size (sizeof(info)) and the requested size (cmd->iov_size) > | here. > > There is also a check which returns an invalid parameter error if > 'cmd->iov_size' is less than 'dcmd_size'. Ie. OOB access occurs when > cmd->iov_size is greater than 'dcmd_size'. Turns out you're much better at reading code than me - yes, true, all is good :) Reviewed-by: Alexander Graf <agraf@suse.de> Alex
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c index a63a581..dcbd3e1 100644 --- a/hw/scsi/megasas.c +++ b/hw/scsi/megasas.c @@ -1446,7 +1446,7 @@ static int megasas_dcmd_set_properties(MegasasState *s, MegasasCmd *cmd) dcmd_size); return MFI_STAT_INVALID_PARAMETER; } - dma_buf_write((uint8_t *)&info, cmd->iov_size, &cmd->qsg); + dma_buf_write((uint8_t *)&info, dcmd_size, &cmd->qsg); trace_megasas_dcmd_unsupported(cmd->index, cmd->iov_size); return MFI_STAT_OK; }