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

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Grzegorz Uriasz <gorbak25@xxxxxxxxx>
> Sent: 29 April 2020 04:04
> To: qemu-devel@xxxxxxxxxx
> Cc: Grzegorz Uriasz <gorbak25@xxxxxxxxx>; marmarek@xxxxxxxxxxxxxxxxxxxxxx; 
> artur@xxxxxxxxxxxx;
> jakub@xxxxxxxxxxx; j.nowak26@xxxxxxxxxxxxxxxxx; Stefano Stabellini 
> <sstabellini@xxxxxxxxxx>; Anthony
> Perard <anthony.perard@xxxxxxxxxx>; Paul Durrant <paul@xxxxxxx>; 
> xen-devel@xxxxxxxxxxxxxxxxxxxx
> 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@xxxxxxxxx>

With the typo fixed...

Reviewed-by: Paul Durrant <paul@xxxxxxx>