qemu: hotplug: Don't access srcPriv when it's not allocated

[libvirt] [PATCH] qemu: hotplug: Don't access srcPriv when it's not allocated

Posted by Peter Krempa 5 years, 9 months ago

The private data of a virStorageSource which is backing an iSCSI hostdev
may be NULL if no authentication is present. The code handling the
hotplug would attempt to extract the authentication info stored in
'secinfo' without checking if it is allocated which resulted in a crash.

Here we opt the easy way to check if srcPriv is not NULL so that we
don't duplicate all the logic which selects whether the disk source has
a secret.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1597550

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/qemu/qemu_hotplug.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index fcd8eb0ffa..075f2fb72e 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2240,7 +2240,8 @@ qemuDomainAttachHostSCSIDevice(virQEMUDriverPtr driver,
     if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) {
         qemuDomainStorageSourcePrivatePtr srcPriv =
             QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(scsisrc->u.iscsi.src);
-        secinfo = srcPriv->secinfo;
+        if (srcPriv)
+            secinfo = srcPriv->secinfo;
     }

     if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) {
-- 
2.16.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: hotplug: Don't access srcPriv when it's not allocated

Posted by John Ferlan 5 years, 9 months ago


On 07/03/2018 08:05 AM, Peter Krempa wrote:
> The private data of a virStorageSource which is backing an iSCSI hostdev
> may be NULL if no authentication is present. The code handling the
> hotplug would attempt to extract the authentication info stored in
> 'secinfo' without checking if it is allocated which resulted in a crash.
> 
> Here we opt the easy way to check if srcPriv is not NULL so that we
> don't duplicate all the logic which selects whether the disk source has
> a secret.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1597550
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/qemu/qemu_hotplug.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

Feels like we've done this before...

97202988 <sigh> and 6050affb7f

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: hotplug: Don't access srcPriv when it's not allocated

Posted by Peter Krempa 5 years, 9 months ago

On Tue, Jul 03, 2018 at 09:34:56 -0400, John Ferlan wrote:
> 
> 
> On 07/03/2018 08:05 AM, Peter Krempa wrote:
> > The private data of a virStorageSource which is backing an iSCSI hostdev
> > may be NULL if no authentication is present. The code handling the
> > hotplug would attempt to extract the authentication info stored in
> > 'secinfo' without checking if it is allocated which resulted in a crash.
> > 
> > Here we opt the easy way to check if srcPriv is not NULL so that we
> > don't duplicate all the logic which selects whether the disk source has
> > a secret.
> > 
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1597550
> > 
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> >  src/qemu/qemu_hotplug.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> 
> Reviewed-by: John Ferlan <jferlan@redhat.com>
> 
> John
> 
> Feels like we've done this before...
> 
> 97202988 <sigh> and 6050affb7f

Specifically the former commit was not good enough to cover this case
despite touching this code.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: hotplug: Don't access srcPriv when it's not allocated

Posted by Marc Hartmayer 5 years, 9 months ago

On Tue, Jul 03, 2018 at 04:22 PM +0200, Peter Krempa <pkrempa@redhat.com> wrote:
> On Tue, Jul 03, 2018 at 09:34:56 -0400, John Ferlan wrote:
>>
>>
>> On 07/03/2018 08:05 AM, Peter Krempa wrote:
>> > The private data of a virStorageSource which is backing an iSCSI hostdev
>> > may be NULL if no authentication is present. The code handling the
>> > hotplug would attempt to extract the authentication info stored in
>> > 'secinfo' without checking if it is allocated which resulted in a crash.
>> >
>> > Here we opt the easy way to check if srcPriv is not NULL so that we
>> > don't duplicate all the logic which selects whether the disk source has
>> > a secret.
>> >
>> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1597550
>> >
>> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>> > ---
>> >  src/qemu/qemu_hotplug.c | 3 ++-
>> >  1 file changed, 2 insertions(+), 1 deletion(-)
>> >
>>
>> Reviewed-by: John Ferlan <jferlan@redhat.com>
>>
>> John
>>
>> Feels like we've done this before...
>>
>> 97202988 <sigh> and 6050affb7f
>
> Specifically the former commit was not good enough to cover this case
> despite touching this code.

Would it make sense to always allocate the
qemuDomainStorageSourcePrivate struct? We’d be wasting some memory, but
at the same time the code would be easier to read and less prone to
error. e.g. qemuDomainDeviceDiskDefPostParseRestoreSecAlias would be
simpler.

For the privateData of virConnect, virDomainVsockDef, virDisk,
virDomainVcpu… it’s implemented this way.

Anyway: Reviewed-by: Marc Hartmayer <mhartmay@linux.ibm.com> and thanks
for fixing it!

>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
Beste Grüße / Kind regards
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: hotplug: Don't access srcPriv when it's not allocated

Posted by Peter Krempa 5 years, 9 months ago

On Tue, Jul 03, 2018 at 17:05:31 +0200, Marc Hartmayer wrote:
> On Tue, Jul 03, 2018 at 04:22 PM +0200, Peter Krempa <pkrempa@redhat.com> wrote:
> > On Tue, Jul 03, 2018 at 09:34:56 -0400, John Ferlan wrote:
> >> Feels like we've done this before...
> >>
> >> 97202988 <sigh> and 6050affb7f
> >
> > Specifically the former commit was not good enough to cover this case
> > despite touching this code.
> 
> Would it make sense to always allocate the
> qemuDomainStorageSourcePrivate struct? We’d be wasting some memory, but
> at the same time the code would be easier to read and less prone to
> error. e.g. qemuDomainDeviceDiskDefPostParseRestoreSecAlias would be
> simpler.
> 
> For the privateData of virConnect, virDomainVsockDef, virDisk,
> virDomainVcpu… it’s implemented this way.
> 
> Anyway: Reviewed-by: Marc Hartmayer <mhartmay@linux.ibm.com> and thanks
> for fixing it!

virStorageSource is not allocated using a helper so this would require
refactoring all allocations and passing in of the private data structure
which contains the callback to allocate the private data. I was thinking
about it but did not find fixing it compelling enough to do it myself.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list