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

[Stefan Priebe]

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


> 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