From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55491) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YvY04-0000Cy-OR for qemu-devel@nongnu.org; Thu, 21 May 2015 17:27:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YvY03-0004cl-QB for qemu-devel@nongnu.org; Thu, 21 May 2015 17:27:32 -0400 Message-ID: <555E4DBB.9080802@redhat.com> Date: Thu, 21 May 2015 17:27:23 -0400 From: John Snow MIME-Version: 1.0 References: <1432214378-31891-1-git-send-email-kwolf@redhat.com> <1432214378-31891-8-git-send-email-kwolf@redhat.com> In-Reply-To: <1432214378-31891-8-git-send-email-kwolf@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 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: peter.maydell@linaro.org, qemu-devel@nongnu.org On 05/21/2015 09:19 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. > > 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 > --- > hw/block/fdc.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > Reviewed-by: John Snow