From patchwork Thu May 21 13:19:37 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kevin Wolf X-Patchwork-Id: 475009 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 6FAB3140081 for ; Thu, 21 May 2015 23:21:10 +1000 (AEST) Received: from localhost ([::1]:57650 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YvQPM-0003Vy-8Z for incoming@patchwork.ozlabs.org; Thu, 21 May 2015 09:21:08 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38614) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YvQOX-0002D3-Rb for qemu-devel@nongnu.org; Thu, 21 May 2015 09:20:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YvQOT-0007iH-Nt for qemu-devel@nongnu.org; Thu, 21 May 2015 09:20:17 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57626) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YvQON-0007bt-CR; Thu, 21 May 2015 09:20:07 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 123CAB5E6D; Thu, 21 May 2015 13:20:07 +0000 (UTC) Received: from noname.redhat.com (ovpn-116-72.ams2.redhat.com [10.36.116.72]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t4LDJpga023142; Thu, 21 May 2015 09:20:05 -0400 From: Kevin Wolf To: qemu-block@nongnu.org Date: Thu, 21 May 2015 15:19:37 +0200 Message-Id: <1432214378-31891-8-git-send-email-kwolf@redhat.com> In-Reply-To: <1432214378-31891-1-git-send-email-kwolf@redhat.com> References: <1432214378-31891-1-git-send-email-kwolf@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: kwolf@redhat.com, peter.maydell@linaro.org, jsnow@redhat.com, qemu-devel@nongnu.org Subject: [Qemu-devel] [PATCH v2 7/8] fdc: Fix MSR.RQM flag X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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. It also clear RQM after receiving all bytes even if the phase transition immediately sets it again. While it's technically not necessary at the moment because the state between clearing and setting RQM is not observable by the guest, this is more explicit and matches how real hardware works. It will actually become necessary in qemu once asynchronous code paths are introduced. 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 Reviewed-by: John Snow --- 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 3c64194..6e79459 100644 --- a/hw/block/fdc.c +++ b/hw/block/fdc.c @@ -1223,7 +1223,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. @@ -1438,7 +1440,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 */ @@ -1618,6 +1620,7 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl) } if (++fdctrl->data_pos == fdctrl->data_len) { + fdctrl->msr &= ~FD_MSR_RQM; fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00); } break; @@ -1625,6 +1628,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; fdctrl_to_command_phase(fdctrl); fdctrl_reset_irq(fdctrl); } @@ -2094,6 +2098,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: /* For DMA requests, RQM should be cleared during execution phase, so @@ -2132,6 +2140,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; }