[pve-devel] [PATCH pve-qemu-kvm] fix #991: Can't install Win7/8

[Thomas Lamprecht]

follow up inline;

On 05/11/2016 02:53 PM, Thomas Lamprecht wrote:
> comments inline
>
> On 05/11/2016 02:30 PM, Wolfgang Link wrote:
>> ---
>>  Makefile | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/Makefile b/Makefile
>> index 67b30dd..07cd162 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -24,7 +24,8 @@ download:
>>  	#git clone git://git.qemu-project.org/qemu.git -b stable-2.4 ${KVMDIR} 
>>  	git clone git://git.qemu-project.org/qemu.git ${KVMDIR}
>>  	# see https://bugs.launchpad.net/qemu/+bug/1488363?comments=all
>> -	cd ${KVMDIR}; git checkout v2.5.1.1; git revert --no-edit b8eb5512fd8a115f164edbbe897cdf8884920ccb
>> +	# see for 44b https://bugzilla.proxmox.com/show_bug.cgi?id=991
>> +	cd ${KVMDIR}; git checkout v2.5.1.1; git revert --no-edit b8eb5512fd8a115f164edbbe897cdf8884920ccb; git revert --no-edit 44b86aa32e4147c727fadd9a0f0bc503a5dedb72;
> The second commit reverts a CVE fix? Its a fix for a DoS of the guest so
> it may be seen as less critical but still, is upstream informed on this,
> I mean if this one breaks win7/8 also in qemu 2.6 this could be a
> blocker for them...
>
> Respective commit is:
>
>> commit 44b86aa32e4147c727fadd9a0f0bc503a5dedb72
>> Author: Gerd Hoffmann <kraxel at redhat.com>
>> Date:   Tue Apr 26 14:48:06 2016 +0200
>>
>>     vga: make sure vga register setup for vbe stays intact
>> (CVE-2016-3712).
>>    
>>     Call vbe_update_vgaregs() when the guest touches GFX, SEQ or CRT
>>     registers, to make sure the vga registers will always have the
>>     values needed by vbe mode.  This makes sure the sanity checks
>>     applied by vbe_fixup_regs() are effective.
>>    
>>     Without this guests can muck with shift_control, can turn on planar
>>     vga modes or text mode emulation while VBE is active, making qemu
>>     take code paths meant for CGA compatibility, but with the very
>>     large display widths and heigts settable using VBE registers.
>>    
>>     Which is good for one or another buffer overflow.  Not that
>>     critical as they typically read overflows happening somewhere
>>     in the display code.  So guests can DoS by crashing qemu with a
>>     segfault, but it is probably not possible to break out of the VM.
>>    
>>     Fixes: CVE-2016-3712
>>     Reported-by: Zuozhi Fzz <zuozhi.fzz at alibaba-inc.com>
>>     Reported-by: P J P <ppandit at redhat.com>
>>     Signed-off-by: Gerd Hoffmann <kraxel at redhat.com>
>>     Signed-off-by: Michael Roth <mdroth at linux.vnet.ibm.com>
>>
>> diff --git a/hw/display/vga.c b/hw/display/vga.c
>> index 10ac7df..679070e 100644
>> --- a/hw/display/vga.c
>> +++ b/hw/display/vga.c
>> @@ -140,6 +140,8 @@ static uint32_t expand4[256];
>>  static uint16_t expand2[256];
>>  static uint8_t expand4to8[16];
>>  
>> +static void vbe_update_vgaregs(VGACommonState *s);
>> +
>>  static inline bool vbe_enabled(VGACommonState *s)
>>  {
>>      return s->vbe_regs[VBE_DISPI_INDEX_ENABLE] & VBE_DISPI_ENABLED;
>> @@ -482,6 +484,7 @@ 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);
>>          }
>> @@ -513,6 +516,7 @@ void vga_ioport_write(void *opaque, uint32_t addr,
>> uint32_t val)
>>          printf("vga: write GR%x = 0x%02x\n", s->gr_index, val);
>>  #endif
>>          s->gr[s->gr_index] = val & gr_mask[s->gr_index];
>> +        vbe_update_vgaregs(s);
>>          vga_update_memory_access(s);
>>          break;
>>      case VGA_CRT_IM:
>> @@ -531,10 +535,12 @@ void vga_ioport_write(void *opaque, uint32_t
>> addr, uint32_t val)
>>              if (s->cr_index == VGA_CRTC_OVERFLOW) {
>>                  s->cr[VGA_CRTC_OVERFLOW] = (s->cr[VGA_CRTC_OVERFLOW]
>> & ~0x10) |
>>                      (val & 0x10);
>> +                vbe_update_vgaregs(s);
>>              }
>>              return;
>>          }
>>          s->cr[s->cr_index] = val;
>> +        vbe_update_vgaregs(s);
>>  
>>          switch(s->cr_index) {
>>          case VGA_CRTC_H_TOTAL:
>>
> I'll do some checks on QEMU 2.6-rc5 which includes this also.
> For the future, could you please explain in the CI message which commit
> you revert and why, not that we open a security hole because of a
> "workaround" by accident.

I can circumvent the hanging windows 7 installation with choosing "Linux
2.6,3.X,4.X" as OS for the installation, then it works just fine!

Also qemu 2.6-rc5 - with the here reverted patches and the PVE patchset
ontop - hangs on a windows 7 Installation *when* Windows 7 is chosen as OS.
So the CVE patch probably isn't the real problem it triggers a side
effect/bug with a parameter I guess

I'll look into what we do different on windows/linux guest OS and if
there is something not quite correct anymore, anyhow I'm against
reverting the CVE as a workaround (without any *good* explanation why we
should revert it, i.e. what functionality inside QEMU it breaks in which
way), a workaround for now is choosing Linux as OS during installation.

Thanks,
Thomas

>>  	tar czf ${KVMSRC} --exclude CVS --exclude .git --exclude .svn ${KVMDIR}
>>  
>>  ${DEBS} kvm: ${KVMSRC}
>
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>