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

[P J P]

  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