Re: 回复: [PATCH 0/2] use unsigned type for MegasasState fields - Philippe Mathieu-Daudé

From: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
To: "Ding, Ren" <rding@gatech.edu>, P J P <ppandit@redhat.com>
Cc: "Fam Zheng" <fam@euphon.net>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"QEMU Developers" <qemu-devel@nongnu.org>,
	"Zhao, Hanqing" <hanqing@gatech.edu>,
	"Marc-André Lureau" <marcandre.lureau@redhat.com>
Subject: Re: 回复: [PATCH 0/2] use unsigned type for MegasasState fields
Date: Tue, 12 May 2020 22:54:58 +0200	[thread overview]
Message-ID: <4a9f8ae4-ef42-5a82-6d36-fb30f2878c6a@amsat.org> (raw)
In-Reply-To: <BN6PR07MB341283EBBF78F86AAA995382CABE0@BN6PR07MB3412.namprd07.prod.outlook.com>

Hi Ren,

On 5/12/20 8:49 PM, Ding, Ren wrote:
> Hi all,
> 
> To clarify, the bug has been reported 6 months ago with the commit 
> version of 98b2e3c9ab3abfe476a2b02f8f51813edb90e72d, which was the 
> upstream back then. The reproducing driver along with the ASAN log we 
> provided was for that version specifically.

When I first read I thought you'd have miswriten 'months' for 'days' or 
'weeks', but this commit is 6 *months* old indeed.

The cover describes the bug as OOB, so I suppose this is a security 
issue. Now a 6 months embargo surprises me. I was expecting some period 
in a 30-90days range to be the default. However reading the 'Publication 
embargo' chapter on https://www.qemu.org/contribute/security-process/, 
it is only stated "Embargo periods will be negotiated by mutual 
agreement between members of the security team and other relevant 
parties to the problem." Shouldn't be a maximum upper limit on the 
embargo period? Are there QEMU security bugs embargoed for more than a 
year? That would be a shame.

> 
> Thanks,
> 
> Ren
> 
> *发件人: *P J P <mailto:ppandit@redhat.com>
> *发送时间: *2020年5月12日14:37
> *收件人: *Philippe Mathieu-Daudé <mailto:philmd@redhat.com>
> *抄送: *QEMU Developers <mailto:qemu-devel@nongnu.org>; Fam Zheng 
> <mailto:fam@euphon.net>; Paolo Bonzini <mailto:pbonzini@redhat.com>; 
> Ding, Ren <mailto:rding@gatech.edu>; Marc-André Lureau 
> <mailto:marcandre.lureau@redhat.com>
> *主题: *Re: [PATCH 0/2] use unsigned type for MegasasState fields
> 
> +-- On Tue, 12 May 2020, Philippe Mathieu-Daudéwrote --+
> | Cc'ing Marc-Andréour signed/unsigned conversion expert (with Paolo).
> 
>    megasas_init_firmware
>      pa_lo = le32_to_cpu(initq->pi_addr_lo);
>      pa_hi = le32_to_cpu(initq->pi_addr_hi);
>      s->producer_pa = ((uint64_t) pa_hi << 32) | pa_lo;
>      s->reply_queue_head = ldl_le_pci_dma(pcid, s->producer_pa);
> 
> IIUC, here ldl_le_pci_dma() returns an 'uint32_t' type, but since
> 'reply_queue_head' is a signed int, large 'uint32_t' value turns negative.
> 
> | Do you have a reproducer?
> 
>    Yes, there is a reproducer with ASAN, though it did not work for me.
> Ren(CC'd) had shared this trace:
> 
> AddressSanitizer: heap-buffer-overflow on address 0x7f9159054058 at pc 
> 0x55763514b5cd bp 0x7f9179bd6d90 sp 0x7f9179bd6d88
> READ of size 8 at 0x7f9159054058 thread T2
>    #0 0x55763514b5cc in megasas_lookup_frame 
> /home/ren/tmp/redacted-dbg/qemu/hw/scsi/megasas.c:449:30
>    #1 0x55763513205c in megasas_handle_abort 
> /home/ren/tmp/redacted-dbg/qemu/hw/scsi/megasas.c:1904:17
>    #2 0x55763512d0f8 in megasas_handle_frame 
> /home/ren/tmp/redacted-dbg/qemu/hw/scsi/megasas.c:1961:24
>    #3 0x55763512ba7d in megasas_mmio_write 
> /home/ren/tmp/redacted-dbg/qemu/hw/scsi/megasas.c:2122:9
>    #4 0x55763515247c in megasas_port_write 
> /home/ren/tmp/redacted-dbg/qemu/hw/scsi/megasas.c:2173:5
>    #5 0x557634621b3b in memory_region_write_accessor 
> /home/ren/tmp/redacted-dbg/qemu/memory.c:483:5
>    #6 0x557634621741 in access_with_adjusted_size 
> /home/ren/tmp/redacted-dbg/qemu/memory.c:544:18
>    #7 0x557634620498 in memory_region_dispatch_write 
> /home/ren/tmp/redacted-dbg/qemu/memory.c:1482:16
>    #8 0x5576344b6b6c in flatview_write_continue 
> /home/ren/tmp/redacted-dbg/qemu/exec.c:3161:23
>    #9 0x5576344a87d9 in flatview_write 
> /home/ren/tmp/redacted-dbg/qemu/exec.c:3201:14
>    #10 0x5576344a8376 in address_space_write 
> /home/ren/tmp/redacted-dbg/qemu/exec.c:3291:18
>    #11 0x5576344a8af4 in address_space_rw 
> /home/ren/tmp/redacted-dbg/qemu/exec.c:3301:16
>    #12 0x557634689e10 in kvm_handle_io 
> /home/ren/tmp/redacted-dbg/qemu/accel/kvm/kvm-all.c:2086:9
>    #13 0x557634688a45 in kvm_cpu_exec 
> /home/ren/tmp/redacted-dbg/qemu/accel/kvm/kvm-all.c:2332:13
>    #14 0x5576345ee7aa in qemu_kvm_cpu_thread_fn 
> /home/ren/tmp/redacted-dbg/qemu/cpus.c:1299:17
>    #15 0x557635a11509 in qemu_thread_start 
> /home/ren/tmp/redacted-dbg/qemu/util/qemu-thread-posix.c:519:9
>    #16 0x7f918cec26b9 in start_thread 
> (/lib/x86_64-linux-gnu/libpthread.so.0+0x76b9)
>    #17 0x7f918c5d441c in clone 
> /build/glibc-LK5gWL/glibc-2.23/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:109

This is previous information useful for the commit description, thanks 
for sharing it!

> 
> 
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
> 