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

[Stefan Priebe <s.priebe@profihost.ag>]

Am 13.05.2015 um 21:04 schrieb 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.

i'm my host ;-) I was just interested if for whatever reason live 
migration is not enough and was wondering why someone wants to reboot a 
vm for something like this.

Thanks!

Stefan

>
>>>
>>> Am 13.05.2015 um 16:33 schrieb John Snow:
>>>> From: Petr Matousek <pmatouse@redhat.com>
>>>>
>>>> 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 <pmatouse@redhat.com>
>>>> Reviewed-by: John Snow <jsnow@redhat.com>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>>    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
>>>
>>>
>>
>