[PATCH] ide:atapi: check io_buffer_index in ide_atapi_cmd_reply_end

[PATCH] ide:atapi: check io_buffer_index in ide_atapi_cmd_reply_end

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

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>
---
 hw/ide/atapi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 14a2b0bb2f..b991947c5c 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -276,6 +276,9 @@ void ide_atapi_cmd_reply_end(IDEState *s)
         s->packet_transfer_size -= size;
         s->elementary_transfer_size -= size;
         s->io_buffer_index += size;
+        if (s->io_buffer_index > s->io_buffer_total_len) {
+            return;
+        }
 
         /* Some adapters process PIO data right away.  In that case, we need
          * to avoid mutual recursion between ide_transfer_start
-- 
2.28.0

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

[P J P]

+-- 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>
| ---
|  hw/ide/atapi.c | 3 +++
|  1 file changed, 3 insertions(+)
| 
| diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
| index 14a2b0bb2f..b991947c5c 100644
| --- a/hw/ide/atapi.c
| +++ b/hw/ide/atapi.c
| @@ -276,6 +276,9 @@ void ide_atapi_cmd_reply_end(IDEState *s)
|          s->packet_transfer_size -= size;
|          s->elementary_transfer_size -= size;
|          s->io_buffer_index += size;
| +        if (s->io_buffer_index > s->io_buffer_total_len) {
| +            return;
| +        }
|  
|          /* Some adapters process PIO data right away.  In that case, we need
|           * to avoid mutual recursion between ide_transfer_start
| 

Ping...!
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

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

[Paolo Bonzini]

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

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

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

[Philippe Mathieu-Daudé]

Hi Prasad,

On 12/1/20 4:00 PM, 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   

If the code is confusing, you can rewrite in a less confuse way :)
That way the problem are easier to notice.

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

[thread hijack]

About reproducer, Michael asked about CVE-2020-24352 (ati_vga OOB in
ati_2d_blt) this morning. What happens to reproducers when a CVE is
assigned, but the bug is marked as "out of the QEMU security boundary"?

Is it possible to release the reproducer to the community, so we can
work on a fix and test it?

Thanks,

Phil.

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

[Peter Maydell]

On Tue, 1 Dec 2020 at 15:28, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> About reproducer, Michael asked about CVE-2020-24352 (ati_vga OOB in
> ati_2d_blt) this morning. What happens to reproducers when a CVE is
> assigned, but the bug is marked as "out of the QEMU security boundary"?

Also, why are we assigning CVEs for bugs we don't consider security bugs?

thanks
-- PMM

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

[Paolo Bonzini]

On 01/12/20 16:30, Peter Maydell wrote:
> On Tue, 1 Dec 2020 at 15:28, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>> About reproducer, Michael asked about CVE-2020-24352 (ati_vga OOB in
>> ati_2d_blt) this morning. What happens to reproducers when a CVE is
>> assigned, but the bug is marked as "out of the QEMU security boundary"?
> 
> Also, why are we assigning CVEs for bugs we don't consider security bugs?

Sometimes CVEs are requested by other entities even before reaching the 
QEMU security mailing list.

Paolo

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

[Paolo Bonzini]

On 01/12/20 16:23, Philippe Mathieu-Daudé wrote:
> Hi Prasad,
> 
> On 12/1/20 4:00 PM, 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
> 
> If the code is confusing, you can rewrite in a less confuse way :)
> That way the problem are easier to notice.
> 
>> * I tested the patch with a reproducer and it helped to fix the crash.
> 
> [thread hijack]
> 
> About reproducer, Michael asked about CVE-2020-24352 (ati_vga OOB in
> ati_2d_blt) this morning. What happens to reproducers when a CVE is
> assigned, but the bug is marked as "out of the QEMU security boundary"?
> 
> Is it possible to release the reproducer to the community, so we can
> work on a fix and test it?

Ideally it should be, in the shape of a qtest. :)

Paolo

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

[Paolo Bonzini]

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

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

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

Indeed.

Posting puzzles is a negative-sum game.  You save a little time, but
your reviewers have to pay back, with usurious interest.  Depending on
how nice they are, they may even do it a few times for you, but
eventually even the nicest ones will take one look at your patch, and
bounce it right back to you.  Mission accomplished: all terms in the
negative sum are now negative.

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

[P J P]

[-- Attachment #1: Type: text/plain, Size: 2232 bytes --]

  Hi,

[doing a combined reply]

+-- On Tue, 1 Dec 2020, Philippe Mathieu-Daudé wrote --+ 
| Is it possible to release the reproducer to the community, so we can work on 
| a fix and test it?

* No, we can not release/share reproducers on a public list.

* We can request reporters to do so by their volition.


| What happens to reproducers when a CVE is assigned, but the bug is marked as 
| "out of the QEMU security boundary"?
|
+-- On Tue, 1 Dec 2020, Peter Maydell wrote --+
| Also, why are we assigning CVEs for bugs we don't consider security bugs?

* We need to mark these componets 'out of security scope' at the source level, 
  rather than on each bug.

* Easiest could be to just have a list of such components in the git text 
  file. At least till the time we device --security build and run time option 
  discussed earlier.
  -> https://lists.nongnu.org/archive/html/qemu-devel/2020-07/msg04680.html

+-- On Tue, 1 Dec 2020, Paolo Bonzini wrote --+
| 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.

* I've been doing the patch work out of my own interest.

* We generally rely on upstream/engineering for fix patches, because of our 
  narrower understanding of the code base.

+-- On Wed, 2 Dec 2020, Markus Armbruster wrote --+
| Paolo Bonzini <pbonzini@redhat.com> writes:
| > you need at least to enclose the reproducer, otherwise you're posting a 
| > puzzle not a patch. :)
| 
| Indeed. Posting puzzles is a negative-sum game.]

* Agreed. I think the upcoming 'qemu-security' list may help in this regard.  
  As issue reports+reproducer details shall go there.

* Even then, we'll need to ask reporter's permission before sharing their 
  reproducers on a public list OR with non-members.

* Best is if reporters share/release reproducers themselves. Maybe we can have 
  a public git repository and they can send a PR to include their reproducers 
  in the repository.

* That way multiple reproducers for the same issue can be held together.


Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

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

[Paolo Bonzini]

On 02/12/20 14:17, P J P wrote:
>    Hi,
> 
> [doing a combined reply]
> 
> +-- On Tue, 1 Dec 2020, Philippe Mathieu-Daudé wrote --+
> | Is it possible to release the reproducer to the community, so we can work on
> | a fix and test it?
> 
> * No, we can not release/share reproducers on a public list.

We do not need researchers to release or share reproducers though, we 
"only" encourage people who contribute bugfixes (esp. for 
security-sensitive issues) to add a qtest testcase.  Often the testcase 
would have no resemblance to the original reproducer (or even always, 
except for fuzzed issues).

Would that be allowed?  If not, we have a problem, because it means we 
cannot follow the basic principle of regression testing.

> * I've been doing the patch work out of my own interest.

Ok, that at least wasn't clear to me.  Thanks for making it clearer.

Paolo

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

[Philippe Mathieu-Daudé]

On 12/2/20 2:17 PM, P J P wrote:
> +-- On Tue, 1 Dec 2020, Philippe Mathieu-Daudé wrote --+ 
> | Is it possible to release the reproducer to the community, so we can work on 
> | a fix and test it?
> 
> * No, we can not release/share reproducers on a public list.
> 
> * We can request reporters to do so by their volition.
> 
[...]
> 
> * Even then, we'll need to ask reporter's permission before sharing their 
>   reproducers on a public list OR with non-members.
> 
> * Best is if reporters share/release reproducers themselves. Maybe we can have 
>   a public git repository and they can send a PR to include their reproducers 
>   in the repository.

While EDK2 security workflow has its own drawbacks (inherent
to the project), a fair part is to ask the reporter to attach
its reproducer to the private BZ, then when the embargo expires
the BZ becomes public (as the reproducer). Thus the community
can look at how the bug was handled, how it was reviewed/tested,
by who, etc.

https://github.com/tianocore/tianocore.github.io/wiki/Reporting-Security-Issues

> 
> * That way multiple reproducers for the same issue can be held together.

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

[P J P]

[-- Attachment #1: Type: text/plain, Size: 300 bytes --]

+-- On Wed, 2 Dec 2020, Philippe Mathieu-Daudé wrote --+
| a fair part is to ask the reporter to attach its reproducer to the private 
| BZ,

Yes, reporters sharing/releasing it is best.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

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

[Wenxiang Qian]

[-- Attachment #1: Type: text/plain, Size: 8998 bytes --]

Hello,

I may not have made the detail clear in my previous email. The details of
the AHCI device, after running the reproducer I attached in my report are
as follows. If there is any information I can provide, please let me know.
Thank you.

###root cause###
(1) The s->packet_transfer_size is bigger than the actual data.
(2) packet_transfer_size is passed into  ide_atapi_cmd_reply_end, as the
total number of iterations. Each iterate round, s->io_buffer_index is
increased by 2048, but without boundary check.
(3) The call to ide_transfer_start_norecurse use s->io_buffer +
s->io_buffer_index - size as the index, cause an OOB access.

###details###
1. The reproducer sends a command of [WIN_PACKETCMD] + [CMD_READ] and value
of IDE device's registers from guest to qemu.

Callback ahci_port_write is called, then check_cmd is called.

2. The packet will set all the registers of the device via: handle_cmd -->
handle_reg_h2d_fis.

Registers will be set here:

handle_reg_h2d_fis(..){
...
    ide_state->feature = cmd_fis[3];   //######[1]###### , cmd_fis is the
data sent by the reproducer.
    ide_state->sector = cmd_fis[4];      /* LBA 7:0 */
    ide_state->lcyl = cmd_fis[5];        /* LBA 15:8  */
    ide_state->hcyl = cmd_fis[6];        /* LBA 23:16 */
    ide_state->select = cmd_fis[7];      /* LBA 27:24 (LBA28) */
    ide_state->hob_sector = cmd_fis[8];  /* LBA 31:24 */
    ide_state->hob_lcyl = cmd_fis[9];    /* LBA 39:32 */
    ide_state->hob_hcyl = cmd_fis[10];   /* LBA 47:40 */
    ide_state->hob_feature = cmd_fis[11];
    ide_state->nsector = (int64_t)((cmd_fis[13] << 8) | cmd_fis[12]);

 and ide_exec_cmd will be called to process [WIN_PACKETCMD] command.
     ide_exec_cmd(&s->dev[port].port, cmd_fis[2]);

