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 <kw...@redhat.com>
> ---
> 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;
> }
>
>
