| Message ID | 20180105133241.14141-2-muriloo@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 */ } > }, > }; > >
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.
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.
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 */ } }, };
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(+)