RE: [PATCH v2 1/2] Fix undefined behaviour

[Paul Durrant <xadimgnik@gmail.com>]

> -----Original Message-----
> From: Grzegorz Uriasz <gorbak25@gmail.com>
> Sent: 29 April 2020 04:04
> To: qemu-devel@nongnu.org
> Cc: Grzegorz Uriasz <gorbak25@gmail.com>; marmarek@invisiblethingslab.com; artur@puzio.waw.pl;
> jakub@bartmin.ski; j.nowak26@student.uw.edu.pl; Stefano Stabellini <sstabellini@kernel.org>; Anthony
> Perard <anthony.perard@citrix.com>; Paul Durrant <paul@xen.org>; xen-devel@lists.xenproject.org
> Subject: [PATCH v2 1/2] Fix undefined behaviour
> 
> This patch fixes qemu crashes when passing through an IGD device to HVM guests under XEN. The problem
> is that on almost every laptop
> reading the IGD ROM from SYSFS will fail, the reason for it is that the IGD rom is polymorphic and it
> modifies itself
> during bootup - this results in an invalid rom image - the kernel checks whether the image is valid
> and when that's not the case
> it won't allow userspace to read it. It seems that when the code was first written(xen_pt_load_rom.c)
> the kernel's back then didn't check
> whether the rom is valid and just passed the contents to userspace - because of this qemu also tries
> to repair the rom later in the code.
> When stating the rom the kernel isn't validating the rom contents so it is returning the proper size
> of the rom(32 4kb pages).
> 
> This results in undefined behaviour - pci_assign_dev_load_option_rom is creating a buffer and then
> writes the size of the buffer to a pointer.
> In pci_assign_dev_load_option_rom under old kernels if the fstat would succeed then fread would also
> succeed - this means if the buffer
> is allocated the size of the buffer will be set. On newer kernels this is not the case - on all
> laptops I've tested(spanning a few
> generations of IGD) the fstat is successful and the buffer is allocated but the fread is failing - as
> the buffer is not deallocated
> the function is returning a valid pointer without setting the size of the buffer for the caller. The
> caller of pci_assign_dev_load_option_rom
> is holding the size of the buffer in an uninitialized variable and is just checking whether
> pci_assign_dev_load_option_rom returned a valid pointer.
> This later results in cpu_physical_memory_write(0xc0000, result_of_pci_assign_dev_load_option_rom,
> unitialized_variable) which
> depending on the compiler parameters, configure flags, etc... might crash qemu or might work - the xen
> 4.12 stable release with default configure
> parameters works but changing the compiler options slightly(for instance by enabling qemu debug)
> results in qemu crashing ¯\_(;-;)_/¯
> 
> The only situation when the original pci_assign_dev_load_option_rom works is when the IDG is not the

I think you mean IGD

> boot gpu - this won't happen on any laptop and
> will be rare on desktops.
> 
> This patch deallocates the buffer and returns NULL if reading the VBIOS fails - the caller of the
> function then properly shuts down qemu.
> The next patch in the series introduces a better method for getting the vbios so qemu does not exit
> when pci_assign_dev_load_option_rom fails -
> this is the reason I've changed error_report to warn_report as whether a failure in
> pci_assign_dev_load_option_rom is fatal
> depends on the caller.
> 
> Signed-off-by: Grzegorz Uriasz <gorbak25@gmail.com>

With the typo fixed...

Reviewed-by: Paul Durrant <paul@xen.org>