Re: [PATCH v2] hw/scsi/scsi-disk: Fix out of bounds access in mode_sense_page()

From: Li Qiang <liq3ea@gmail.com>
To: "Philippe Mathieu-Daudé" <philmd@redhat.com>
Cc: Fam Zheng <fam@euphon.net>, Alexander Bulekov <alxndr@bu.edu>,
	qemu-stable@nongnu.org, Qemu Developers <qemu-devel@nongnu.org>,
	Darren Kenny <darren.kenny@oracle.com>,
	Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH v2] hw/scsi/scsi-disk: Fix out of bounds access in mode_sense_page()
Date: Fri, 5 Feb 2021 12:59:11 +0800	[thread overview]
Message-ID: <CAKXe6SKTj0GwLzR7XTTvtgRhAA3mcXDw1NP_pkmmeeUvqq69DA@mail.gmail.com> (raw)
In-Reply-To: <20210204225041.1822673-1-philmd@redhat.com>

Philippe Mathieu-Daudé <philmd@redhat.com> 于2021年2月5日周五 上午6:50写道：
>
> Per the "SCSI Commands Reference Manual" (Rev. J) chapter 5.3
> "Mode parameters" and table 359 "Mode page codes and subpage
> codes", the last page code is 0x3f. When using it as array index,
> the array must have 0x40 elements. Replace the magic 0x3f value
> by its definition and increase the size of the mode_sense_valid[]
> to avoid an out of bound access (reproducer available in [Buglink]):
>
>   hw/scsi/scsi-disk.c:1104:10: runtime error: index 63 out of bounds for type 'const int [63]'
>   SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior hw/scsi/scsi-disk.c:1104:10 in
>   =================================================================
>   ==1813911==ERROR: AddressSanitizer: global-buffer-overflow
>   READ of size 4 at 0x5624b84aff1c thread T0
>       #0 0x5624b63004b3 in mode_sense_page hw/scsi/scsi-disk.c:1104:10
>       #1 0x5624b630f798 in scsi_disk_check_mode_select hw/scsi/scsi-disk.c:1447:11
>       #2 0x5624b630efb8 in mode_select_pages hw/scsi/scsi-disk.c:1515:17
>       #3 0x5624b630988e in scsi_disk_emulate_mode_select hw/scsi/scsi-disk.c:1570:13
>       #4 0x5624b62f08e7 in scsi_disk_emulate_write_data hw/scsi/scsi-disk.c:1861:9
>       #5 0x5624b62b171b in scsi_req_continue hw/scsi/scsi-bus.c:1391:9
>       #6 0x5624b62b2d4c in scsi_req_data hw/scsi/scsi-bus.c:1427:5
>       #7 0x5624b62f05f6 in scsi_disk_emulate_write_data hw/scsi/scsi-disk.c:1853:9
>       #8 0x5624b62b171b in scsi_req_continue hw/scsi/scsi-bus.c:1391:9
>       #9 0x5624b63e47ed in megasas_enqueue_req hw/scsi/megasas.c:1660:9
>       #10 0x5624b63b9cfa in megasas_handle_scsi hw/scsi/megasas.c:1735:5
>       #11 0x5624b63acf91 in megasas_handle_frame hw/scsi/megasas.c:1974:24
>       #12 0x5624b63aa200 in megasas_mmio_write hw/scsi/megasas.c:2131:9
>       #13 0x5624b63ebed3 in megasas_port_write hw/scsi/megasas.c:2182:5
>       #14 0x5624b6f43568 in memory_region_write_accessor softmmu/memory.c:491:5
>
> Cc: qemu-stable@nongnu.org
> Reported-by: OSS-Fuzz
> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1914638
> Fixes: a8f4bbe2900 ("scsi-disk: store valid mode pages in a table")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Li Qiang <liq3ea@gmail.com>

> ---
> v2: Mention reproducer link
> ---
>  hw/scsi/scsi-disk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index ed52fcd49ff..93aec483e88 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -1089,7 +1089,7 @@ static int scsi_emulate_mechanism_status(SCSIDiskState *s, uint8_t *outbuf)
>  static int mode_sense_page(SCSIDiskState *s, int page, uint8_t **p_outbuf,
>                             int page_control)
>  {
> -    static const int mode_sense_valid[0x3f] = {
> +    static const int mode_sense_valid[MODE_PAGE_ALLS + 1] = {
>          [MODE_PAGE_HD_GEOMETRY]            = (1 << TYPE_DISK),
>          [MODE_PAGE_FLEXIBLE_DISK_GEOMETRY] = (1 << TYPE_DISK),
>          [MODE_PAGE_CACHING]                = (1 << TYPE_DISK) | (1 << TYPE_ROM),
> --
> 2.26.2
>