> -----Original Message----- > From: Grzegorz Uriasz <gorba...@gmail.com> > Sent: 29 April 2020 04:04 > To: qemu-devel@nongnu.org > Cc: Grzegorz Uriasz <gorba...@gmail.com>; marma...@invisiblethingslab.com; > ar...@puzio.waw.pl; > ja...@bartmin.ski; j.nowa...@student.uw.edu.pl; Stefano Stabellini > <sstabell...@kernel.org>; Anthony > Perard <anthony.per...@citrix.com>; Paul Durrant <p...@xen.org>; > xen-de...@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 <gorba...@gmail.com> With the typo fixed... Reviewed-by: Paul Durrant <p...@xen.org>
