From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45031) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGBrT-0000qu-Tm for qemu-devel@nongnu.org; Mon, 04 Jan 2016 15:36:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aGBrP-00039d-ON for qemu-devel@nongnu.org; Mon, 04 Jan 2016 15:36:15 -0500 References: <1451928613-29476-1-git-send-email-mark.cave-ayland@ilande.co.uk> <568AC4E3.8050101@ilande.co.uk> From: John Snow Message-ID: <568AD7B8.8050205@redhat.com> Date: Mon, 4 Jan 2016 15:36:08 -0500 MIME-Version: 1.0 In-Reply-To: <568AC4E3.8050101@ilande.co.uk> 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: Mark Cave-Ayland , P J P Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org, agraf@suse.de 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