Re: [PATCH v2] hw/ide: check null block before _cancel_dma_sync

[P J P]

+-- On Tue, 29 Sep 2020, Li Qiang wrote --+
| P J P <ppandit@redhat.com> 于2020年9月29日周二 下午2:22写道：
| > +-- On Fri, 18 Sep 2020, Li Qiang wrote --+
| > | P J P <ppandit@redhat.com> 于2020年9月18日周五 下午6:26写道：
| > | > +-- On Fri, 18 Sep 2020, Li Qiang wrote --+
| > | > | Update v2: use an assert() call
| > | > |   
->https://lists.nongnu.org/archive/html/qemu-devel/2020-08/msg08336.html
| > |
| > | In 'ide_ioport_write' the guest can set 'bus->unit' to 0 or 1 by issue
| > | 'ATA_IOPORT_WR_DEVICE_HEAD'. So this case the guest can set the active 
ifs.
| > | If the guest set this to 1.
| > |
| > | Then in 'idebus_active_if' will return 'IDEBus.ifs[1]' and thus the 
's->blk'
| > | will be NULL.
| >
| > Right, guest does select the drive via
| >
| >   portio_write
| >    ->ide_ioport_write
| >       case ATA_IOPORT_WR_DEVICE_HEAD:
| >       /* FIXME: HOB readback uses bit 7 */
| >       bus->ifs[0].select = (val & ~0x10) | 0xa0;
| >       bus->ifs[1].select = (val | 0x10) | 0xa0;
| >       /* select drive */
| >       bus->unit = (val >> 4) & 1;     <== set bus->unit=0x1
| >       break;
| >
| >
| > | So from your (Peter's) saying, we need to check the value in
| > | 'ATA_IOPORT_WR_DEVICE_HEAD' handler. To say if the guest
| > | set a valid 'bus->unit'. This can also work I think.
| >
| > Yes, with the following fix, an assert(3) in ide_cancel_dma_sync fails.
| >
| > ===
| > diff --git a/hw/ide/core.c b/hw/ide/core.c
| > index f76f7e5234..cb55cc8b0f 100644
| > --- a/hw/ide/core.c
| > +++ b/hw/ide/core.c
| > @@ -1300,7 +1300,11 @@ void ide_ioport_write(void *opaque, uint32_t addr,
| > uint_)
| >          bus->ifs[0].select = (val & ~0x10) | 0xa0;
| >          bus->ifs[1].select = (val | 0x10) | 0xa0;
| >          /* select drive */
| > +        uint8_t bu = bus->unit;
| >          bus->unit = (val >> 4) & 1;
| > +        if (!bus->ifs[bus->unit].blk) {
| > +            bus->unit = bu;
| > +        }
| >          break;
| >      default:
| >
| > qemu-system-x86_64: ../hw/ide/core.c:724: ide_cancel_dma_sync: Assertion 
`s->bus->dma->aiocb == NULL' failed.
| > Aborted (core dumped)
| 
| This is what I am worried, in the 'ide_ioport_write' set the 'bus->unit'. It 
| also change the 'buf->ifs[0].select'. Also there maybe some other corner 
| case that causes some inconsistent. And if we choice this method we need to 
| deep into the more ahci-spec to know how things really going.
| 
| > ===
| >
| > | As we the 'ide_exec_cmd' and other functions in 'hw/ide/core.c' check the
| > | 's->blk' directly. I think we just check it in 'ide_cancel_dma_sync' is
| > | enough and also this is more consistent with the other functions.
| > | 'ide_cancel_dma_sync' is also called by 'cmd_device_reset' which is one of
| > | the 'ide_cmd_table' handler.
| >
| >   Yes, I'm okay with either approach. Earlier patch v1 checks 's->blk' in
| > ide_cancel_dma_sync().
| 
| I prefer the 'check the s->blk in the beginning of ide_cancel_dma_sync' 
method.
| Some little different with your earlier patch.
| 
| Anyway, let the maintainer do the choices.
| 

@John ...wdyt?

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D