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

[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)

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