Hello Paolo, +-- On Tue, 1 Dec 2020, Paolo Bonzini wrote --+ | 1) Obviously this condition was not in the mind of whoever wrote the code. | For this reason the first thing to understand if how the bug came to happen, | which precondition was not respected. Your backtraces shows that you came | from ide_atapi_cmd_read_pio, so: | | - ide_atapi_cmd_reply_end is first entered with s->io_buffer_index == | s->cd_sector_size | | - s->lba is assumed to be != -1. from there you go to cd_read_sector -> | cd_read_sector_cb and then reenter ide_atapi_cmd_reply_end with | s->io_buffer_index == 0. Or to cd_read_sector_sync and then continue down | ide_atapi_cmd_reply_end, again with s->io_buffer_index == 0 | | - if s->elementary_transfer_size > 0, the number of bytes that are read is | bounded to s->cd_sector_size - s->io_buffer_index | | - if s->elementary_transfer_size == 0, the size is again bounded to | s->cd_sector_size - s->io_buffer_index in this code: | | /* we cannot transmit more than one sector at a time */ | if (s->lba != -1) { | if (size > (s->cd_sector_size - s->io_buffer_index)) | size = (s->cd_sector_size - s->io_buffer_index); | } | | So my understanding is that, for the bug to happen, you need to enter | ide_atapi_cmd_reply_end with s->lba == -1 despite being in the read CD path. | This might be possible by passing 0xFFFFFFFF as the LBA in cmd_read. | The correct fix would be to check lba against the media size in cmd_read.
Oh, okay. | This is reasoning that you should understand even before starting to write a | patch. Did you do this step? ... | 2)... So if you did do step 1, you need to explain it to me, because at this | point you know this part of the code better than I do. This is a step that | you did not do, because your commit message has no such explanation. -> https://tc.gts3.org/cs3210/2016/spring/r/hardware/ATA8-ACS.pdf Section #7.22 Packet command * Yes, I tried to follow the code with the above comand description as reference. * Because io_index was running past io_buffer, I was thikning around right lengths and sizes. Above specification mentions about 'Byte Count Limit' and that data transfer can not exceed that limit. * I was thinking about checking 'elementary_transfer_size' against 'byte_count_limit', but that did not work out. The loop is confusing there, it first sets elementary_size = size and subtracts the same * 's->lba == -1' did not strike me as wrong, because code explicitly checks it and gets past it. It does not flag an error when 's->lba == -1'. | If so, no problem---I still believe the patch is incorrect, but please | explain how my reasoning is wrong and we'll take it from there. If not, how | do you know your patch is not papering over a bigger issue somewhere? * I tested the patch with a reproducer and it helped to fix the crash. * My doubt about the patch was that it prematurely ends the while loop ie. before s->packet_transfer_size reaches zero(0), there may be possible data loss. | 3) We also need to ensure that the bug will not happen again. Did you get a | reproducer from the reporter? If not, how did you trust the report to be | correct? If so, did you try to include it in the QEMU qtest testsuite? * I did test it against a reproducer, but did not get to the qtest part for the time constraints. | If my understanding of the bug is correct, the patch is not the correct fix. | The correct fix is to add an assertion here *and* to fix the incorrect | assumption up in the call chain. For example: | | - add an assertion in ide_atapi_cmd_read_{dma,pio} that s->lba <= | s->nb_sectors && s->lba != -1 | | - add a range check in cmd_read and cmd_read_cd similar to what is already | done in cmd_seek (but checking the full range from lba to lba+nb_sectors-1. Okay, will do. | Even if the patch were correct, however, bullets (2) and (3) are two reasons | why this patch is not acceptable for QEMU's standards---not even for a | security fix. | | This is nothing new. Even though I have sometimes applied (or redone_ your | fixes, I have told you a variation of the above every time I saw a a patch of | yours. The output of "git log --author pjp tests" tells me you didn't heed | the advice though; I'm calling you out this time because it's especially clear | that you didn't do these steps and because the result is especially wrong. * While I understand and agree that having qtests is greatly helpful, I could not get to it due to time/cycles constraints. * It's certainly not that I purposely did not heed the advice/suggestions. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D