From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45955) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yv2eB-0000aq-Ix for qemu-devel@nongnu.org; Wed, 20 May 2015 07:58:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yv2eA-0000Cv-FG for qemu-devel@nongnu.org; Wed, 20 May 2015 07:58:51 -0400 Message-ID: <555C76F4.6000908@redhat.com> Date: Wed, 20 May 2015 07:58:44 -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> <555B9FD9.9000904@redhat.com> <20150520081421.GB4917@noname.redhat.com> In-Reply-To: <20150520081421.GB4917@noname.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 Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org On 05/20/2015 04:14 AM, Kevin Wolf wrote: > 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 > OK, just amend the commit message to explain that clearing the bits here is to accommodate a possible asynchronous refactor, or to be more explicit, or etc etc etc. --js