From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47811) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YuoJz-00068l-4k for qemu-devel@nongnu.org; Tue, 19 May 2015 16:41:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YuoJv-0001hN-2u for qemu-devel@nongnu.org; Tue, 19 May 2015 16:41:03 -0400 Message-ID: <555B9FD9.9000904@redhat.com> Date: Tue, 19 May 2015 16:40:57 -0400 From: John Snow MIME-Version: 1.0 References: <1432049762-2184-1-git-send-email-kwolf@redhat.com> <1432049762-2184-8-git-send-email-kwolf@redhat.com> In-Reply-To: <1432049762-2184-8-git-send-email-kwolf@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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: Kevin Wolf , qemu-block@nongnu.org Cc: qemu-devel@nongnu.org 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? > fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00); > } > break; > @@ -1567,6 +1570,7 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl) > case FD_PHASE_RESULT: > assert(!(fdctrl->msr & FD_MSR_NONDMA)); > if (++fdctrl->data_pos == fdctrl->data_len) { > + fdctrl->msr &= ~FD_MSR_RQM; Same here with to_command_phase. > fdctrl_to_command_phase(fdctrl); > fdctrl_reset_irq(fdctrl); > } > @@ -2036,6 +2040,10 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value) > pos %= FD_SECTOR_LEN; > fdctrl->fifo[pos] = value; > > + if (fdctrl->data_pos == fdctrl->data_len) { > + fdctrl->msr &= ~FD_MSR_RQM; > + } > + > switch (fdctrl->phase) { > case FD_PHASE_EXECUTION: > assert(fdctrl->msr & FD_MSR_NONDMA); > @@ -2071,6 +2079,9 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value) > * as many parameters as this command requires. */ > cmd = get_command(value); > fdctrl->data_len = cmd->parameters + 1; > + if (cmd->parameters) { > + fdctrl->msr |= FD_MSR_RQM; > + } > fdctrl->msr |= FD_MSR_CMDBUSY; > } > >