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
