| Message ID | 1463475294-14119-1-git-send-email-kraxel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, thanks for the patch. On 05/17/2016 10:54 AM, Gerd Hoffmann wrote: > Commit "fd3c136 vga: make sure vga register setup for vbe stays intact > (CVE-2016-3712)." causes a regression. The win7 installer is unhappy > because it can't freely modify vga registers any more while in vbe mode. > > This patch introduces a new sr_vbe register set. The vbe_update_vgaregs > will fill sr_vbe[] instead of sr[]. Normal vga register reads and > writes go to sr[]. Any sr register read access happens through a new > sr() helper function which will read from sr_vbe[] with vbe active and > from sr[] otherwise. > > This way we can allow guests update sr[] registers as they want, without > allowing them disrupt vbe video modes that way. Just documenting my test with the patch here: This fixes the issue with QEMU 2.5.1.1 but only if I'm using SeaBIOS. OVMF leads to a almost similar result as without the patch, after "windows is loading files" the "Starting Windows" appears short then hangs then the screen remains blank, so the blank screen is new with OVMF. The same is with QEMU 2.6.0, with SeaBIOS it is working with this patch but with OVMF still not. best regards, Thomas > > Reported-by: Thomas Lamprecht <thomas@lamprecht.org> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > hw/display/vga.c | 50 ++++++++++++++++++++++++++++---------------------- > hw/display/vga_int.h | 1 + > 2 files changed, 29 insertions(+), 22 deletions(-) > > diff --git a/hw/display/vga.c b/hw/display/vga.c > index 4a55ec6..9ebc54f 100644 > --- a/hw/display/vga.c > +++ b/hw/display/vga.c > @@ -149,6 +149,11 @@ static inline bool vbe_enabled(VGACommonState *s) > return s->vbe_regs[VBE_DISPI_INDEX_ENABLE] & VBE_DISPI_ENABLED; > } > > +static inline uint8_t sr(VGACommonState *s, int idx) > +{ > + return vbe_enabled(s) ? s->sr_vbe[idx] : s->sr[idx]; > +} > + > static void vga_update_memory_access(VGACommonState *s) > { > hwaddr base, offset, size; > @@ -163,8 +168,8 @@ static void vga_update_memory_access(VGACommonState *s) > s->has_chain4_alias = false; > s->plane_updated = 0xf; > } > - if ((s->sr[VGA_SEQ_PLANE_WRITE] & VGA_SR02_ALL_PLANES) == > - VGA_SR02_ALL_PLANES && s->sr[VGA_SEQ_MEMORY_MODE] & VGA_SR04_CHN_4M) { > + if ((sr(s, VGA_SEQ_PLANE_WRITE) & VGA_SR02_ALL_PLANES) == > + VGA_SR02_ALL_PLANES && sr(s, VGA_SEQ_MEMORY_MODE) & VGA_SR04_CHN_4M) { > offset = 0; > switch ((s->gr[VGA_GFX_MISC] >> 2) & 3) { > case 0: > @@ -234,7 +239,7 @@ static void vga_precise_update_retrace_info(VGACommonState *s) > ((s->cr[VGA_CRTC_OVERFLOW] >> 6) & 2)) << 8); > vretr_end_line = s->cr[VGA_CRTC_V_SYNC_END] & 0xf; > > - clocking_mode = (s->sr[VGA_SEQ_CLOCK_MODE] >> 3) & 1; > + clocking_mode = (sr(s, VGA_SEQ_CLOCK_MODE) >> 3) & 1; > clock_sel = (s->msr >> 2) & 3; > dots = (s->msr & 1) ? 8 : 9; > > @@ -486,7 +491,6 @@ void vga_ioport_write(void *opaque, uint32_t addr, uint32_t val) > printf("vga: write SR%x = 0x%02x\n", s->sr_index, val); > #endif > s->sr[s->sr_index] = val & sr_mask[s->sr_index]; > - vbe_update_vgaregs(s); > if (s->sr_index == VGA_SEQ_CLOCK_MODE) { > s->update_retrace_info(s); > } > @@ -680,13 +684,13 @@ static void vbe_update_vgaregs(VGACommonState *s) > > if (s->vbe_regs[VBE_DISPI_INDEX_BPP] == 4) { > shift_control = 0; > - s->sr[VGA_SEQ_CLOCK_MODE] &= ~8; /* no double line */ > + s->sr_vbe[VGA_SEQ_CLOCK_MODE] &= ~8; /* no double line */ > } else { > shift_control = 2; > /* set chain 4 mode */ > - s->sr[VGA_SEQ_MEMORY_MODE] |= VGA_SR04_CHN_4M; > + s->sr_vbe[VGA_SEQ_MEMORY_MODE] |= VGA_SR04_CHN_4M; > /* activate all planes */ > - s->sr[VGA_SEQ_PLANE_WRITE] |= VGA_SR02_ALL_PLANES; > + s->sr_vbe[VGA_SEQ_PLANE_WRITE] |= VGA_SR02_ALL_PLANES; > } > s->gr[VGA_GFX_MODE] = (s->gr[VGA_GFX_MODE] & ~0x60) | > (shift_control << 5); > @@ -836,7 +840,7 @@ uint32_t vga_mem_readb(VGACommonState *s, hwaddr addr) > break; > } > > - if (s->sr[VGA_SEQ_MEMORY_MODE] & VGA_SR04_CHN_4M) { > + if (sr(s, VGA_SEQ_MEMORY_MODE) & VGA_SR04_CHN_4M) { > /* chain 4 mode : simplest access */ > assert(addr < s->vram_size); > ret = s->vram_ptr[addr]; > @@ -904,11 +908,11 @@ void vga_mem_writeb(VGACommonState *s, hwaddr addr, uint32_t val) > break; > } > > - if (s->sr[VGA_SEQ_MEMORY_MODE] & VGA_SR04_CHN_4M) { > + if (sr(s, VGA_SEQ_MEMORY_MODE) & VGA_SR04_CHN_4M) { > /* chain 4 mode : simplest access */ > plane = addr & 3; > mask = (1 << plane); > - if (s->sr[VGA_SEQ_PLANE_WRITE] & mask) { > + if (sr(s, VGA_SEQ_PLANE_WRITE) & mask) { > assert(addr < s->vram_size); > s->vram_ptr[addr] = val; > #ifdef DEBUG_VGA_MEM > @@ -921,7 +925,7 @@ void vga_mem_writeb(VGACommonState *s, hwaddr addr, uint32_t val) > /* odd/even mode (aka text mode mapping) */ > plane = (s->gr[VGA_GFX_PLANE_READ] & 2) | (addr & 1); > mask = (1 << plane); > - if (s->sr[VGA_SEQ_PLANE_WRITE] & mask) { > + if (sr(s, VGA_SEQ_PLANE_WRITE) & mask) { > addr = ((addr & ~1) << 1) | plane; > if (addr >= s->vram_size) { > return; > @@ -996,7 +1000,7 @@ void vga_mem_writeb(VGACommonState *s, hwaddr addr, uint32_t val) > > do_write: > /* mask data according to sr[2] */ > - mask = s->sr[VGA_SEQ_PLANE_WRITE]; > + mask = sr(s, VGA_SEQ_PLANE_WRITE); > s->plane_updated |= mask; /* only used to detect font change */ > write_mask = mask16[mask]; > if (addr * sizeof(uint32_t) >= s->vram_size) { > @@ -1152,10 +1156,10 @@ static void vga_get_text_resolution(VGACommonState *s, int *pwidth, int *pheight > /* total width & height */ > cheight = (s->cr[VGA_CRTC_MAX_SCAN] & 0x1f) + 1; > cwidth = 8; > - if (!(s->sr[VGA_SEQ_CLOCK_MODE] & VGA_SR01_CHAR_CLK_8DOTS)) { > + if (!(sr(s, VGA_SEQ_CLOCK_MODE) & VGA_SR01_CHAR_CLK_8DOTS)) { > cwidth = 9; > } > - if (s->sr[VGA_SEQ_CLOCK_MODE] & 0x08) { > + if (sr(s, VGA_SEQ_CLOCK_MODE) & 0x08) { > cwidth = 16; /* NOTE: no 18 pixel wide */ > } > width = (s->cr[VGA_CRTC_H_DISP] + 1); > @@ -1197,7 +1201,7 @@ static void vga_draw_text(VGACommonState *s, int full_update) > int64_t now = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL); > > /* compute font data address (in plane 2) */ > - v = s->sr[VGA_SEQ_CHARACTER_MAP]; > + v = sr(s, VGA_SEQ_CHARACTER_MAP); > offset = (((v >> 4) & 1) | ((v << 1) & 6)) * 8192 * 4 + 2; > if (offset != s->font_offsets[0]) { > s->font_offsets[0] = offset; > @@ -1506,11 +1510,11 @@ static void vga_draw_graphic(VGACommonState *s, int full_update) > } > > if (shift_control == 0) { > - if (s->sr[VGA_SEQ_CLOCK_MODE] & 8) { > + if (sr(s, VGA_SEQ_CLOCK_MODE) & 8) { > disp_width <<= 1; > } > } else if (shift_control == 1) { > - if (s->sr[VGA_SEQ_CLOCK_MODE] & 8) { > + if (sr(s, VGA_SEQ_CLOCK_MODE) & 8) { > disp_width <<= 1; > } > } > @@ -1574,7 +1578,7 @@ static void vga_draw_graphic(VGACommonState *s, int full_update) > > if (shift_control == 0) { > full_update |= update_palette16(s); > - if (s->sr[VGA_SEQ_CLOCK_MODE] & 8) { > + if (sr(s, VGA_SEQ_CLOCK_MODE) & 8) { > v = VGA_DRAW_LINE4D2; > } else { > v = VGA_DRAW_LINE4; > @@ -1582,7 +1586,7 @@ static void vga_draw_graphic(VGACommonState *s, int full_update) > bits = 4; > } else if (shift_control == 1) { > full_update |= update_palette16(s); > - if (s->sr[VGA_SEQ_CLOCK_MODE] & 8) { > + if (sr(s, VGA_SEQ_CLOCK_MODE) & 8) { > v = VGA_DRAW_LINE2D2; > } else { > v = VGA_DRAW_LINE2; > @@ -1629,7 +1633,7 @@ static void vga_draw_graphic(VGACommonState *s, int full_update) > #if 0 > printf("w=%d h=%d v=%d line_offset=%d cr[0x09]=0x%02x cr[0x17]=0x%02x linecmp=%d sr[0x01]=0x%02x\n", > width, height, v, line_offset, s->cr[9], s->cr[VGA_CRTC_MODE], > - s->line_compare, s->sr[VGA_SEQ_CLOCK_MODE]); > + s->line_compare, sr(s, VGA_SEQ_CLOCK_MODE)); > #endif > addr1 = (s->start_addr * 4); > bwidth = (width * bits + 7) / 8; > @@ -1781,6 +1785,7 @@ void vga_common_reset(VGACommonState *s) > { > s->sr_index = 0; > memset(s->sr, '\0', sizeof(s->sr)); > + memset(s->sr_vbe, '\0', sizeof(s->sr_vbe)); > s->gr_index = 0; > memset(s->gr, '\0', sizeof(s->gr)); > s->ar_index = 0; > @@ -1883,10 +1888,10 @@ static void vga_update_text(void *opaque, console_ch_t *chardata) > /* total width & height */ > cheight = (s->cr[VGA_CRTC_MAX_SCAN] & 0x1f) + 1; > cw = 8; > - if (!(s->sr[VGA_SEQ_CLOCK_MODE] & VGA_SR01_CHAR_CLK_8DOTS)) { > + if (!(sr(s, VGA_SEQ_CLOCK_MODE) & VGA_SR01_CHAR_CLK_8DOTS)) { > cw = 9; > } > - if (s->sr[VGA_SEQ_CLOCK_MODE] & 0x08) { > + if (sr(s, VGA_SEQ_CLOCK_MODE) & 0x08) { > cw = 16; /* NOTE: no 18 pixel wide */ > } > width = (s->cr[VGA_CRTC_H_DISP] + 1); > @@ -2053,6 +2058,7 @@ static int vga_common_post_load(void *opaque, int version_id) > > /* force refresh */ > s->graphic_mode = -1; > + vbe_update_vgaregs(s); > return 0; > } > > diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h > index bdb43a5..3ce5544 100644 > --- a/hw/display/vga_int.h > +++ b/hw/display/vga_int.h > @@ -98,6 +98,7 @@ typedef struct VGACommonState { > MemoryRegion chain4_alias; > uint8_t sr_index; > uint8_t sr[256]; > + uint8_t sr_vbe[256]; > uint8_t gr_index; > uint8_t gr[256]; > uint8_t ar_index;
Hi, > > This way we can allow guests update sr[] registers as they want, without > > allowing them disrupt vbe video modes that way. > > Just documenting my test with the patch here: > > This fixes the issue with QEMU 2.5.1.1 but only if I'm using SeaBIOS. > > OVMF leads to a almost similar result as without the patch, after > "windows is loading files" the "Starting Windows" appears short then > hangs then the screen remains blank, so the blank screen is new with OVMF. Doesn't reproduce here. Details please: Which ovmf version? With/without csm? 32/64 bit windows version? cheers, Gerd
Hi, On 05/17/2016 12:50 PM, Gerd Hoffmann wrote: > Hi, > >>> This way we can allow guests update sr[] registers as they want, without >>> allowing them disrupt vbe video modes that way. >> Just documenting my test with the patch here: >> >> This fixes the issue with QEMU 2.5.1.1 but only if I'm using SeaBIOS. >> >> OVMF leads to a almost similar result as without the patch, after >> "windows is loading files" the "Starting Windows" appears short then >> hangs then the screen remains blank, so the blank screen is new with OVMF. > Doesn't reproduce here. > > Details please: Which ovmf version? With/without csm? 32/64 bit > windows version? I tested with: edk2.git-ovmf-x64-0-20151117.b1317.g386cdfb.noarch.rpm $ git show 386cdfb commit 386cdfbecbbacb600ffc8e2ffa8c7af1b3855a61 Author: Mark Rutland <mark.rutland@arm.com> Date: Tue Nov 17 13:58:19 2015 +0000 which I used for a little while now and to see if its just my "old" OVMF I also tested the newest version from your jenkins repo (much thanks for them!!): git show 05b2f9c commit 05b2f9c94e0c0b663ff2d2fb55397d8215eeb3f5 Author: Dandan Bi <dandan.bi@intel.com> Date: Tue May 10 18:51:44 2016 +0800 So without csm. The OS is 64bit windows 7 professional SP1 The QEMU command for my test: ./x86_64-softmmu/qemu-system-x86_64 -boot d -cdrom W7SP1_PROFESSIONAL.iso -m 1024 -smp 2 -enable-kvm -cpu host -drive if=pflash,format=raw,unit=0,readonly,file=OVMF_CODE-pure-efi.fd -drive if=pflash,format=raw,unit=1,file=/tmp/OVMF_VARS.fd Happy to provide more details/test if needed. best regards, Thomas
Hi, > ./x86_64-softmmu/qemu-system-x86_64 -boot d -cdrom > W7SP1_PROFESSIONAL.iso -m 1024 -smp 2 -enable-kvm -cpu host -drive > if=pflash,format=raw,unit=0,readonly,file=OVMF_CODE-pure-efi.fd -drive > if=pflash,format=raw,unit=1,file=/tmp/OVMF_VARS.fd Still not reproduced. Installed win7, then updated with sp1, rebooted, still working fine. Can you double-check you really tested with a fixed qemu version? I'll go fetch a win7sp1.iso meanwhile ... cheers, Gerd
Hi, sorry for the delay. On 20.05.2016 12:06, Gerd Hoffmann wrote: > Hi, > >> ./x86_64-softmmu/qemu-system-x86_64 -boot d -cdrom >> W7SP1_PROFESSIONAL.iso -m 1024 -smp 2 -enable-kvm -cpu host -drive >> if=pflash,format=raw,unit=0,readonly,file=OVMF_CODE-pure-efi.fd -drive >> if=pflash,format=raw,unit=1,file=/tmp/OVMF_VARS.fd > Still not reproduced. Installed win7, then updated with sp1, rebooted, > still working fine. > > Can you double-check you really tested with a fixed qemu version? I checked on an Arch Host and there I cannot reproduce this, it works fine, with and without OVMF. Sorry for causing trouble/noise, it seems that the Debian based host has here another problem (here it resets constantly with OVMF). For a "tested by" (if even wanted) I'd like to recheck on a plain Debian Jessie tomorrow, this didn't had any suspicious qemu-vga related packages installed or modified but maybe I'm overlooking something. Really thanks for the quick patch and the fast bug recognition! cheers, Thomas
On 05/23/2016 11:39 PM, Thomas Lamprecht wrote: > Hi, > > sorry for the delay. > > On 20.05.2016 12:06, Gerd Hoffmann wrote: >> Hi, >> >>> ./x86_64-softmmu/qemu-system-x86_64 -boot d -cdrom >>> W7SP1_PROFESSIONAL.iso -m 1024 -smp 2 -enable-kvm -cpu host -drive >>> if=pflash,format=raw,unit=0,readonly,file=OVMF_CODE-pure-efi.fd -drive >>> if=pflash,format=raw,unit=1,file=/tmp/OVMF_VARS.fd >> Still not reproduced. Installed win7, then updated with sp1, rebooted, >> still working fine. >> >> Can you double-check you really tested with a fixed qemu version? > > I checked on an Arch Host and there I cannot reproduce this, it works > fine, with and without OVMF. > Sorry for causing trouble/noise, it seems that the Debian based host > has here another problem (here it resets constantly with OVMF). > For a "tested by" (if even wanted) I'd like to recheck on a plain > Debian Jessie tomorrow, this didn't had any suspicious qemu-vga > related packages installed or modified but maybe I'm overlooking > something. > I can reproduce it on a pure Debian Jessie system. It works with the patch and without OVMF but not with OVMF, after the "Windows loading files" finishes it just resets the VM. So on Arch I have no problems but on Debian. Kernel 3.16 vs 4.5 and Gcc 4.9 vs 6.1. OVMF was the same version on both, namely 26bd643 pure (from your repo). Although the Debian based Proxmox VE Distro has Kernel 4.4 (Ubuntu Kernel) and there it doesn't works also, GCC is the same there (just for info). The used Kernel: Linux debian-pure 3.16.0-4-amd64 #1 SMP Debian 3.16.7-ckt25-2 (2016-04-08) x86_64 GNU/Linux GCC: Using built-in specs. COLLECT_GCC=gcc COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/4.9/lto-wrapper Target: x86_64-linux-gnu Configured with: ../src/configure -v --with-pkgversion='Debian 4.9.2-10' --with-bugurl=file:///usr/share/doc/gcc-4.9/README.Bugs --enable-languages=c,c++,java,go,d,fortran,objc,obj-c++ --prefix=/usr --program-suffix=-4.9 --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --with-gxx-include-dir=/usr/include/c++/4.9 --libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --enable-gnu-unique-object --disable-vtable-verify --enable-plugin --with-system-zlib --disable-browser-plugin --enable-java-awt=gtk --enable-gtk-cairo --with-java-home=/usr/lib/jvm/java-1.5.0-gcj-4.9-amd64/jre --enable-java-home --with-jvm-root-dir=/usr/lib/jvm/java-1.5.0-gcj-4.9-amd64 --with-jvm-jar-dir=/usr/lib/jvm-exports/java-1.5.0-gcj-4.9-amd64 --with-arch-directory=amd64 --with-ecj-jar=/usr/share/java/eclipse-ecj.jar --enable-objc-gc --enable-multiarch --with-arch-32=i586 --with-abi=m64 --with-multilib-list=m32,m64,mx32 --enable-multilib --with-tune=generic --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu Thread model: posix gcc version 4.9.2 (Debian 4.9.2-10) My command: ./x86_64-softmmu/qemu-system-x86_64 -boot d -cdrom /mnt/iso/template/iso/W7SP1_PROFESSIONAL.iso -m 2048 -enable-kvm -drive if=pflash,format=raw,unit=0,readonly,file=/root/OVMF_CODE-pure-efi.fd -drive if=pflash,format=raw,unit=1,file=/root/OVMF_VARS-pure-efi.fd My ./configure for this test: Install prefix /usr/local BIOS directory /usr/local/share/qemu binary directory /usr/local/bin library directory /usr/local/lib module directory /usr/local/lib/qemu libexec directory /usr/local/libexec include directory /usr/local/include config directory /usr/local/etc local state directory /usr/local/var Manual directory /usr/local/share/man ELF interp prefix /usr/gnemul/qemu-%M Source path /root/qemu C compiler cc Host C compiler cc C++ compiler c++ Objective-C compiler cc ARFLAGS rv CFLAGS -O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -pthread -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -g QEMU_CFLAGS -I/usr/include/pixman-1 -Werror -fPIE -DPIE -m64 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -Wendif-labels -Wmissing-include-dirs -Wempty-body -Wnested-externs -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-declaration -Wold-style-definition -Wtype-limits -fstack-protector-strong -I/usr/include/libpng12 LDFLAGS -Wl,--warn-common -Wl,-z,relro -Wl,-z,now -pie -m64 -g make make install install python python -B smbd /usr/sbin/smbd module support no host CPU x86_64 host big endian no target list x86_64-softmmu tcg debug enabled no gprof enabled no sparse enabled no strip binaries no profiler no static build no pixman system SDL support no GTK support yes GTK GL support no GNUTLS support no GNUTLS hash no GNUTLS rnd no libgcrypt no libgcrypt kdf no nettle yes (2.7.1) nettle kdf no libtasn1 yes VTE support no curses support no virgl support no curl support no mingw32 support no Audio drivers alsa Block whitelist (rw) Block whitelist (ro) VirtFS support no VNC support yes VNC SASL support no VNC JPEG support yes VNC PNG support yes xen support no brlapi support no bluez support no Documentation yes PIE yes vde support no netmap support no Linux AIO support no ATTR/XATTR support yes Install blobs yes KVM support yes RDMA support no TCG interpreter no fdt support yes preadv support yes fdatasync yes madvise yes posix_madvise yes sigev_thread_id yes uuid support yes libcap-ng support no vhost-net support yes vhost-scsi support yes Trace backends log spice support no rbd support no xfsctl support no smartcard support no libusb no usb net redir no OpenGL support no OpenGL dmabufs no libiscsi support no libnfs support no build guest agent yes QGA VSS support no QGA w32 disk info no QGA MSI support no seccomp support no coroutine backend ucontext coroutine pool yes GlusterFS support no Archipelago support no gcov gcov gcov enabled no TPM support yes libssh2 support no TPM passthrough yes QOM debugging yes vhdx yes lzo support no snappy support no bzip2 support no NUMA host support no tcmalloc support no jemalloc support no avx2 optimization yes
diff --git a/hw/display/vga.c b/hw/display/vga.c index 4a55ec6..9ebc54f 100644 --- a/hw/display/vga.c +++ b/hw/display/vga.c @@ -149,6 +149,11 @@ static inline bool vbe_enabled(VGACommonState *s) return s->vbe_regs[VBE_DISPI_INDEX_ENABLE] & VBE_DISPI_ENABLED; } +static inline uint8_t sr(VGACommonState *s, int idx) +{ + return vbe_enabled(s) ? s->sr_vbe[idx] : s->sr[idx]; +} + static void vga_update_memory_access(VGACommonState *s) { hwaddr base, offset, size; @@ -163,8 +168,8 @@ static void vga_update_memory_access(VGACommonState *s) s->has_chain4_alias = false; s->plane_updated = 0xf; } - if ((s->sr[VGA_SEQ_PLANE_WRITE] & VGA_SR02_ALL_PLANES) == - VGA_SR02_ALL_PLANES && s->sr[VGA_SEQ_MEMORY_MODE] & VGA_SR04_CHN_4M) { + if ((sr(s, VGA_SEQ_PLANE_WRITE) & VGA_SR02_ALL_PLANES) == + VGA_SR02_ALL_PLANES && sr(s, VGA_SEQ_MEMORY_MODE) & VGA_SR04_CHN_4M) { offset = 0; switch ((s->gr[VGA_GFX_MISC] >> 2) & 3) { case 0: @@ -234,7 +239,7 @@ static void vga_precise_update_retrace_info(VGACommonState *s) ((s->cr[VGA_CRTC_OVERFLOW] >> 6) & 2)) << 8); vretr_end_line = s->cr[VGA_CRTC_V_SYNC_END] & 0xf; - clocking_mode = (s->sr[VGA_SEQ_CLOCK_MODE] >> 3) & 1; + clocking_mode = (sr(s, VGA_SEQ_CLOCK_MODE) >> 3) & 1; clock_sel = (s->msr >> 2) & 3; dots = (s->msr & 1) ? 8 : 9; @@ -486,7 +491,6 @@ void vga_ioport_write(void *opaque, uint32_t addr, uint32_t val) printf("vga: write SR%x = 0x%02x\n", s->sr_index, val); #endif s->sr[s->sr_index] = val & sr_mask[s->sr_index]; - vbe_update_vgaregs(s); if (s->sr_index == VGA_SEQ_CLOCK_MODE) { s->update_retrace_info(s); } @@ -680,13 +684,13 @@ static void vbe_update_vgaregs(VGACommonState *s) if (s->vbe_regs[VBE_DISPI_INDEX_BPP] == 4) { shift_control = 0; - s->sr[VGA_SEQ_CLOCK_MODE] &= ~8; /* no double line */ + s->sr_vbe[VGA_SEQ_CLOCK_MODE] &= ~8; /* no double line */ } else { shift_control = 2; /* set chain 4 mode */ - s->sr[VGA_SEQ_MEMORY_MODE] |= VGA_SR04_CHN_4M; + s->sr_vbe[VGA_SEQ_MEMORY_MODE] |= VGA_SR04_CHN_4M; /* activate all planes */ - s->sr[VGA_SEQ_PLANE_WRITE] |= VGA_SR02_ALL_PLANES; + s->sr_vbe[VGA_SEQ_PLANE_WRITE] |= VGA_SR02_ALL_PLANES; } s->gr[VGA_GFX_MODE] = (s->gr[VGA_GFX_MODE] & ~0x60) | (shift_control << 5); @@ -836,7 +840,7 @@ uint32_t vga_mem_readb(VGACommonState *s, hwaddr addr) break; } - if (s->sr[VGA_SEQ_MEMORY_MODE] & VGA_SR04_CHN_4M) { + if (sr(s, VGA_SEQ_MEMORY_MODE) & VGA_SR04_CHN_4M) { /* chain 4 mode : simplest access */ assert(addr < s->vram_size); ret = s->vram_ptr[addr]; @@ -904,11 +908,11 @@ void vga_mem_writeb(VGACommonState *s, hwaddr addr, uint32_t val) break; } - if (s->sr[VGA_SEQ_MEMORY_MODE] & VGA_SR04_CHN_4M) { + if (sr(s, VGA_SEQ_MEMORY_MODE) & VGA_SR04_CHN_4M) { /* chain 4 mode : simplest access */ plane = addr & 3; mask = (1 << plane); - if (s->sr[VGA_SEQ_PLANE_WRITE] & mask) { + if (sr(s, VGA_SEQ_PLANE_WRITE) & mask) { assert(addr < s->vram_size); s->vram_ptr[addr] = val; #ifdef DEBUG_VGA_MEM @@ -921,7 +925,7 @@ void vga_mem_writeb(VGACommonState *s, hwaddr addr, uint32_t val) /* odd/even mode (aka text mode mapping) */ plane = (s->gr[VGA_GFX_PLANE_READ] & 2) | (addr & 1); mask = (1 << plane); - if (s->sr[VGA_SEQ_PLANE_WRITE] & mask) { + if (sr(s, VGA_SEQ_PLANE_WRITE) & mask) { addr = ((addr & ~1) << 1) | plane; if (addr >= s->vram_size) { return; @@ -996,7 +1000,7 @@ void vga_mem_writeb(VGACommonState *s, hwaddr addr, uint32_t val) do_write: /* mask data according to sr[2] */ - mask = s->sr[VGA_SEQ_PLANE_WRITE]; + mask = sr(s, VGA_SEQ_PLANE_WRITE); s->plane_updated |= mask; /* only used to detect font change */ write_mask = mask16[mask]; if (addr * sizeof(uint32_t) >= s->vram_size) { @@ -1152,10 +1156,10 @@ static void vga_get_text_resolution(VGACommonState *s, int *pwidth, int *pheight /* total width & height */ cheight = (s->cr[VGA_CRTC_MAX_SCAN] & 0x1f) + 1; cwidth = 8; - if (!(s->sr[VGA_SEQ_CLOCK_MODE] & VGA_SR01_CHAR_CLK_8DOTS)) { + if (!(sr(s, VGA_SEQ_CLOCK_MODE) & VGA_SR01_CHAR_CLK_8DOTS)) { cwidth = 9; } - if (s->sr[VGA_SEQ_CLOCK_MODE] & 0x08) { + if (sr(s, VGA_SEQ_CLOCK_MODE) & 0x08) { cwidth = 16; /* NOTE: no 18 pixel wide */ } width = (s->cr[VGA_CRTC_H_DISP] + 1); @@ -1197,7 +1201,7 @@ static void vga_draw_text(VGACommonState *s, int full_update) int64_t now = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL); /* compute font data address (in plane 2) */ - v = s->sr[VGA_SEQ_CHARACTER_MAP]; + v = sr(s, VGA_SEQ_CHARACTER_MAP); offset = (((v >> 4) & 1) | ((v << 1) & 6)) * 8192 * 4 + 2; if (offset != s->font_offsets[0]) { s->font_offsets[0] = offset; @@ -1506,11 +1510,11 @@ static void vga_draw_graphic(VGACommonState *s, int full_update) } if (shift_control == 0) { - if (s->sr[VGA_SEQ_CLOCK_MODE] & 8) { + if (sr(s, VGA_SEQ_CLOCK_MODE) & 8) { disp_width <<= 1; } } else if (shift_control == 1) { - if (s->sr[VGA_SEQ_CLOCK_MODE] & 8) { + if (sr(s, VGA_SEQ_CLOCK_MODE) & 8) { disp_width <<= 1; } } @@ -1574,7 +1578,7 @@ static void vga_draw_graphic(VGACommonState *s, int full_update) if (shift_control == 0) { full_update |= update_palette16(s); - if (s->sr[VGA_SEQ_CLOCK_MODE] & 8) { + if (sr(s, VGA_SEQ_CLOCK_MODE) & 8) { v = VGA_DRAW_LINE4D2; } else { v = VGA_DRAW_LINE4; @@ -1582,7 +1586,7 @@ static void vga_draw_graphic(VGACommonState *s, int full_update) bits = 4; } else if (shift_control == 1) { full_update |= update_palette16(s); - if (s->sr[VGA_SEQ_CLOCK_MODE] & 8) { + if (sr(s, VGA_SEQ_CLOCK_MODE) & 8) { v = VGA_DRAW_LINE2D2; } else { v = VGA_DRAW_LINE2; @@ -1629,7 +1633,7 @@ static void vga_draw_graphic(VGACommonState *s, int full_update) #if 0 printf("w=%d h=%d v=%d line_offset=%d cr[0x09]=0x%02x cr[0x17]=0x%02x linecmp=%d sr[0x01]=0x%02x\n", width, height, v, line_offset, s->cr[9], s->cr[VGA_CRTC_MODE], - s->line_compare, s->sr[VGA_SEQ_CLOCK_MODE]); + s->line_compare, sr(s, VGA_SEQ_CLOCK_MODE)); #endif addr1 = (s->start_addr * 4); bwidth = (width * bits + 7) / 8; @@ -1781,6 +1785,7 @@ void vga_common_reset(VGACommonState *s) { s->sr_index = 0; memset(s->sr, '\0', sizeof(s->sr)); + memset(s->sr_vbe, '\0', sizeof(s->sr_vbe)); s->gr_index = 0; memset(s->gr, '\0', sizeof(s->gr)); s->ar_index = 0; @@ -1883,10 +1888,10 @@ static void vga_update_text(void *opaque, console_ch_t *chardata) /* total width & height */ cheight = (s->cr[VGA_CRTC_MAX_SCAN] & 0x1f) + 1; cw = 8; - if (!(s->sr[VGA_SEQ_CLOCK_MODE] & VGA_SR01_CHAR_CLK_8DOTS)) { + if (!(sr(s, VGA_SEQ_CLOCK_MODE) & VGA_SR01_CHAR_CLK_8DOTS)) { cw = 9; } - if (s->sr[VGA_SEQ_CLOCK_MODE] & 0x08) { + if (sr(s, VGA_SEQ_CLOCK_MODE) & 0x08) { cw = 16; /* NOTE: no 18 pixel wide */ } width = (s->cr[VGA_CRTC_H_DISP] + 1); @@ -2053,6 +2058,7 @@ static int vga_common_post_load(void *opaque, int version_id) /* force refresh */ s->graphic_mode = -1; + vbe_update_vgaregs(s); return 0; } diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h index bdb43a5..3ce5544 100644 --- a/hw/display/vga_int.h +++ b/hw/display/vga_int.h @@ -98,6 +98,7 @@ typedef struct VGACommonState { MemoryRegion chain4_alias; uint8_t sr_index; uint8_t sr[256]; + uint8_t sr_vbe[256]; uint8_t gr_index; uint8_t gr[256]; uint8_t ar_index;
Commit "fd3c136 vga: make sure vga register setup for vbe stays intact (CVE-2016-3712)." causes a regression. The win7 installer is unhappy because it can't freely modify vga registers any more while in vbe mode. This patch introduces a new sr_vbe register set. The vbe_update_vgaregs will fill sr_vbe[] instead of sr[]. Normal vga register reads and writes go to sr[]. Any sr register read access happens through a new sr() helper function which will read from sr_vbe[] with vbe active and from sr[] otherwise. This way we can allow guests update sr[] registers as they want, without allowing them disrupt vbe video modes that way. Reported-by: Thomas Lamprecht <thomas@lamprecht.org> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/display/vga.c | 50 ++++++++++++++++++++++++++++---------------------- hw/display/vga_int.h | 1 + 2 files changed, 29 insertions(+), 22 deletions(-)