| Message ID | 1451928613-29476-1-git-send-email-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
+-- 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
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.
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
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.
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
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.
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
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;
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(-)