Re: [PATCH] ide:atapi: check io_buffer_index in ide_atapi_cmd_reply_end

[Paolo Bonzini <pbonzini@redhat.com>]

On 27/11/20 14:57, P J P wrote:
> +-- On Wed, 18 Nov 2020, P J P wrote --+
> | During data transfer via packet command in 'ide_atapi_cmd_reply_end'
> | 's->io_buffer_index' could exceed the 's->io_buffer' length, leading
> | to OOB access issue. Add check to avoid it.
> |  ...
> |  #9  ahci_pio_transfer ../hw/ide/ahci.c:1383
> |  #10 ide_transfer_start_norecurse ../hw/ide/core.c:553
> |  #11 ide_atapi_cmd_reply_end ../hw/ide/atapi.c:284
> |  #12 ide_atapi_cmd_read_pio ../hw/ide/atapi.c:329
> |  #13 ide_atapi_cmd_read ../hw/ide/atapi.c:442
> |  #14 cmd_read ../hw/ide/atapi.c:988
> |  #15 ide_atapi_cmd ../hw/ide/atapi.c:1352
> |  #16 ide_transfer_start ../hw/ide/core.c:561
> |  #17 cmd_packet ../hw/ide/core.c:1729
> |  #18 ide_exec_cmd ../hw/ide/core.c:2107
> |  #19 handle_reg_h2d_fis ../hw/ide/ahci.c:1267
> |  #20 handle_cmd ../hw/ide/ahci.c:1318
> |  #21 check_cmd ../hw/ide/ahci.c:592
> |  #22 ahci_port_write ../hw/ide/ahci.c:373
> |  #23 ahci_mem_write ../hw/ide/ahci.c:513
> |
> | Reported-by: Wenxiang Qian <leonwxqian@gmail.com>
> | Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>

Prasad, please try to put yourself in the reviewer's shoes.

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.

This is reasoning that you should understand even before starting to 
write a patch.  Did you do this step?  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?


2) The maintainer is not the one that knows the code best: it's only 
someone who is trusted to make judgment calls that are good enough.  My 
job is to poke holes in your reasoning, not to reverse engineer it.  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.

There's also another reason to do this: recording the details of the bug 
in the commit message helps anyone who wants to understand the story of 
the code.


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?


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.


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.

Thanks,

Paolo