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

[Paolo Bonzini <pbonzini@redhat.com>]

On 01/12/20 16:00, P J P wrote:
> * 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

Yes, that part is correct.

> * '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'.

The command spec only matters to some extent (it matters more in writing 
a fix than in understanding the bug).

The questions to ask yourself after reading the code are:

1) ide_atapi_cmd_reply_end does contain an attempt at resetting an 
out-of-bounds io_buffer_index to 0.  Why is the reproducer bypassing it? 
  The answer must be because s->lba == -1.

2) what it means for ide_atapi_cmd_reply_end treats s->lba == -1 
differently.  The answer is that s->lba == -1 means the command is not a 
read (according to the code) but in this case you get there with a read.

> * I tested the patch with a reproducer and it helped to fix the crash.

Yes, but fixing the crash is not enough.  You need to ensure that the 
code makes sense.  You need to distinguish between violated 
preconditions and the consequences of those violations.  Once a 
precondition is violated, all bets are off on what happens in the code 
below it.  So if you don't catch the violated precondition at the 
_first_ place where it can happen, you can have other issues elsewhere.

So again the question to ask is, how do you get s->lba == -1 in read 
context?  Where did things go wrong?  Are there other read paths that 
can set s->lba == -1?  This is where reading the spec (as opposed to the 
code) starts to be important.

> * 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.

Right, catching the problem above means that you can just raise an ATAPI 
command error.

> | 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.

qtests are not just helpful.  Adding regression tests for bugs is a 
*basic* software engineering principle.  If you don't have time to write 
tests, you (or your organization) should find it.

But even if you don't write tests you need at least to enclose the 
reproducer, otherwise you're posting a puzzle not a patch. :)

Paolo