Re: [Qemu-stable] [Qemu-devel] [PATCH] fdc: force the fifo access to be in bounds of the allocated buffer

[John Snow]

On 05/13/2015 02:59 PM, Stefan Priebe wrote:
> 
> Am 13.05.2015 um 20:51 schrieb Stefan Weil:
>> Hi,
>>
>> I just noticed this patch because my provider told me that my KVM based
>> server
>> needs a reboot because of a CVE (see this German news:
>> http://www.heise.de/newsticker/meldung/Venom-Schwachstelle-Aus-Hypervisor-ausbrechen-und-VMs-ausspionieren-2649614.html)
>>
> 
> Isn't a live migration to a fixed version enough instead of a reboot?
> 
> Stefan
> 
> 

If your management API or host or whatever lets you migrate back to the
same host, or has another host they can migrate you to, yes.

>>
>> Am 13.05.2015 um 16:33 schrieb John Snow:
>>> From: Petr Matousek <address@hidden>
>>>
>>> During processing of certain commands such as FD_CMD_READ_ID and
>>> FD_CMD_DRIVE_SPECIFICATION_COMMAND the fifo memory access could
>>> get out of bounds leading to memory corruption with values coming
>>> from the guest.
>>>
>>> Fix this by making sure that the index is always bounded by the
>>> allocated memory.
>>>
>>> This is CVE-2015-3456.
>>>
>>> Signed-off-by: Petr Matousek <address@hidden>
>>> Reviewed-by: John Snow <address@hidden>
>>> Signed-off-by: John Snow <address@hidden>
>>> ---
>>>   hw/block/fdc.c | 17 +++++++++++------
>>>   1 file changed, 11 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>>> index f72a392..d8a8edd 100644
>>> --- a/hw/block/fdc.c
>>> +++ b/hw/block/fdc.c
>>> @@ -1497,7 +1497,7 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>>>   {
>>>       FDrive *cur_drv;
>>>       uint32_t retval = 0;
>>> -    int pos;
>>> +    uint32_t pos;
>>>       cur_drv = get_cur_drv(fdctrl);
>>>       fdctrl->dsr &= ~FD_DSR_PWRDOWN;
>>> @@ -1506,8 +1506,8 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>>>           return 0;
>>>       }
>>>       pos = fdctrl->data_pos;
>>> +    pos %= FD_SECTOR_LEN;
>>
>> I'd combine both statements and perhaps use fdctrl->fifo_size (even if
>> the resulting code will be slightly larger):
>>
>> pos = fdctrl->data_pos % fdctrl->fifo_size;
>>
>>
>>>       if (fdctrl->msr & FD_MSR_NONDMA) {
>>> -        pos %= FD_SECTOR_LEN;
>>>           if (pos == 0) {
>>>               if (fdctrl->data_pos != 0)
>>>                   if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {
>>> @@ -1852,10 +1852,13 @@ static void fdctrl_handle_option(FDCtrl
>>> *fdctrl, int direction)
>>>   static void fdctrl_handle_drive_specification_command(FDCtrl
>>> *fdctrl, int direction)
>>>   {
>>>       FDrive *cur_drv = get_cur_drv(fdctrl);
>>> +    uint32_t pos;
>>> -    if (fdctrl->fifo[fdctrl->data_pos - 1] & 0x80) {
>>> +    pos = fdctrl->data_pos - 1;
>>> +    pos %= FD_SECTOR_LEN;
>>
>> Shorter (and more clear):
>>
>> uint32_t pos = (fdctrl->data_pos - 1) % fdctrl->fifo_size;
>>
>>> +    if (fdctrl->fifo[pos] & 0x80) {
>>>           /* Command parameters done */
>>> -        if (fdctrl->fifo[fdctrl->data_pos - 1] & 0x40) {
>>> +        if (fdctrl->fifo[pos] & 0x40) {
>>>               fdctrl->fifo[0] = fdctrl->fifo[1];
>>>               fdctrl->fifo[2] = 0;
>>>               fdctrl->fifo[3] = 0;
>>> @@ -1955,7 +1958,7 @@ static uint8_t command_to_handler[256];
>>>   static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
>>>   {
>>>       FDrive *cur_drv;
>>> -    int pos;
>>> +    uint32_t pos;
>>>       /* Reset mode */
>>>       if (!(fdctrl->dor & FD_DOR_nRESET)) {
>>> @@ -2004,7 +2007,9 @@ static void fdctrl_write_data(FDCtrl *fdctrl,
>>> uint32_t value)
>>>       }
>>>       FLOPPY_DPRINTF("%s: %02x\n", __func__, value);
>>> -    fdctrl->fifo[fdctrl->data_pos++] = value;
>>> +    pos = fdctrl->data_pos++;
>>> +    pos %= FD_SECTOR_LEN;
>>> +    fdctrl->fifo[pos] = value;
>>>       if (fdctrl->data_pos == fdctrl->data_len) {
>>>           /* We now have all parameters
>>>            * and will be able to treat the command
>>
>> Not strictly related to this patch: The code which sets fifo_size could
>> also be improved.
>>
>>      fdctrl->fifo = qemu_memalign(512, FD_SECTOR_LEN);
>>      fdctrl->fifo_size = 512;
>>
>> The 2nd line should be
>>
>>      fdctrl->fifo_size = FD_SECTOR_LEN;
>>
>>
>> As far as I see the original code can read or write illegal memory
>> locations in the address space of the QEMU process. It cannot (as it was
>> claimed) modify the code of the VM host because those memory is usually
>> write protected - at least if QEMU is running without KVM. If the code
>> which is generated for KVM is writable from anywhere in QEMU, we should
>> perhaps consider changing that.
>>
>> Regards
>> Stefan
>>
>>
> 

-- 
—js