[Qemu-devel] [PATCH 0/1] qemu/nbd: fix segmentation fault when .desc is not null-terminated

[Qemu-devel] [PATCH 0/1] qemu/nbd: fix segmentation fault when .desc is not null-terminated

[Murilo Opsfelder Araujo]

Hello, QEMU community.

The following patch is to fix a segmentation fault when nbd_runtime_opts.desc is
not null-terminated.

This issue emerged when building QEMU with --enable-gcov, as described by
https://bugs.launchpad.net/qemu/+bug/1727259.

To reproduce on ppc64le, run the following commands on QEMU git tree:

mkdir build
mkdir -p tests/qemu-iotests/scratch
cd build
../configure --target-list=ppc64-softmmu --enable-gcov
make -j$(nproc)

./qemu-img create -f qcow2 -o compat=1.1 ./tests/qemu-iotests/scratch/t.qcow2 64M
./qemu-io --cache writeback -f qcow2 -c "write -P 0xa 0x1000 0x1000" ./tests/qemu-iotests/scratch/t.qcow2
./qemu-io --cache writeback -f qcow2 -c "write -P 0xb 0x2000 0x1000" ./tests/qemu-iotests/scratch/t.qcow2
./qemu-img snapshot -c sn1 ./tests/qemu-iotests/scratch/t.qcow2
./qemu-io --cache writeback -f qcow2 -c "write -P 0xc 0x1000 0x1000" ./tests/qemu-iotests/scratch/t.qcow2
./qemu-io --cache writeback -f qcow2 -c "write -P 0xd 0x2000 0x1000" ./tests/qemu-iotests/scratch/t.qcow2
./qemu-img check -f qcow2 ./tests/qemu-iotests/scratch/t.qcow2
./qemu-io --cache writeback -f qcow2 -c "read -P 0xc 0x1000 0x1000" ./tests/qemu-iotests/scratch/t.qcow2
./qemu-io --cache writeback -f qcow2 -c "read -P 0xd 0x2000 0x1000" ./tests/qemu-iotests/scratch/t.qcow2

# run in background, wait some seconds until it starts
./qemu-nbd -v -t -k ${PWD}/tests/qemu-iotests/scratch/test_qemu_nbd_socket ./tests/qemu-iotests/scratch/t.qcow2 -l sn1 &
sleep 5

# this will cause qemu-io to segfault
./qemu-io --cache writeback -f qcow2 -f raw --cache=writeback -r -c "read -P 0xa 0x1000 0x1000" nbd:unix:${PWD}/tests/qemu-iotests/scratch/test_qemu_nbd_socket

# kill qemu-nbd
kill $(pidof qemu-nbd)

Cheers
Murilo

Murilo Opsfelder Araujo (1):
  block/nbd: fix segmentation fault when .desc is not null-terminated

 block/nbd.c | 1 +
 1 file changed, 1 insertion(+)

-- 
2.14.3

[Qemu-devel] [PATCH 1/1] block/nbd: fix segmentation fault when .desc is not null-terminated

[Murilo Opsfelder Araujo]

The find_desc_by_name() from util/qemu-option.c relies on the .name not being
NULL to call strcmp(). This check becomes unsafe when the list is not
NULL-terminated, which is the case of nbd_runtime_opts in block/nbd.c, and can
result in segmentation fault when strcmp() tries to access an invalid memory:

    #0 0x00007fff8c75f7d4 in __strcmp_power9 () from /lib64/libc.so.6
    #1 0x00000000102d3ec8 in find_desc_by_name (desc=0x1036d6f0, name=0x28e46670 "server.path") at util/qemu-option.c:166
    #2 0x00000000102d93e0 in qemu_opts_absorb_qdict (opts=0x28e47a80, qdict=0x28e469a0, errp=0x7fffec247c98) at util/qemu-option.c:1026
    #3 0x000000001012a2e4 in nbd_open (bs=0x28e42290, options=0x28e469a0, flags=24578, errp=0x7fffec247d80) at block/nbd.c:406
    #4 0x00000000100144e8 in bdrv_open_driver (bs=0x28e42290, drv=0x1036e070 <bdrv_nbd_unix>, node_name=0x0, options=0x28e469a0, open_flags=24578, errp=0x7fffec247f50) at block.c:1135
    #5 0x0000000010015b04 in bdrv_open_common (bs=0x28e42290, file=0x0, options=0x28e469a0, errp=0x7fffec247f50) at block.c:1395

>From gdb, the desc[i].name was not NULL and resulted in strcmp() accessing an
invalid memory:

    >>> p desc[5]
    $8 = {
      name = 0x1037f098 "R27A",
      type = 1561964883,
      help = 0xc0bbb23e <error: Cannot access memory at address 0xc0bbb23e>,
      def_value_str = 0x2 <error: Cannot access memory at address 0x2>
    }
    >>> p desc[6]
    $9 = {
      name = 0x103dac78 <__gcov0.do_qemu_init_bdrv_nbd_init> "\001",
      type = 272101528,
      help = 0x29ec0b754403e31f <error: Cannot access memory at address 0x29ec0b754403e31f>,
      def_value_str = 0x81f343b9 <error: Cannot access memory at address 0x81f343b9>
    }

This patch fixes the segmentation fault in strcmp() by adding a NULL element at
the end of nbd_runtime_opts.desc list, which is the common practice to most of
other structs like runtime_opts in block/null.c. Thus, the desc[i].name != NULL
check becomes safe because it will not evaluate to true when .desc list reached
its end.

