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

[John Snow <jsnow@redhat.com>]

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 <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?
> 
> 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