[Qemu-devel] [PATCH] macio: fix overflow in lba to offset conversion for ATAPI devices

[Qemu-devel] [PATCH] macio: fix overflow in lba to offset conversion for ATAPI devices

[Mark Cave-Ayland]

As the IDEState lba field is an int32_t, make sure we cast to int64_t before
shifting to calculate the offset. Otherwise we end up with an overflow when
trying to access sectors beyond 2GB as can occur when using DVD images.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ide/macio.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 3ee962f..a78b6e0 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -280,7 +280,7 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
     }
 
     /* Calculate current offset */
-    offset = (int64_t)(s->lba << 11) + s->io_buffer_index;
+    offset = ((int64_t)(s->lba) << 11) + s->io_buffer_index;
 
     pmac_dma_read(s->blk, offset, io->len, pmac_ide_atapi_transfer_cb, io);
     return;
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH] macio: fix overflow in lba to offset conversion for ATAPI devices

[P J P]

+-- On Mon, 4 Jan 2016, Mark Cave-Ayland wrote --+
|      /* Calculate current offset */
| -    offset = (int64_t)(s->lba << 11) + s->io_buffer_index;
| +    offset = ((int64_t)(s->lba) << 11) + s->io_buffer_index;

Maybe ((int64_t)s->lba << 11) ? No parenthesis around s->lba.

--
 - P J P
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH] macio: fix overflow in lba to offset conversion for ATAPI devices

[Mark Cave-Ayland]

On 04/01/16 19:04, P J P wrote:

> +-- On Mon, 4 Jan 2016, Mark Cave-Ayland wrote --+
> |      /* Calculate current offset */
> | -    offset = (int64_t)(s->lba << 11) + s->io_buffer_index;
> | +    offset = ((int64_t)(s->lba) << 11) + s->io_buffer_index;
> 
> Maybe ((int64_t)s->lba << 11) ? No parenthesis around s->lba.

Yes that works here too (perhaps I was just being over-cautious).
Alex/John, please let me know if you want me to resubmit.


ATB,

Mark.

Re: [Qemu-devel] [PATCH] macio: fix overflow in lba to offset conversion for ATAPI devices

[John Snow]

On 01/04/2016 02:15 PM, Mark Cave-Ayland wrote:
> On 04/01/16 19:04, P J P wrote:
> 
>> +-- On Mon, 4 Jan 2016, Mark Cave-Ayland wrote --+
>> |      /* Calculate current offset */
>> | -    offset = (int64_t)(s->lba << 11) + s->io_buffer_index;
>> | +    offset = ((int64_t)(s->lba) << 11) + s->io_buffer_index;
>>
>> Maybe ((int64_t)s->lba << 11) ? No parenthesis around s->lba.
> 
> Yes that works here too (perhaps I was just being over-cautious).
> Alex/John, please let me know if you want me to resubmit.
> 

PJP's version should work just fine. I won't ask you to resubmit, though...

> 
> ATB,
> 
> Mark.
> 

...But, well, while we're here, I have a question for you:

So s->lba is an int that we left shift by 11 for a max of (2^43 - 2^11)
then we add it against s->io_buffer_index, a uint64_t, so this statement
could still in theory overflow.

Except not really, since io_buffer_index is bounded (in general) by
io_buffer_total_len, which is usually (IDE_DMA_BUF_SECTORS*512 + 4) ->
~132K.

I don't think there's any rigorous bounds-checking of io_buffer_index,
just ad-hoc checking when we're good enough to remember to do it. And we
don't seem to do it anywhere in macio. Is it worth peppering in an
assert somewhere that io_buffer_index is reasonably small?

--js

Re: [Qemu-devel] [PATCH] macio: fix overflow in lba to offset conversion for ATAPI devices

[Mark Cave-Ayland]

On 04/01/16 20:36, John Snow wrote:

