[PATCH] virsocketaddr: Zero @netmask in virSocketAddrPrefixToNetmask()

Michal Privoznik posted 1 patch 2 weeks ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/796573c57a88bc05ef360c86c18cda20fea14786.1602254587.git.mprivozn@redhat.com
src/util/virsocketaddr.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

[PATCH] virsocketaddr: Zero @netmask in virSocketAddrPrefixToNetmask()

Posted by Michal Privoznik 2 weeks ago
The aim of virSocketAddrPrefixToNetmask() is to initialize passed
virSocketAddr structure based on prefix length and family.
However, it doesn't set all members in the struct which may lead
to reads of uninitialized values:

==15421== Use of uninitialised value of size 8
==15421==    at 0x50F297A: _itoa_word (in /lib64/libc-2.31.so)
==15421==    by 0x510C8FE: __vfprintf_internal (in /lib64/libc-2.31.so)
==15421==    by 0x5120295: __vsnprintf_internal (in /lib64/libc-2.31.so)
==15421==    by 0x50F8969: snprintf (in /lib64/libc-2.31.so)
==15421==    by 0x51BB602: getnameinfo (in /lib64/libc-2.31.so)
==15421==    by 0x496DEE0: virSocketAddrFormatFull (virsocketaddr.c:486)
==15421==    by 0x496DD9F: virSocketAddrFormat (virsocketaddr.c:444)
==15421==    by 0x11871F: networkDnsmasqConfContents (bridge_driver.c:1404)
==15421==    by 0x1118F5: testCompareXMLToConfFiles (networkxml2conftest.c:48)
==15421==    by 0x111BAF: testCompareXMLToConfHelper (networkxml2conftest.c:112)
==15421==    by 0x112679: virTestRun (testutils.c:142)
==15421==    by 0x111D09: mymain (networkxml2conftest.c:144)
==15421==  Uninitialised value was created by a stack allocation
==15421==    at 0x1175D2: networkDnsmasqConfContents (bridge_driver.c:1056)

