Re: Abort in mch_update_pciexbar

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 5/11/20 9:39 AM, Michael S. Tsirkin wrote:
> On Mon, May 11, 2020 at 09:10:48AM +0200, Philippe Mathieu-Daudé wrote:
>> On 5/11/20 8:19 AM, Philippe Mathieu-Daudé wrote:
>>> On 5/11/20 6:59 AM, Alexander Bulekov wrote:
>>>> Hello,
>>>> While fuzzing, I found an input that triggers an assertion failure in
>>>> mch_update_pciexbar:
>>>>
>>>> #6 0x7f38d387c55a in abort
>>>> /build/glibc-GwnBeO/glibc-2.30/stdlib/abort.c:79:7
>>>> #7 0x55c27e94ffd0 in mch_update_pciexbar hw/pci-host/q35.c:331:9
>>>> #8 0x55c27e94db38 in mch_write_config hw/pci-host/q35.c:487:9
>>>> #9 0x55c27e9e3f4c in pci_host_config_write_common hw/pci/pci_host.c:81:5
>>>> #10 0x55c27e9e5307 in pci_data_write hw/pci/pci_host.c:118:5
>>>> #11 0x55c27e9e6601 in pci_host_data_write hw/pci/pci_host.c:165:9
>>>> #12 0x55c27ca3b17b in memory_region_write_accessor memory.c:496:5
>>>> #13 0x55c27ca3a5e4 in access_with_adjusted_size memory.c:557:18
>>>> #14 0x55c27ca38177 in memory_region_dispatch_write memory.c:1488:16
>>>> #15 0x55c27c721325 in flatview_write_continue exec.c:3174:23
>>>> #16 0x55c27c70994d in flatview_write exec.c:3214:14
>>>> #17 0x55c27c709462 in address_space_write exec.c:3305:18
>>>
>>> These lines don't match QEMU v5.0.0.
>>>
>>>>
>>>> I can reproduce it in a qemu 5.0 build using:
>>>> cat << EOF | ~/Development/qemu/build/i386-softmmu/qemu-system-i386
>>>> -M pc-q35-5.0 -display none -nodefaults -nographic -qtest stdio
>>>> outl 0xcf8 0xf2000060
>>>> outl 0xcfc 0x8400056e
>>>
>>> The guest shouldn't ask for a reserved bar length (grep for
>>> MCH_HOST_BRIDGE_PCIEXBAR_LENGTH_RVD). I suppose we should simply report
>>> it as GUEST_ERROR and ignore it.
>>
>> This patch prevent the crash:
>>
>> -- >8 --
>> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
>> index 2bbc90b28f..2b744aca93 100644
>> --- a/hw/pci-host/q35.c
>> +++ b/hw/pci-host/q35.c
>> @@ -36,6 +36,7 @@
>>   #include "qapi/error.h"
>>   #include "qapi/visitor.h"
>>   #include "qemu/module.h"
>> +#include "qemu/log.h"
>>
>>
>> /****************************************************************************
>>    * Q35 host
>> @@ -320,6 +321,9 @@ static void mch_update_pciexbar(MCHPCIState *mch)
>>           addr_mask |= MCH_HOST_BRIDGE_PCIEXBAR_64ADMSK;
>>           break;
>>       case MCH_HOST_BRIDGE_PCIEXBAR_LENGTH_RVD:
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid use of reserved
>> value\n",
>> +                                       __func__);
>> +        return;
>>       default:
>>           abort();
>>       }
> 
> Maybe report the value too.

This is a 2 bit field, all are covered except 0b11 which is the RESERVED 
case, and the abort() now will never fire. It is unclear why the 
compiler doesn't notice that.

> 
>> ---
>>
>> But the real question is what would the real hardware do in this case.
> 
> Spec doesn't say unfortunately. All it says is:
> 	Designers must not rely on the absence or characteristics of any
> 	features or instructions marked "reserved" or "undefined." Intel
> 	reserves these for
> 
> 	future definition and shall have no responsibility whatsoever
> 	for conflicts or incompatibilities arising from future changes to them.

OK, thanks for checking. I'll submit formal patch with proper description.

> 
>>>
>>>> EOF
>>>>
>>>> I also uploaded the above trace, in case the formatting is broken:
>>>>
>>>> curl https://paste.debian.net/plain/1146095 | qemu-system-i386 -M
>>>> pc-q35-5.0 -display none -nodefaults -nographic -qtest stdio
>>>>
>>>> Please let me know if I can provide any further info.
>>>
>>> It would help the community if you fill your bug reports with Launchpad,
>>> so they don't get lost in the high email flow, and we can track/update
>>> them. See for example:
>>> https://bugs.launchpad.net/qemu/+bug/1835865 and
>>> https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg06082.html
>>> which refers it.
>