Re: [Qemu-devel] [PATCH 7/8] fdc: Fix MSR.RQM flag

[John Snow <jsnow@redhat.com>]

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 <kwolf@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;
>          }
>  
>