Reported-by: R. Nageswara Sastry <nasastry@in.ibm.com>
Buglink: https://bugs.launchpad.net/qemu/+bug/1727259
Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.vnet.ibm.com>
---
 block/nbd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/nbd.c b/block/nbd.c
index a50d24b50a..8b8ba56cdd 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -388,6 +388,7 @@ static QemuOptsList nbd_runtime_opts = {
             .type = QEMU_OPT_STRING,
             .help = "ID of the TLS credentials to use",
         },
+        { /* end of list */ }
     },
 };
 
-- 
2.14.3

Re: [Qemu-devel] [PATCH 1/1] block/nbd: fix segmentation fault when .desc is not null-terminated

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1821 bytes --]

On 01/05/2018 07:32 AM, Murilo Opsfelder Araujo wrote:
> The find_desc_by_name() from util/qemu-option.c relies on the .name not being
> NULL to call strcmp(). This check becomes unsafe when the list is not
> NULL-terminated, which is the case of nbd_runtime_opts in block/nbd.c, and can
> result in segmentation fault when strcmp() tries to access an invalid memory:

Thanks for the report and patch.  Adding qemu-stable in cc.

> 
> This patch fixes the segmentation fault in strcmp() by adding a NULL element at
> the end of nbd_runtime_opts.desc list, which is the common practice to most of
> other structs like runtime_opts in block/null.c. Thus, the desc[i].name != NULL
> check becomes safe because it will not evaluate to true when .desc list reached
> its end.
> 
> Reported-by: R. Nageswara Sastry <nasastry@in.ibm.com>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1727259
> Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.vnet.ibm.com>

I'll update the commit message to add in the commit id that introduced
the problem, as well as check that other QemuOptsList do not have a
similar problem; I'm queueing this on the NBD tree and will submit a
pull request soon.

Reviewed-by: Eric Blake <eblake@redhat.com>

> ---
>  block/nbd.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index a50d24b50a..8b8ba56cdd 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -388,6 +388,7 @@ static QemuOptsList nbd_runtime_opts = {
>              .type = QEMU_OPT_STRING,
>              .help = "ID of the TLS credentials to use",
>          },
> +        { /* end of list */ }
>      },
>  };
>  
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 1/1] block/nbd: fix segmentation fault when .desc is not null-terminated

[Murilo Opsfelder Araújo]

On 01/05/2018 11:57 AM, Eric Blake wrote:
> On 01/05/2018 07:32 AM, Murilo Opsfelder Araujo wrote:
>> The find_desc_by_name() from util/qemu-option.c relies on the .name not being
>> NULL to call strcmp(). This check becomes unsafe when the list is not
>> NULL-terminated, which is the case of nbd_runtime_opts in block/nbd.c, and can
>> result in segmentation fault when strcmp() tries to access an invalid memory:
> 
> Thanks for the report and patch.  Adding qemu-stable in cc.
> 
>>
>> This patch fixes the segmentation fault in strcmp() by adding a NULL element at
>> the end of nbd_runtime_opts.desc list, which is the common practice to most of
>> other structs like runtime_opts in block/null.c. Thus, the desc[i].name != NULL
>> check becomes safe because it will not evaluate to true when .desc list reached
>> its end.
>>
>> Reported-by: R. Nageswara Sastry <nasastry@in.ibm.com>
>> Buglink: https://bugs.launchpad.net/qemu/+bug/1727259
>> Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.vnet.ibm.com>
> 
> I'll update the commit message to add in the commit id that introduced
> the problem, as well as check that other QemuOptsList do not have a
> similar problem; I'm queueing this on the NBD tree and will submit a
> pull request soon.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Hi, Eric.

A quick look brought my attention to:

block/ssh.c
530:static QemuOptsList ssh_runtime_opts = {

I've sent a patch to fix it too.

Thanks.
-- 
Murilo

Re: [Qemu-devel] [PATCH 1/1] block/nbd: fix segmentation fault when .desc is not null-terminated

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1326 bytes --]

On 01/05/2018 08:47 AM, Murilo Opsfelder Araújo wrote:

>>> This patch fixes the segmentation fault in strcmp() by adding a NULL element at
>>> the end of nbd_runtime_opts.desc list, which is the common practice to most of
>>> other structs like runtime_opts in block/null.c. Thus, the desc[i].name != NULL
>>> check becomes safe because it will not evaluate to true when .desc list reached
>>> its end.
>>>
>>> Reported-by: R. Nageswara Sastry <nasastry@in.ibm.com>
>>> Buglink: https://bugs.launchpad.net/qemu/+bug/1727259
>>> Signed-off-by: Murilo Opsfelder Araujo <muriloo@linux.vnet.ibm.com>
>>
>> I'll update the commit message to add in the commit id that introduced

Commit 7ccc44fd7, in 2.7.0.

>> the problem, as well as check that other QemuOptsList do not have a
>> similar problem; I'm queueing this on the NBD tree and will submit a
>> pull request soon.
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> Hi, Eric.
> 
> A quick look brought my attention to:
> 
> block/ssh.c
> 530:static QemuOptsList ssh_runtime_opts = {
> 
> I've sent a patch to fix it too.

And my audit matches yours that there were no other culprits besides
those two.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]