> On 01/04/2016 02:15 PM, Mark Cave-Ayland wrote:
>> On 04/01/16 19:04, P J P wrote:
>>
>>> +-- On Mon, 4 Jan 2016, Mark Cave-Ayland wrote --+
>>> |      /* Calculate current offset */
>>> | -    offset = (int64_t)(s->lba << 11) + s->io_buffer_index;
>>> | +    offset = ((int64_t)(s->lba) << 11) + s->io_buffer_index;
>>>
>>> Maybe ((int64_t)s->lba << 11) ? No parenthesis around s->lba.
>>
>> Yes that works here too (perhaps I was just being over-cautious).
>> Alex/John, please let me know if you want me to resubmit.
>>
> 
> PJP's version should work just fine. I won't ask you to resubmit, though...

Great, thanks :)

> ...But, well, while we're here, I have a question for you:
> 
> So s->lba is an int that we left shift by 11 for a max of (2^43 - 2^11)
> then we add it against s->io_buffer_index, a uint64_t, so this statement
> could still in theory overflow.
> 
> Except not really, since io_buffer_index is bounded (in general) by
> io_buffer_total_len, which is usually (IDE_DMA_BUF_SECTORS*512 + 4) ->
> ~132K.
> 
> I don't think there's any rigorous bounds-checking of io_buffer_index,
> just ad-hoc checking when we're good enough to remember to do it. And we
> don't seem to do it anywhere in macio. Is it worth peppering in an
> assert somewhere that io_buffer_index is reasonably small?

The DBDMA engine is limited to 16-bit transfers so the maximum transfer
size is 64K, and s->io_buffer_index is used to hold the current position
within this transfer so unless we get some very large disks I think we
should be okay here?


ATB,

Mark.

Re: [Qemu-devel] [PATCH] macio: fix overflow in lba to offset conversion for ATAPI devices

[John Snow]

On 01/04/2016 03:54 PM, Mark Cave-Ayland wrote:
> On 04/01/16 20:36, John Snow wrote:
> 
>> On 01/04/2016 02:15 PM, Mark Cave-Ayland wrote:
>>> On 04/01/16 19:04, P J P wrote:
>>>
>>>> +-- On Mon, 4 Jan 2016, Mark Cave-Ayland wrote --+
>>>> |      /* Calculate current offset */
>>>> | -    offset = (int64_t)(s->lba << 11) + s->io_buffer_index;
>>>> | +    offset = ((int64_t)(s->lba) << 11) + s->io_buffer_index;
>>>>
>>>> Maybe ((int64_t)s->lba << 11) ? No parenthesis around s->lba.
>>>
>>> Yes that works here too (perhaps I was just being over-cautious).
>>> Alex/John, please let me know if you want me to resubmit.
>>>
>>
>> PJP's version should work just fine. I won't ask you to resubmit, though...
> 
> Great, thanks :)
> 
>> ...But, well, while we're here, I have a question for you:
>>
>> So s->lba is an int that we left shift by 11 for a max of (2^43 - 2^11)
>> then we add it against s->io_buffer_index, a uint64_t, so this statement
>> could still in theory overflow.
>>
>> Except not really, since io_buffer_index is bounded (in general) by
>> io_buffer_total_len, which is usually (IDE_DMA_BUF_SECTORS*512 + 4) ->
>> ~132K.
>>
>> I don't think there's any rigorous bounds-checking of io_buffer_index,
>> just ad-hoc checking when we're good enough to remember to do it. And we
>> don't seem to do it anywhere in macio. Is it worth peppering in an
>> assert somewhere that io_buffer_index is reasonably small?
> 
> The DBDMA engine is limited to 16-bit transfers so the maximum transfer
> size is 64K, and s->io_buffer_index is used to hold the current position
> within this transfer so unless we get some very large disks I think we
> should be okay here?
> 

For all non-malicious uses of the code, yes.

If I want to apply some rigorous checking to this bound I should just
add a function to manipulate it centrally in core.c, I think.

