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

[John Snow]

On 05/13/2015 02:51 PM, Stefan Weil wrote:
> 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)
> 
> 
> 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):
> 

Sure. Send me a patch and I'll ACK it.

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

Good here, too.

>> +    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;
> 

Agreed, and it came up during the review for this, but we kept it out to
keep this a one patch targeted fix.

Also arising from the review: I want to move tmpbuf off of the stack,
though that particular buffer appears to be properly bounded at all times.

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

I don't think we are aware of any particular weaknesses, the security
report only said the "possibility" of arbitrary code execution due to
the buffer overflow. I haven't heard any more detailed explanation than
this.

> Regards
> Stefan
> 

Thanks,
--js