P J P <ppan...@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 > ... > | I think it is better to defer this check to 'ide_cancel_dma_sync'. > | 'ide_cancel_dma_sync' is also called by 'cmd_device_reset' and all of the > | handlers of 'ide_cmd_table' will check whether the 's->blk' is NULL in the > | beginning of 'ide_exec_cmd'. > | > | So I think it is reasonable to check 's->blk' at the begining of > | 'ide_cancel_dma_sync'. > > * Yes, earlier patch v1 above does the same. > > * From Peter's reply in another thread of similar issue I gather, issue is > setting 'blk' to NULL, even erroneously. So (blk == NULL) check should be > done where 'blk' is set to null, rather than where it is dereferenced. >
I don't find anywhere set the 'blk' to NULL. I think this NULL deference is caused by following: In 'pci_ide_create_devs' we call 'ide_create_drive', in the latter function it will create the 's->blk'. However it is not for every IDEBus. IDEBus[0]: ifs[2] IDEBus[1]: ifs[2] The 'ide_create_drive' will only initialize the 'IDEBus[0].ifs[0]' and 'IDEBus[1].ifs[0]' from the reproducer command line. So the 'IDEBus[0].ifs[1]' and 'IDEBus[1].ifs[1]' s blk will be NULL. 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. 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. 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. BTW, where is the Peter's email saying this, just want to learn something, :). Thanks, Li Qiang > * At the dereference point, assert(3) is good. > > > Thank you. > -- > Prasad J Pandit / Red Hat Product Security Team > 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D >
