| Message ID | 20190808065636.28787-1-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ide: ahci: add check to avoid null dereference (CVE-2019-12067) | expand |
Hi Prasad, On 8/8/19 8:56 AM, P J P wrote: > From: Prasad J Pandit <pjp@fedoraproject.org> > > AHCI emulator while committing DMA buffer in ahci_commit_buf() > may do a NULL dereference if the command header 'ad->cur_cmd' > is null. Add check to avoid it. > > Reported-by: Bugs SysSec <bugs-syssec@rub.de> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > --- > hw/ide/ahci.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c > index 00ba422a48..9fff94075b 100644 > --- a/hw/ide/ahci.c > +++ b/hw/ide/ahci.c > @@ -1458,8 +1458,10 @@ static void ahci_commit_buf(IDEDMA *dma, uint32_t tx_bytes) > { > AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma); > > - tx_bytes += le32_to_cpu(ad->cur_cmd->status); > - ad->cur_cmd->status = cpu_to_le32(tx_bytes); > + if (ad->cur_cmd) { My 2 cents, John will correct me: This is not a valid condition, so an assert() might be more appropriate here. Why is dma_buf_commit() being called with a null command? This check should go elsewhere to avoid the call. > + tx_bytes += le32_to_cpu(ad->cur_cmd->status); > + ad->cur_cmd->status = cpu_to_le32(tx_bytes); > + } > } > > static int ahci_dma_rw_buf(IDEDMA *dma, int is_write) >
On 8/8/19 5:11 AM, Philippe Mathieu-Daudé wrote: > Hi Prasad, > > On 8/8/19 8:56 AM, P J P wrote: >> From: Prasad J Pandit <pjp@fedoraproject.org> >> >> AHCI emulator while committing DMA buffer in ahci_commit_buf() >> may do a NULL dereference if the command header 'ad->cur_cmd' >> is null. Add check to avoid it. >> >> Reported-by: Bugs SysSec <bugs-syssec@rub.de> >> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> >> --- >> hw/ide/ahci.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c >> index 00ba422a48..9fff94075b 100644 >> --- a/hw/ide/ahci.c >> +++ b/hw/ide/ahci.c >> @@ -1458,8 +1458,10 @@ static void ahci_commit_buf(IDEDMA *dma, uint32_t tx_bytes) >> { >> AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma); >> >> - tx_bytes += le32_to_cpu(ad->cur_cmd->status); >> - ad->cur_cmd->status = cpu_to_le32(tx_bytes); >> + if (ad->cur_cmd) { > > My 2 cents, John will correct me: > > This is not a valid condition, so an assert() might be more appropriate > here. > > Why is dma_buf_commit() being called with a null command? This check > should go elsewhere to avoid the call. > Yes, something else is broken. Can you please give a reproducer or more detailed report so I can fix this properly? This likely is just adding a whole host of new undefined problems waiting to happen by adding an if (...), so while it's not nice to have a host assert() it's probably better than taking the state machine off the rails and seeing what else breaks. >> + tx_bytes += le32_to_cpu(ad->cur_cmd->status); >> + ad->cur_cmd->status = cpu_to_le32(tx_bytes); >> + } >> } >> >> static int ahci_dma_rw_buf(IDEDMA *dma, int is_write) >>
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 00ba422a48..9fff94075b 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -1458,8 +1458,10 @@ static void ahci_commit_buf(IDEDMA *dma, uint32_t tx_bytes) { AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma); - tx_bytes += le32_to_cpu(ad->cur_cmd->status); - ad->cur_cmd->status = cpu_to_le32(tx_bytes); + if (ad->cur_cmd) { + tx_bytes += le32_to_cpu(ad->cur_cmd->status); + ad->cur_cmd->status = cpu_to_le32(tx_bytes); + } } static int ahci_dma_rw_buf(IDEDMA *dma, int is_write)