3. ide_exec_cmd (core.c) handles the command,

    [WIN_PACKETCMD]               = { cmd_packet, CD_OK },

and make a call to cmd_packet,

cmd_packet(...) {
    ...

s->atapi_dma = s->feature & 1;  //######[2]######
    if (s->atapi_dma) {
        s->dma_cmd = IDE_DMA_ATAPI;
    }
    s->nsector = 1;
    ide_transfer_start(s, s->io_buffer, ATAPI_PACKET_SIZE,
                       ide_atapi_cmd);
    ...
}

and set the device to use PIO mode according to s->feature (set in Step
2->##[1]##).

Then, ide_transfer_start is called.
It will pass the [CMD_READ] part after [WIN_PACKETCMD] to ide_atapi_cmd.

4. ide_atapi_cmd parses [CMD_READ], and then calls the corresponding
handler: cmd_read.

    [ 0x28 ] = { cmd_read, /* (10) */               CHECK_READY },

In cmd_read, the values of nb_sectors and lba are determined according to
the packets passed by the reproducer.

In my PoC I set lba to -1 (0xfffffff) and nb_sectors to a large value, such
as 0x800.


static void cmd_read(IDEState *s, uint8_t* buf)
{
    int nb_sectors, lba;

    if (buf[0] == GPCMD_READ_10) {
        nb_sectors = lduw_be_p(buf + 7);
    } else {
        nb_sectors = ldl_be_p(buf + 6);   //#####3#####
    }

    lba = ldl_be_p(buf + 2);   //######4######

....

    ide_atapi_cmd_read(s, lba, nb_sectors, 2048);
}

5. The code enters the ide_atapi_cmd_read -> ide_atapi_cmd_read_pio.

static void ide_atapi_cmd_read(.....)
{...
    if (s->atapi_dma) {
        ide_atapi_cmd_read_dma(s, lba, nb_sectors, sector_size);
    } else {
        ide_atapi_cmd_read_pio(s, lba, nb_sectors, sector_size);
 //######5#######
    }
}

It will set the attributes according to the value passed in the previous
steps.
The number of s->packet_transfer_size, which is the packet to read or
write, nb_sectors * 2048 can be larger than the buffer pre-allocated by
qemu (about 256KB).


static void ide_atapi_cmd_read_pio(IDEState *s, int lba, int nb_sectors,
                                   int sector_size)
{
    s->lba = lba;
    s->packet_transfer_size = nb_sectors * sector_size;
 //########6#########
    s->elementary_transfer_size = 0;
    s->io_buffer_index = sector_size;
    s->cd_sector_size = sector_size;

    ide_atapi_cmd_reply_end(s);
}


6. In ide_atapi_cmd_reply_end, the data is processed according to
packet_transfer_size.

void ide_atapi_cmd_reply_end(IDEState *s)
{
...
    while (s->packet_transfer_size > 0) {  //########7#######
....
        s->packet_transfer_size -= size;
        s->elementary_transfer_size -= size;
        s->io_buffer_index += size;  //#######8#######

        if (!ide_transfer_start_norecurse(s,
                                          s->io_buffer + s->io_buffer_index
- size,
                                          size, ide_atapi_cmd_reply_end)) {
            return;
        }


The "size" is usually 2048, which means the io_buffer_index increases by
2048 per round.

Qemu does not test if the result of this operation exceeds the length of
the io_buffer itself, resulting in out-of-bounds access.

In ide_transfer_start_norecurse,

bool ide_transfer_start_norecurse(IDEState *s, uint8_t *buf, int size,
                                  EndTransferFunc *end_transfer_func)
{
    s->data_ptr = buf;         //s->io_buffer + s->io_buffer_index - size
    s->data_end = buf + size;  //data_ptr + 2048
....
    s->bus->dma->ops->pio_transfer(s->bus->dma);  //#######9########
    return true;
}

//####9####:
static const IDEDMAOps ahci_dma_ops = {
...
    .pio_transfer = ahci_pio_transfer,
...
};

In the final processing function ahci_pio_transfer:

static void ahci_pio_transfer(const IDEDMA *dma)
{
....

    uint32_t size = (uint32_t)(s->data_end - s->data_ptr);  // 2048, usually

uint16_t opts = le16_to_cpu(ad->cur_cmd->opts);  //####user controlled
value#####
    int is_write = opts & AHCI_CMD_WRITE;            // read or write is
decided by user.

.....


    if (has_sglist && size) {
        if (is_write) {
            dma_buf_write(s->data_ptr, size, &s->sg);   //##10##### both
can be reached ####
        } else {
            dma_buf_read(s->data_ptr, size, &s->sg);    //##11##### both
can be reached ####
        }
    }
}


s->data_ptr can be a value out of range, so base on ad->cur_cmd->opts,
 ##10## ##11## can be OOB read or OOB write.

OOB read: obtain the leaked information, which can be used to bypass ASLR
or obtain information about the host.
OOB write: which may overwrite some structs of the virtual device after it,
overwrite the function pointers in the struct.

Best regards,
Wenxiang Qian

P J P <ppandit@redhat.com> 于2020年12月2日周三 下午9:17写道：

>   Hi,
>
> [doing a combined reply]
>
> +-- On Tue, 1 Dec 2020, Philippe Mathieu-Daudé wrote --+
> | Is it possible to release the reproducer to the community, so we can
> work on
> | a fix and test it?
>
> * No, we can not release/share reproducers on a public list.
>
> * We can request reporters to do so by their volition.
>
>
> | What happens to reproducers when a CVE is assigned, but the bug is
> marked as
> | "out of the QEMU security boundary"?
> |
> +-- On Tue, 1 Dec 2020, Peter Maydell wrote --+
> | Also, why are we assigning CVEs for bugs we don't consider security bugs?
>
> * We need to mark these componets 'out of security scope' at the source
> level,
>   rather than on each bug.
>
> * Easiest could be to just have a list of such components in the git text
>   file. At least till the time we device --security build and run time
> option
>   discussed earlier.
>   ->
> https://lists.nongnu.org/archive/html/qemu-devel/2020-07/msg04680.html
>
> +-- On Tue, 1 Dec 2020, Paolo Bonzini wrote --+
> | 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.
>
> * I've been doing the patch work out of my own interest.
>
> * We generally rely on upstream/engineering for fix patches, because of
> our
>   narrower understanding of the code base.
>
> +-- On Wed, 2 Dec 2020, Markus Armbruster wrote --+
> | Paolo Bonzini <pbonzini@redhat.com> writes:
> | > you need at least to enclose the reproducer, otherwise you're posting
> a
> | > puzzle not a patch. :)
> |
> | Indeed. Posting puzzles is a negative-sum game.]
>
> * Agreed. I think the upcoming 'qemu-security' list may help in this
> regard.
>   As issue reports+reproducer details shall go there.
>
> * Even then, we'll need to ask reporter's permission before sharing their
>   reproducers on a public list OR with non-members.
>
> * Best is if reporters share/release reproducers themselves. Maybe we can
> have
>   a public git repository and they can send a PR to include their
> reproducers
>   in the repository.
>
> * That way multiple reproducers for the same issue can be held together.
>
>
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

[-- Attachment #2: Type: text/html, Size: 10685 bytes --]

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

[Wenxiang Qian]

[-- Attachment #1: Type: text/plain, Size: 10189 bytes --]

+ The lba is set to -1 to avoid some code paths, to make PoC simpler.

void ide_atapi_cmd_reply_end(IDEState *s)
{
    int byte_count_limit, size, ret;
    while (s->packet_transfer_size > 0) {
.....
        if (s->lba != -1 && s->io_buffer_index >= s->cd_sector_size) {
<----- set lba to -1 to avoid this part
 .....
        }
        if (s->elementary_transfer_size > 0) {
......
        } else {
.......
            if (s->lba != -1) {   <-----
                if (size > (s->cd_sector_size - s->io_buffer_index))
                    size = (s->cd_sector_size - s->io_buffer_index);  <-----
            }
        }

Wenxiang Qian <leonwxqian@gmail.com> 于2020年12月11日周五 下午4:23写道：

> Hello,
>
> I may not have made the detail clear in my previous email. The details of
> the AHCI device, after running the reproducer I attached in my report are
> as follows. If there is any information I can provide, please let me know.
> Thank you.
>
> ###root cause###
> (1) The s->packet_transfer_size is bigger than the actual data.
> (2) packet_transfer_size is passed into  ide_atapi_cmd_reply_end, as the
> total number of iterations. Each iterate round, s->io_buffer_index is
> increased by 2048, but without boundary check.
> (3) The call to ide_transfer_start_norecurse use s->io_buffer +
> s->io_buffer_index - size as the index, cause an OOB access.
>
> ###details###
> 1. The reproducer sends a command of [WIN_PACKETCMD] + [CMD_READ] and
> value of IDE device's registers from guest to qemu.
>
> Callback ahci_port_write is called, then check_cmd is called.
>
> 2. The packet will set all the registers of the device via: handle_cmd -->
> handle_reg_h2d_fis.
>
> Registers will be set here:
>
> handle_reg_h2d_fis(..){
> ...
>     ide_state->feature = cmd_fis[3];   //######[1]###### , cmd_fis is the
> data sent by the reproducer.
>     ide_state->sector = cmd_fis[4];      /* LBA 7:0 */
>     ide_state->lcyl = cmd_fis[5];        /* LBA 15:8  */
>     ide_state->hcyl = cmd_fis[6];        /* LBA 23:16 */
>     ide_state->select = cmd_fis[7];      /* LBA 27:24 (LBA28) */
>     ide_state->hob_sector = cmd_fis[8];  /* LBA 31:24 */
>     ide_state->hob_lcyl = cmd_fis[9];    /* LBA 39:32 */
>     ide_state->hob_hcyl = cmd_fis[10];   /* LBA 47:40 */
>     ide_state->hob_feature = cmd_fis[11];
>     ide_state->nsector = (int64_t)((cmd_fis[13] << 8) | cmd_fis[12]);
>
>  and ide_exec_cmd will be called to process [WIN_PACKETCMD] command.
>      ide_exec_cmd(&s->dev[port].port, cmd_fis[2]);
>
> 3. ide_exec_cmd (core.c) handles the command,
>
>     [WIN_PACKETCMD]               = { cmd_packet, CD_OK },
>
> and make a call to cmd_packet,
>
> cmd_packet(...) {
>     ...
>
> s->atapi_dma = s->feature & 1;  //######[2]######
>     if (s->atapi_dma) {
>         s->dma_cmd = IDE_DMA_ATAPI;
>     }
>     s->nsector = 1;
>     ide_transfer_start(s, s->io_buffer, ATAPI_PACKET_SIZE,
>                        ide_atapi_cmd);
>     ...
> }
>
> and set the device to use PIO mode according to s->feature (set in Step
> 2->##[1]##).
>
> Then, ide_transfer_start is called.
> It will pass the [CMD_READ] part after [WIN_PACKETCMD] to ide_atapi_cmd.
>
> 4. ide_atapi_cmd parses [CMD_READ], and then calls the corresponding
> handler: cmd_read.
>
>     [ 0x28 ] = { cmd_read, /* (10) */               CHECK_READY },
>
> In cmd_read, the values of nb_sectors and lba are determined according to
> the packets passed by the reproducer.
>
> In my PoC I set lba to -1 (0xfffffff) and nb_sectors to a large value,
> such as 0x800.
>
>
> static void cmd_read(IDEState *s, uint8_t* buf)
> {
>     int nb_sectors, lba;
>
>     if (buf[0] == GPCMD_READ_10) {
>         nb_sectors = lduw_be_p(buf + 7);
>     } else {
>         nb_sectors = ldl_be_p(buf + 6);   //#####3#####
>     }
>
>     lba = ldl_be_p(buf + 2);   //######4######
>
> ....
>
>     ide_atapi_cmd_read(s, lba, nb_sectors, 2048);
> }
>
> 5. The code enters the ide_atapi_cmd_read -> ide_atapi_cmd_read_pio.
>
> static void ide_atapi_cmd_read(.....)
> {...
>     if (s->atapi_dma) {
>         ide_atapi_cmd_read_dma(s, lba, nb_sectors, sector_size);
>     } else {
>         ide_atapi_cmd_read_pio(s, lba, nb_sectors, sector_size);
>  //######5#######
>     }
> }
>
> It will set the attributes according to the value passed in the previous
> steps.
> The number of s->packet_transfer_size, which is the packet to read or
> write, nb_sectors * 2048 can be larger than the buffer pre-allocated by
> qemu (about 256KB).
>
>
> static void ide_atapi_cmd_read_pio(IDEState *s, int lba, int nb_sectors,
>                                    int sector_size)
> {
>     s->lba = lba;
>     s->packet_transfer_size = nb_sectors * sector_size;
>  //########6#########
>     s->elementary_transfer_size = 0;
>     s->io_buffer_index = sector_size;
>     s->cd_sector_size = sector_size;
>
>     ide_atapi_cmd_reply_end(s);
> }
>
>
> 6. In ide_atapi_cmd_reply_end, the data is processed according to
> packet_transfer_size.
>
> void ide_atapi_cmd_reply_end(IDEState *s)
> {
> ...
>     while (s->packet_transfer_size > 0) {  //########7#######
> ....
>         s->packet_transfer_size -= size;
>         s->elementary_transfer_size -= size;
>         s->io_buffer_index += size;  //#######8#######
>
>         if (!ide_transfer_start_norecurse(s,
>                                           s->io_buffer +
> s->io_buffer_index - size,
>                                           size, ide_atapi_cmd_reply_end)) {
>             return;
>         }
>
>
> The "size" is usually 2048, which means the io_buffer_index increases by
> 2048 per round.
>
> Qemu does not test if the result of this operation exceeds the length of
> the io_buffer itself, resulting in out-of-bounds access.
>
> In ide_transfer_start_norecurse,
>
> bool ide_transfer_start_norecurse(IDEState *s, uint8_t *buf, int size,
>                                   EndTransferFunc *end_transfer_func)
> {
>     s->data_ptr = buf;         //s->io_buffer + s->io_buffer_index - size
>     s->data_end = buf + size;  //data_ptr + 2048
> ....
>     s->bus->dma->ops->pio_transfer(s->bus->dma);  //#######9########
>     return true;
> }
>
> //####9####:
> static const IDEDMAOps ahci_dma_ops = {
> ...
>     .pio_transfer = ahci_pio_transfer,
> ...
> };
>
> In the final processing function ahci_pio_transfer:
>
> static void ahci_pio_transfer(const IDEDMA *dma)
> {
> ....
>
>     uint32_t size = (uint32_t)(s->data_end - s->data_ptr);  // 2048,
> usually
>
> uint16_t opts = le16_to_cpu(ad->cur_cmd->opts);  //####user controlled
> value#####
>     int is_write = opts & AHCI_CMD_WRITE;            // read or write is
> decided by user.
>
> .....
>
>
>     if (has_sglist && size) {
>         if (is_write) {
>             dma_buf_write(s->data_ptr, size, &s->sg);   //##10##### both
> can be reached ####
>         } else {
>             dma_buf_read(s->data_ptr, size, &s->sg);    //##11##### both
> can be reached ####
>         }
>     }
> }
>
>
> s->data_ptr can be a value out of range, so base on ad->cur_cmd->opts,
>  ##10## ##11## can be OOB read or OOB write.
>
> OOB read: obtain the leaked information, which can be used to bypass ASLR
> or obtain information about the host.
> OOB write: which may overwrite some structs of the virtual device after
> it, overwrite the function pointers in the struct.
>
> Best regards,
> Wenxiang Qian
>
> P J P <ppandit@redhat.com> 于2020年12月2日周三 下午9:17写道：
>
>>   Hi,
>>
>> [doing a combined reply]
>>
>> +-- On Tue, 1 Dec 2020, Philippe Mathieu-Daudé wrote --+
>> | Is it possible to release the reproducer to the community, so we can
>> work on
>> | a fix and test it?
>>
>> * No, we can not release/share reproducers on a public list.
>>
>> * We can request reporters to do so by their volition.
>>
>>
>> | What happens to reproducers when a CVE is assigned, but the bug is
>> marked as
>> | "out of the QEMU security boundary"?
>> |
>> +-- On Tue, 1 Dec 2020, Peter Maydell wrote --+
>> | Also, why are we assigning CVEs for bugs we don't consider security
>> bugs?
>>
>> * We need to mark these componets 'out of security scope' at the source
>> level,
>>   rather than on each bug.
>>
>> * Easiest could be to just have a list of such components in the git text
>>   file. At least till the time we device --security build and run time
>> option
>>   discussed earlier.
>>   ->
>> https://lists.nongnu.org/archive/html/qemu-devel/2020-07/msg04680.html
>>
>> +-- On Tue, 1 Dec 2020, Paolo Bonzini wrote --+
>> | 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.
>>
>> * I've been doing the patch work out of my own interest.
>>
>> * We generally rely on upstream/engineering for fix patches, because of
>> our
>>   narrower understanding of the code base.
>>
>> +-- On Wed, 2 Dec 2020, Markus Armbruster wrote --+
>> | Paolo Bonzini <pbonzini@redhat.com> writes:
>> | > you need at least to enclose the reproducer, otherwise you're posting
>> a
>> | > puzzle not a patch. :)
>> |
>> | Indeed. Posting puzzles is a negative-sum game.]
>>
>> * Agreed. I think the upcoming 'qemu-security' list may help in this
>> regard.
>>   As issue reports+reproducer details shall go there.
>>
>> * Even then, we'll need to ask reporter's permission before sharing their
>>   reproducers on a public list OR with non-members.
>>
>> * Best is if reporters share/release reproducers themselves. Maybe we can
>> have
>>   a public git repository and they can send a PR to include their
>> reproducers
>>   in the repository.
>>
>> * That way multiple reproducers for the same issue can be held together.
>>
>>
>> Thank you.
>> --
>> Prasad J Pandit / Red Hat Product Security Team
>> 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
>
>

[-- Attachment #2: Type: text/html, Size: 11910 bytes --]

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

[Paolo Bonzini]

On 11/12/20 09:32, Wenxiang Qian wrote:
> + The lba is set to -1 to avoid some code paths, to make PoC simpler.
> 
> void ide_atapi_cmd_reply_end(IDEState *s)
> {
>      int byte_count_limit, size, ret;
>      while (s->packet_transfer_size > 0) {
> .....
>          if (s->lba != -1 && s->io_buffer_index >= s->cd_sector_size) { 
> <----- set lba to -1 to avoid this part
>   .....
>          }
>          if (s->elementary_transfer_size > 0) {
> ......
>          } else {
> .......
>              if (s->lba != -1) { <-----
>                  if (size > (s->cd_sector_size - s->io_buffer_index))
>                      size = (s->cd_sector_size - s->io_buffer_index);  
> <-----
>              }
>          }
> 

If the lba is not -1, I don't think bad things can happen on this path. 
  Am I wrong?

Paolo

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

[Paolo Bonzini]

On 11/12/20 09:23, Wenxiang Qian wrote:
> Hello,
> 
> I may not have made the detail clear in my previous email. The details 
> of the AHCI device, after running the reproducer I attached in my report 
> are as follows. If there is any information I can provide, please let me 
> know. Thank you.
> 
> ###root cause###
> (1) The s->packet_transfer_size is bigger than the actual data.
> (2) packet_transfer_size is passed into  ide_atapi_cmd_reply_end, as the 
> total number of iterations. Each iterate round, s->io_buffer_index is 
> increased by 2048, but without boundary check.
> (3) The call to ide_transfer_start_norecurse use s->io_buffer + 
> s->io_buffer_index - size as the index, cause an OOB access.

This is not the root cause.  These are the last steps before bad things 
happen; the root cause is what _led_ to those last steps.  In this case, 
the root cause is that a read request with s->lba == -1 is mistaken for 
a non-read.  Read requests are able to reset s->io_buffer_index and 
start with the index pointing just after the end of the sector buffer; 
non-read requests instead visit the buffer just once and start with 
s->io_buffer_index == 0.

In turn, the fix is to validate:

1) that s->lba is in range when issuing a read request

2) that the size of the device is sane (e.g. the number of blocks is a 
positive 32-bit integer).

Paolo

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

[P J P]

+-- On Fri, 11 Dec 2020, Paolo Bonzini wrote --+
| This is not the root cause.  These are the last steps before bad things 
| happen; the root cause is what _led_ to those last steps.  In this case, the 
| root cause is that a read request with s->lba == -1 is mistaken for a 
| non-read.  Read requests are able to reset s->io_buffer_index and start with 
| the index pointing just after the end of the sector buffer; non-read 
| requests instead visit the buffer just once and start with 
| s->io_buffer_index == 0.
| 
| In turn, the fix is to validate:
| 
| 1) that s->lba is in range when issuing a read request
| 
| 2) that the size of the device is sane (e.g. the number of blocks is a
| positive 32-bit integer).

  Yes, working on a revised patch...

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D