From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34606) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yuz96-0000Jr-0y for qemu-devel@nongnu.org; Wed, 20 May 2015 04:14:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yuz95-00076N-1S for qemu-devel@nongnu.org; Wed, 20 May 2015 04:14:31 -0400 Date: Wed, 20 May 2015 10:14:21 +0200 From: Kevin Wolf Message-ID: <20150520081421.GB4917@noname.redhat.com> References: <1432049762-2184-1-git-send-email-kwolf@redhat.com> <1432049762-2184-8-git-send-email-kwolf@redhat.com> <555B9FD9.9000904@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <555B9FD9.9000904@redhat.com> Subject: Re: [Qemu-devel] [PATCH 7/8] fdc: Fix MSR.RQM flag List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org Am 19.05.2015 um 22:40 hat John Snow geschrieben: > > > On 05/19/2015 11:36 AM, Kevin Wolf wrote: > > The RQM bit in MSR should be set whenever the guest is supposed to > > access the FIFO, and it should be cleared in all other cases. This is > > important so the guest can't continue writing/reading the FIFO beyond > > the length that it's suppossed to access (see CVE-2015-3456). > > > > Commit e9077462 fixed the CVE by adding code that avoids the buffer > > overflow; however it doesn't correct the wrong behaviour of the floppy > > controller which should already have cleared RQM. > > > > Currently, RQM stays set all the time and during all phases while a > > command is being processed. This is error-prone because the command has > > to explicitly clear the flag if it doesn't need data (and indeed, the > > two buggy commands that are the culprits for the CVE just forgot to do > > that). > > > > This patch clears RQM immediately as soon as all bytes that are expected > > have been received. If the the FIFO is used in the next phase, the flag > > has to be set explicitly there. > > > > This alone should have been enough to fix the CVE, but now we have two > > lines of defense - even better. > > > > Signed-off-by: Kevin Wolf > > --- > > hw/block/fdc.c | 13 ++++++++++++- > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/hw/block/fdc.c b/hw/block/fdc.c > > index 8d322e0..c6a046e 100644 > > --- a/hw/block/fdc.c > > +++ b/hw/block/fdc.c > > @@ -1165,7 +1165,9 @@ static void fdctrl_to_command_phase(FDCtrl *fdctrl) > > fdctrl->phase = FD_PHASE_COMMAND; > > fdctrl->data_dir = FD_DIR_WRITE; > > fdctrl->data_pos = 0; > > + fdctrl->data_len = 1; /* Accept command byte, adjust for params later */ > > fdctrl->msr &= ~(FD_MSR_CMDBUSY | FD_MSR_DIO); > > + fdctrl->msr |= FD_MSR_RQM; > > } > > > > /* Update the state to allow the guest to read out the command status. > > @@ -1380,7 +1382,7 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction) > > } > > } > > FLOPPY_DPRINTF("start non-DMA transfer\n"); > > - fdctrl->msr |= FD_MSR_NONDMA; > > + fdctrl->msr |= FD_MSR_NONDMA | FD_MSR_RQM; > > if (direction != FD_DIR_WRITE) > > fdctrl->msr |= FD_MSR_DIO; > > /* IO based transfer: calculate len */ > > @@ -1560,6 +1562,7 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl) > > } > > > > if (++fdctrl->data_pos == fdctrl->data_len) { > > + fdctrl->msr &= ~FD_MSR_RQM; > > Doesn't stop_transfer set this flag back right away? It does, by switching to the result phase. I think it's clearer to disable the bit anywhere where the FIFO has received as many bytes as it's supposed to, even if the next phase is started immediately and reenables it. In real hardware, sending a byte causes the FDC to disable RQM, then process the byte (which means completing command execution for this code path), then reenable RQM if needed. Currently our code is completely synchronous, so we could ignore this detail because the state between clearing and setting RQM isn't observable by the guest. If we ever introduce something asynchronous in the path, we will need this though - and modelling real hardware more precisely has never hurt anyway. Kevin