From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38955) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGMik-0002ai-9q for qemu-devel@nongnu.org; Tue, 05 Jan 2016 03:12:02 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aGMih-0006Xw-3B for qemu-devel@nongnu.org; Tue, 05 Jan 2016 03:11:58 -0500 References: <1451928613-29476-1-git-send-email-mark.cave-ayland@ilande.co.uk> <568AC4E3.8050101@ilande.co.uk> <568AD7B8.8050205@redhat.com> <568ADBFD.8020301@ilande.co.uk> <568ADE08.8070806@redhat.com> From: Mark Cave-Ayland Message-ID: <568B7AB0.307@ilande.co.uk> Date: Tue, 5 Jan 2016 08:11:28 +0000 MIME-Version: 1.0 In-Reply-To: <568ADE08.8070806@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] macio: fix overflow in lba to offset conversion for ATAPI devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow , P J P Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org, agraf@suse.de 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.