All callers expect the function to initialize the structure
fully.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/util/virsocketaddr.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c
index e0eb76ded3..65aaa632c7 100644
--- a/src/util/virsocketaddr.c
+++ b/src/util/virsocketaddr.c
@@ -1097,6 +1097,8 @@ virSocketAddrPrefixToNetmask(unsigned int prefix,
                              virSocketAddrPtr netmask,
                              int family)
 {
+    memset(netmask, 0, sizeof(*netmask));
+
     netmask->data.stor.ss_family = AF_UNSPEC; /* assume failure */
 
     if (family == AF_INET) {
@@ -1135,7 +1137,7 @@ virSocketAddrPrefixToNetmask(unsigned int prefix,
     }
 
     return 0;
- }
+}
 
 /**
  * virSocketAddrGetIPPrefix:
-- 
2.26.2

Re: [PATCH] virsocketaddr: Zero @netmask in virSocketAddrPrefixToNetmask()

Posted by Laine Stump 2 weeks ago
On 10/9/20 10:43 AM, Michal Privoznik wrote:
> The aim of virSocketAddrPrefixToNetmask() is to initialize passed
> virSocketAddr structure based on prefix length and family.
> However, it doesn't set all members in the struct which may lead
> to reads of uninitialized values:
>
> ==15421== Use of uninitialised value of size 8
> ==15421==    at 0x50F297A: _itoa_word (in /lib64/libc-2.31.so)
> ==15421==    by 0x510C8FE: __vfprintf_internal (in /lib64/libc-2.31.so)
> ==15421==    by 0x5120295: __vsnprintf_internal (in /lib64/libc-2.31.so)
> ==15421==    by 0x50F8969: snprintf (in /lib64/libc-2.31.so)
> ==15421==    by 0x51BB602: getnameinfo (in /lib64/libc-2.31.so)
> ==15421==    by 0x496DEE0: virSocketAddrFormatFull (virsocketaddr.c:486)
> ==15421==    by 0x496DD9F: virSocketAddrFormat (virsocketaddr.c:444)
> ==15421==    by 0x11871F: networkDnsmasqConfContents (bridge_driver.c:1404)
> ==15421==    by 0x1118F5: testCompareXMLToConfFiles (networkxml2conftest.c:48)
> ==15421==    by 0x111BAF: testCompareXMLToConfHelper (networkxml2conftest.c:112)
> ==15421==    by 0x112679: virTestRun (testutils.c:142)
> ==15421==    by 0x111D09: mymain (networkxml2conftest.c:144)
> ==15421==  Uninitialised value was created by a stack allocation
> ==15421==    at 0x1175D2: networkDnsmasqConfContents (bridge_driver.c:1056)
>
> All callers expect the function to initialize the structure
> fully.
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>


Reviewed-by: Laine Stump <laine@redhat.com>


Did you see actual errors caused by this, or just the valgrind 
complaints? It's been like this since the function was first implemented 
in 2010 (commit 1ab80f32. Yes, it was me. Sigh.) I wonder why nobody 
ever saw this valgrind complaint before. Some check that was newly added?


(unless you've seen a real-world error, my guess would be that the 
fields that aren't being initialized are actually ignored when the 
family is set to INET or INET6, but even then it's still safer to clear 
everything out.)



> ---
>   src/util/virsocketaddr.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c
> index e0eb76ded3..65aaa632c7 100644
> --- a/src/util/virsocketaddr.c
> +++ b/src/util/virsocketaddr.c
> @@ -1097,6 +1097,8 @@ virSocketAddrPrefixToNetmask(unsigned int prefix,
>                                virSocketAddrPtr netmask,
>                                int family)
>   {
> +    memset(netmask, 0, sizeof(*netmask));
> +
>       netmask->data.stor.ss_family = AF_UNSPEC; /* assume failure */
>   
>       if (family == AF_INET) {
> @@ -1135,7 +1137,7 @@ virSocketAddrPrefixToNetmask(unsigned int prefix,
>       }
>   
>       return 0;
> - }
> +}
>   
>   /**
>    * virSocketAddrGetIPPrefix:


Re: [PATCH] virsocketaddr: Zero @netmask in virSocketAddrPrefixToNetmask()

Posted by Michal Privoznik 2 weeks ago
On 10/10/20 6:39 PM, Laine Stump wrote:
> On 10/9/20 10:43 AM, Michal Privoznik wrote:
>> The aim of virSocketAddrPrefixToNetmask() is to initialize passed
>> virSocketAddr structure based on prefix length and family.
>> However, it doesn't set all members in the struct which may lead
>> to reads of uninitialized values:
>>
>> ==15421== Use of uninitialised value of size 8
>> ==15421==    at 0x50F297A: _itoa_word (in /lib64/libc-2.31.so)
>> ==15421==    by 0x510C8FE: __vfprintf_internal (in /lib64/libc-2.31.so)
>> ==15421==    by 0x5120295: __vsnprintf_internal (in /lib64/libc-2.31.so)
>> ==15421==    by 0x50F8969: snprintf (in /lib64/libc-2.31.so)
>> ==15421==    by 0x51BB602: getnameinfo (in /lib64/libc-2.31.so)
>> ==15421==    by 0x496DEE0: virSocketAddrFormatFull (virsocketaddr.c:486)
>> ==15421==    by 0x496DD9F: virSocketAddrFormat (virsocketaddr.c:444)
>> ==15421==    by 0x11871F: networkDnsmasqConfContents 
>> (bridge_driver.c:1404)
>> ==15421==    by 0x1118F5: testCompareXMLToConfFiles 
>> (networkxml2conftest.c:48)
>> ==15421==    by 0x111BAF: testCompareXMLToConfHelper 
>> (networkxml2conftest.c:112)
>> ==15421==    by 0x112679: virTestRun (testutils.c:142)
>> ==15421==    by 0x111D09: mymain (networkxml2conftest.c:144)
>> ==15421==  Uninitialised value was created by a stack allocation
>> ==15421==    at 0x1175D2: networkDnsmasqConfContents 
>> (bridge_driver.c:1056)
>>
>> All callers expect the function to initialize the structure
>> fully.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> 
> 
> Reviewed-by: Laine Stump <laine@redhat.com>
> 
> 
> Did you see actual errors caused by this, or just the valgrind 
> complaints? It's been like this since the function was first implemented 
> in 2010 (commit 1ab80f32. Yes, it was me. Sigh.) I wonder why nobody 
> ever saw this valgrind complaint before. Some check that was newly added?
> 
> 
> (unless you've seen a real-world error, my guess would be that the 
> fields that aren't being initialized are actually ignored when the 
> family is set to INET or INET6, but even then it's still safer to clear 
> everything out.)

Yeah, that's probably what is happening. I have not seen any real world 
error, but I'm writing some patches that touch network and ran the test 
under valgrind only to find the "bug".

Pushed, thanks.

Michal