[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH v2 1/2] Fix undefined behaviour
From: |
Paul Durrant |
Subject: |
RE: [PATCH v2 1/2] Fix undefined behaviour |
Date: |
Wed, 29 Apr 2020 09:07:22 +0100 |
> -----Original Message-----
> From: Grzegorz Uriasz <address@hidden>
> Sent: 29 April 2020 04:04
> To: address@hidden
> Cc: Grzegorz Uriasz <address@hidden>; address@hidden; address@hidden;
> address@hidden; address@hidden; Stefano Stabellini <address@hidden>; Anthony
> Perard <address@hidden>; Paul Durrant <address@hidden>; address@hidden
> 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 <address@hidden>
With the typo fixed...
Reviewed-by: Paul Durrant <address@hidden>