From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45956) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dwZ2N-0006js-QX for qemu-devel@nongnu.org; Mon, 25 Sep 2017 15:27:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dwZ2M-0008Er-G8 for qemu-devel@nongnu.org; Mon, 25 Sep 2017 15:27:27 -0400 Received: from mail-wm0-x235.google.com ([2a00:1450:400c:c09::235]:43474) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1dwZ2M-0008Ef-9h for qemu-devel@nongnu.org; Mon, 25 Sep 2017 15:27:26 -0400 Received: by mail-wm0-x235.google.com with SMTP id m72so12357498wmc.0 for ; Mon, 25 Sep 2017 12:27:26 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20170920061905.rbv3g2ubd4c5zvn4@pengutronix.de> References: <150555367996.36.15771330325496067998@69b6ddf88678> <20170916103523.1482-1-m.olbrich@pengutronix.de> <20170919082358.43upqf3lawg2aqtg@pengutronix.de> <20170920061905.rbv3g2ubd4c5zvn4@pengutronix.de> From: Peter Maydell Date: Mon, 25 Sep 2017 20:27:04 +0100 Message-ID: Content-Type: text/plain; charset="UTF-8" Subject: Re: [Qemu-devel] [PATCH v2] hw/sd: fix out-of-bounds check for multi block reads List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Olbrich Cc: Alistair Francis , "qemu-devel@nongnu.org Developers" On 20 September 2017 at 07:19, Michael Olbrich wrote: > On Tue, Sep 19, 2017 at 05:09:51PM -0700, Alistair Francis wrote: >> On Tue, Sep 19, 2017 at 1:23 AM, Michael Olbrich >> wrote: >> > On Mon, Sep 18, 2017 at 02:28:26PM -0700, Alistair Francis wrote: >> >> On Sat, Sep 16, 2017 at 3:35 AM, Michael Olbrich >> >> wrote: >> >> > hw/sd/sd.c | 12 ++++++------ >> >> > 1 file changed, 6 insertions(+), 6 deletions(-) >> >> > >> >> > diff --git a/hw/sd/sd.c b/hw/sd/sd.c >> >> > index ba47bff4db80..35347a5bbcde 100644 >> >> > --- a/hw/sd/sd.c >> >> > +++ b/hw/sd/sd.c >> >> > @@ -1797,8 +1797,13 @@ uint8_t sd_read_data(SDState *sd) >> >> > break; >> >> > >> >> > case 18: /* CMD18: READ_MULTIPLE_BLOCK */ >> >> > - if (sd->data_offset == 0) >> >> > + if (sd->data_offset == 0) { >> >> > + if (sd->data_start + io_len > sd->size) { >> >> > + sd->card_status |= ADDRESS_ERROR; >> >> > + return 0x00; >> >> > + } >> >> >> >> Why move it inside the if (sd->data_offset == 0) and not just below >> >> the ret = sd->data[sd->data_offset ++] ? >> >> >> >> > BLK_READ_BLOCK(sd->data_start, io_len); >> > >> > Mostly because of the line above. This copies the full block from the >> > backend storage to sd->data, so we need to make sure that the data is >> > actually available to fill sd->data, not if it's ok to access a certain >> > byte within sd->data. >> >> Doesn't this mean that the check is only done for the first block >> then? When data_offset is 0. > > No, data_offset is reset at the end of the block. > [...] Alistair, were you planning to provide a reviewed-by: for this patch (or did you have more review comments on it)? thanks -- PMM