> 
> ATB,
> 
> Mark.
> 


I'll pull this and edit it to PJP's suggestion.

--js

Re: [Qemu-devel] [PATCH] macio: fix overflow in lba to offset conversion for ATAPI devices

[Mark Cave-Ayland]

On 04/01/16 21:03, John Snow wrote:

> On 01/04/2016 03:54 PM, Mark Cave-Ayland wrote:
>> On 04/01/16 20:36, John Snow wrote:
>>
>>> On 01/04/2016 02:15 PM, Mark Cave-Ayland wrote:
>>>> On 04/01/16 19:04, P J P wrote:
>>>>
>>>>> +-- On Mon, 4 Jan 2016, Mark Cave-Ayland wrote --+
>>>>> |      /* Calculate current offset */
>>>>> | -    offset = (int64_t)(s->lba << 11) + s->io_buffer_index;
>>>>> | +    offset = ((int64_t)(s->lba) << 11) + s->io_buffer_index;
>>>>>
>>>>> Maybe ((int64_t)s->lba << 11) ? No parenthesis around s->lba.
>>>>
>>>> Yes that works here too (perhaps I was just being over-cautious).
>>>> Alex/John, please let me know if you want me to resubmit.
>>>>
>>>
>>> PJP's version should work just fine. I won't ask you to resubmit, though...
>>
>> Great, thanks :)
>>
>>> ...But, well, while we're here, I have a question for you:
>>>
>>> So s->lba is an int that we left shift by 11 for a max of (2^43 - 2^11)
>>> then we add it against s->io_buffer_index, a uint64_t, so this statement
>>> could still in theory overflow.
>>>
>>> Except not really, since io_buffer_index is bounded (in general) by
>>> io_buffer_total_len, which is usually (IDE_DMA_BUF_SECTORS*512 + 4) ->
>>> ~132K.
>>>
>>> I don't think there's any rigorous bounds-checking of io_buffer_index,
>>> just ad-hoc checking when we're good enough to remember to do it. And we
>>> don't seem to do it anywhere in macio. Is it worth peppering in an
>>> assert somewhere that io_buffer_index is reasonably small?
>>
>> The DBDMA engine is limited to 16-bit transfers so the maximum transfer
>> size is 64K, and s->io_buffer_index is used to hold the current position
>> within this transfer so unless we get some very large disks I think we
>> should be okay here?
>>
> 
> For all non-malicious uses of the code, yes.
> 
> If I want to apply some rigorous checking to this bound I should just
> add a function to manipulate it centrally in core.c, I think.

That sounds good - any solution that avoids having to maintain changes
across IDE core and macio separately is always good!

> I'll pull this and edit it to PJP's suggestion.

Brilliant - thanks a lot!


ATB,

Mark.

Re: [Qemu-devel] [PATCH] macio: fix overflow in lba to offset conversion for ATAPI devices

[John Snow]

On 01/04/2016 12:30 PM, Mark Cave-Ayland wrote:
> As the IDEState lba field is an int32_t, make sure we cast to int64_t before
> shifting to calculate the offset. Otherwise we end up with an overflow when
> trying to access sectors beyond 2GB as can occur when using DVD images.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/ide/macio.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> index 3ee962f..a78b6e0 100644
> --- a/hw/ide/macio.c
> +++ b/hw/ide/macio.c
> @@ -280,7 +280,7 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
>      }
>  
>      /* Calculate current offset */
> -    offset = (int64_t)(s->lba << 11) + s->io_buffer_index;
> +    offset = ((int64_t)(s->lba) << 11) + s->io_buffer_index;
>  
>      pmac_dma_read(s->blk, offset, io->len, pmac_ide_atapi_transfer_cb, io);
>      return;
> 

Thanks, applied to my IDE tree:

https://github.com/jnsnow/qemu/commits/ide
https://github.com/jnsnow/qemu.git

--js