[PATCH for-6.2 0/4] Zero sockaddr_in when initializing it

[PATCH for-6.2 0/4] Zero sockaddr_in when initializing it

[Peter Maydell]

The POSIX spec for sockaddr_in says that implementations are allowed
to have implementation-dependent extensions controlled by extra
fields in the struct, and that the way to ensure these are not
accidentally activated is to zero out the whole data structure.
We have several places in our codebase where we don't zero-init
sockaddr_in structs and so (at least in theory) might run into this.
Coverity spotted the ones in the net code (CID 1005338); the
others in this series I found by looking at all uses of sockaddr_in.
(The gdbstub patch changes also a sockaddr_un use, for symmetry.)

Thanks to Eric for the analysis of what the spec says and why
Coverity is correct here.

thanks
-- PMM

Peter Maydell (4):
  net: Zero sockaddr_in in parse_host_port()
  gdbstub: Zero-initialize sockaddr structs
  tests/qtest/ipmi-bt-test: Zero-initialize sockaddr struct
  tests/tcg/multiarch/linux-test: Zero-initialize sockaddr structs

 gdbstub.c                        | 4 ++--
 net/net.c                        | 2 ++
 tests/qtest/ipmi-bt-test.c       | 2 +-
 tests/tcg/multiarch/linux-test.c | 4 ++--
 4 files changed, 7 insertions(+), 5 deletions(-)

-- 
2.20.1

[PATCH for-6.2 1/4] net: Zero sockaddr_in in parse_host_port()

[Peter Maydell]

We don't currently zero-initialize the 'struct sockaddr_in' that
parse_host_port() fills in, so any fields we don't explicitly
initialize might be left as random garbage.  POSIX states that
implementations may define extensions in sockaddr_in, and that those
extensions must not trigger if zero-initialized.  So not zero
initializing might result in inadvertently triggering an impdef
extension.

memset() the sockaddr_in before we start to fill it in.

Fixes: Coverity CID 1005338
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 net/net.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/net.c b/net/net.c
index 76bbb7c31b4..52c99196c69 100644
--- a/net/net.c
+++ b/net/net.c
@@ -75,6 +75,8 @@ int parse_host_port(struct sockaddr_in *saddr, const char *str,
     const char *addr, *p, *r;
     int port, ret = 0;
 
+    memset(saddr, 0, sizeof(*saddr));
+
     substrings = g_strsplit(str, ":", 2);
     if (!substrings || !substrings[0] || !substrings[1]) {
         error_setg(errp, "host address '%s' doesn't contain ':' "
-- 
2.20.1

[PATCH for-6.2 2/4] gdbstub: Zero-initialize sockaddr structs

[Peter Maydell]

Zero-initialize sockaddr_in and sockaddr_un structs that we're about
to fill in and pass to bind() or connect(), to ensure we don't leave
possible implementation-defined extension fields as uninitialized
garbage.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 gdbstub.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 52bde5bdc97..5d8e6ae3cd9 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -3218,7 +3218,7 @@ static bool gdb_accept_socket(int gdb_fd)
 
 static int gdbserver_open_socket(const char *path)
 {
-    struct sockaddr_un sockaddr;
+    struct sockaddr_un sockaddr = {};
     int fd, ret;
 
     fd = socket(AF_UNIX, SOCK_STREAM, 0);
@@ -3247,7 +3247,7 @@ static int gdbserver_open_socket(const char *path)
 
 static bool gdb_accept_tcp(int gdb_fd)
 {
-    struct sockaddr_in sockaddr;
+    struct sockaddr_in sockaddr = {};
     socklen_t len;
     int fd;
 
-- 
2.20.1

[PATCH for-6.2 3/4] tests/qtest/ipmi-bt-test: Zero-initialize sockaddr struct

[Peter Maydell]

Zero-initialize the sockaddr_in struct that we're about to fill in
and pass to bind(), to ensure we don't leave possible
implementation-defined extension fields as uninitialized garbage.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 tests/qtest/ipmi-bt-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/ipmi-bt-test.c b/tests/qtest/ipmi-bt-test.c
index 8492f02a9c3..19612e9405a 100644
--- a/tests/qtest/ipmi-bt-test.c
+++ b/tests/qtest/ipmi-bt-test.c
@@ -378,7 +378,7 @@ static void test_enable_irq(void)
  */
 static void open_socket(void)
 {
-    struct sockaddr_in myaddr;
+    struct sockaddr_in myaddr = {};
     socklen_t addrlen;
 
     myaddr.sin_family = AF_INET;
-- 
2.20.1

[PATCH for-6.2 4/4] tests/tcg/multiarch/linux-test: Zero-initialize sockaddr structs

[Peter Maydell]

Zero-initialize sockaddr_in and sockaddr_un structs that we're about
to fill in and pass to bind() or connect(), to ensure we don't leave
possible implementation-defined extension fields as uninitialized
garbage.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 tests/tcg/multiarch/linux-test.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/tcg/multiarch/linux-test.c b/tests/tcg/multiarch/linux-test.c
index c8c6aeddeb3..019d8175ca6 100644
--- a/tests/tcg/multiarch/linux-test.c
+++ b/tests/tcg/multiarch/linux-test.c
@@ -251,7 +251,7 @@ static void test_time(void)
 static int server_socket(void)
 {
     int val, fd;
-    struct sockaddr_in sockaddr;
+    struct sockaddr_in sockaddr = {};
 
     /* server socket */
     fd = chk_error(socket(PF_INET, SOCK_STREAM, 0));
@@ -271,7 +271,7 @@ static int server_socket(void)
 static int client_socket(uint16_t port)
 {
     int fd;
-    struct sockaddr_in sockaddr;
+    struct sockaddr_in sockaddr = {};
 
     /* server socket */
     fd = chk_error(socket(PF_INET, SOCK_STREAM, 0));
-- 
2.20.1

Re: [PATCH for-6.2 0/4] Zero sockaddr_in when initializing it

[Eric Blake]

On Fri, Aug 13, 2021 at 04:05:02PM +0100, Peter Maydell wrote:
> The POSIX spec for sockaddr_in says that implementations are allowed
> to have implementation-dependent extensions controlled by extra
> fields in the struct, and that the way to ensure these are not
> accidentally activated is to zero out the whole data structure.
> We have several places in our codebase where we don't zero-init
> sockaddr_in structs and so (at least in theory) might run into this.
> Coverity spotted the ones in the net code (CID 1005338); the
> others in this series I found by looking at all uses of sockaddr_in.
> (The gdbstub patch changes also a sockaddr_un use, for symmetry.)
> 
> Thanks to Eric for the analysis of what the spec says and why
> Coverity is correct here.

FWIW, the POSIX wording is interesting - it requires portable
applications to zero out sockaddr_in6 (and even states that memset()
is not yet a portable way to do that on exotic hardware, although a
future version of POSIX may add a zero-bit constraint on
implementations; in practice we only use qemu on hardware where
memset() to zero properly sets pointers to NULL and floating points to
0.0).  But for sockaddr_in, it merely recommends it, with an
acknowledgment that much existing code fails to do so.  Or put another
way, POSIX gives carte blanche to implementations to add IPv6
extensions, but advises that IPv4 implementations should be wary of
extensions that trigger off of uninitialized fields.

Since you are fixing IPv4 usage, and not IPv6, I agree with your
designation that this is 6.2 material, and not a regression fix to
rush into 6.1 (should other patches warrant rc4) - we are unlikely to
be running on an implementation where the uninitialized fields cause
noticeable behavior changes to IPv4 behavior.

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

Re: [PATCH for-6.2 1/4] net: Zero sockaddr_in in parse_host_port()

[Eric Blake]

On Fri, Aug 13, 2021 at 04:05:03PM +0100, Peter Maydell wrote:
> We don't currently zero-initialize the 'struct sockaddr_in' that
> parse_host_port() fills in, so any fields we don't explicitly
> initialize might be left as random garbage.  POSIX states that
> implementations may define extensions in sockaddr_in, and that those
> extensions must not trigger if zero-initialized.  So not zero
> initializing might result in inadvertently triggering an impdef
> extension.
> 
> memset() the sockaddr_in before we start to fill it in.

Technically, POSIX recommends default initialization, as in:

struct sockaddr_in sa = { 0 };
or:
static struct sockaddr_in sa_init;
struct sockaddr_in sa = sa_init;

because of odd platforms where default initialization compiles to
non-zero bits (think platforms where NULL and/or floating point 0.0 do
not have an all-zero-bit representation - yes, C is weird).  But in
practice, that does not plague any of the hardware qemu cares about,
so I'm just fine with memset.

> 
> Fixes: Coverity CID 1005338
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  net/net.c | 2 ++
>  1 file changed, 2 insertions(+)

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

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

Re: [PATCH for-6.2 2/4] gdbstub: Zero-initialize sockaddr structs

[Eric Blake]

On Fri, Aug 13, 2021 at 04:05:04PM +0100, Peter Maydell wrote:
> Zero-initialize sockaddr_in and sockaddr_un structs that we're about
> to fill in and pass to bind() or connect(), to ensure we don't leave
> possible implementation-defined extension fields as uninitialized
> garbage.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  gdbstub.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index 52bde5bdc97..5d8e6ae3cd9 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -3218,7 +3218,7 @@ static bool gdb_accept_socket(int gdb_fd)
>  
>  static int gdbserver_open_socket(const char *path)
>  {
> -    struct sockaddr_un sockaddr;
> +    struct sockaddr_un sockaddr = {};

I know we use this non-standard form (which both gcc and clang accept)
because at least older versions of clang needlessly warn on the
standard C construct {0} in some situations, but figuring out when we
no longer support those older compilers and converting our code-base
to look more like standardized code is not my priority.

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

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

Re: [PATCH for-6.2 3/4] tests/qtest/ipmi-bt-test: Zero-initialize sockaddr struct

[Eric Blake]

On Fri, Aug 13, 2021 at 04:05:05PM +0100, Peter Maydell wrote:
> Zero-initialize the sockaddr_in struct that we're about to fill in
> and pass to bind(), to ensure we don't leave possible
> implementation-defined extension fields as uninitialized garbage.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  tests/qtest/ipmi-bt-test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

> 
> diff --git a/tests/qtest/ipmi-bt-test.c b/tests/qtest/ipmi-bt-test.c
> index 8492f02a9c3..19612e9405a 100644
> --- a/tests/qtest/ipmi-bt-test.c
> +++ b/tests/qtest/ipmi-bt-test.c
> @@ -378,7 +378,7 @@ static void test_enable_irq(void)
>   */
>  static void open_socket(void)
>  {
> -    struct sockaddr_in myaddr;
> +    struct sockaddr_in myaddr = {};
>      socklen_t addrlen;
>  
>      myaddr.sin_family = AF_INET;
> -- 
> 2.20.1
> 

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

Re: [PATCH for-6.2 4/4] tests/tcg/multiarch/linux-test: Zero-initialize sockaddr structs

[Eric Blake]

On Fri, Aug 13, 2021 at 04:05:06PM +0100, Peter Maydell wrote:
> Zero-initialize sockaddr_in and sockaddr_un structs that we're about
> to fill in and pass to bind() or connect(), to ensure we don't leave
> possible implementation-defined extension fields as uninitialized
> garbage.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  tests/tcg/multiarch/linux-test.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

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

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

Re: [PATCH for-6.2 3/4] tests/qtest/ipmi-bt-test: Zero-initialize sockaddr struct

[Thomas Huth]

On 13/08/2021 17.05, Peter Maydell wrote:
> Zero-initialize the sockaddr_in struct that we're about to fill in
> and pass to bind(), to ensure we don't leave possible
> implementation-defined extension fields as uninitialized garbage.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   tests/qtest/ipmi-bt-test.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/ipmi-bt-test.c b/tests/qtest/ipmi-bt-test.c
> index 8492f02a9c3..19612e9405a 100644
> --- a/tests/qtest/ipmi-bt-test.c
> +++ b/tests/qtest/ipmi-bt-test.c
> @@ -378,7 +378,7 @@ static void test_enable_irq(void)
>    */
>   static void open_socket(void)
>   {
> -    struct sockaddr_in myaddr;
> +    struct sockaddr_in myaddr = {};
>       socklen_t addrlen;
>   
>       myaddr.sin_family = AF_INET;
> 

Acked-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH for-6.2 3/4] tests/qtest/ipmi-bt-test: Zero-initialize sockaddr struct

[Corey Minyard]

On Fri, Aug 13, 2021 at 04:05:05PM +0100, Peter Maydell wrote:
> Zero-initialize the sockaddr_in struct that we're about to fill in
> and pass to bind(), to ensure we don't leave possible
> implementation-defined extension fields as uninitialized garbage.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Looks good to me.  Thanks, Peter.  I looked through all the other
patches, too, and reviewed those.

Reviewed-by: Corey Minyard <cminyard@mvista.com>

> ---
>  tests/qtest/ipmi-bt-test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/ipmi-bt-test.c b/tests/qtest/ipmi-bt-test.c
> index 8492f02a9c3..19612e9405a 100644
> --- a/tests/qtest/ipmi-bt-test.c
> +++ b/tests/qtest/ipmi-bt-test.c
> @@ -378,7 +378,7 @@ static void test_enable_irq(void)
>   */
>  static void open_socket(void)
>  {
> -    struct sockaddr_in myaddr;
> +    struct sockaddr_in myaddr = {};
>      socklen_t addrlen;
>  
>      myaddr.sin_family = AF_INET;
> -- 
> 2.20.1
> 

Re: [PATCH for-6.2 0/4] Zero sockaddr_in when initializing it

[Philippe Mathieu-Daudé]

On 8/13/21 8:30 PM, Eric Blake wrote:
> On Fri, Aug 13, 2021 at 04:05:02PM +0100, Peter Maydell wrote:
>> The POSIX spec for sockaddr_in says that implementations are allowed
>> to have implementation-dependent extensions controlled by extra
>> fields in the struct, and that the way to ensure these are not
>> accidentally activated is to zero out the whole data structure.
>> We have several places in our codebase where we don't zero-init
>> sockaddr_in structs and so (at least in theory) might run into this.
>> Coverity spotted the ones in the net code (CID 1005338); the
>> others in this series I found by looking at all uses of sockaddr_in.
>> (The gdbstub patch changes also a sockaddr_un use, for symmetry.)
>>
>> Thanks to Eric for the analysis of what the spec says and why
>> Coverity is correct here.
> 
> FWIW, the POSIX wording is interesting - it requires portable
> applications to zero out sockaddr_in6 (and even states that memset()
> is not yet a portable way to do that on exotic hardware, although a
> future version of POSIX may add a zero-bit constraint on
> implementations; in practice we only use qemu on hardware where
> memset() to zero properly sets pointers to NULL and floating points to
> 0.0).

So this checkpatch.pl error (inherited from Linux) is against POSIX?

2028 # check for static initialisers.
2029         if ($line =~ /\bstatic\s.*=\s*(0|NULL|false)\s*;/) {
2030             ERROR("do not initialise statics to 0 or NULL\n" .
2031                 $herecurr);
2032         }

[...]

Re: [PATCH for-6.2 0/4] Zero sockaddr_in when initializing it

[Peter Maydell]

On Sun, 15 Aug 2021 at 15:34, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> On 8/13/21 8:30 PM, Eric Blake wrote:
> > FWIW, the POSIX wording is interesting - it requires portable
> > applications to zero out sockaddr_in6 (and even states that memset()
> > is not yet a portable way to do that on exotic hardware, although a
> > future version of POSIX may add a zero-bit constraint on
> > implementations; in practice we only use qemu on hardware where
> > memset() to zero properly sets pointers to NULL and floating points to
> > 0.0).
>
> So this checkpatch.pl error (inherited from Linux) is against POSIX?
>
> 2028 # check for static initialisers.
> 2029         if ($line =~ /\bstatic\s.*=\s*(0|NULL|false)\s*;/) {
> 2030             ERROR("do not initialise statics to 0 or NULL\n" .
> 2031                 $herecurr);
> 2032         }

That one is for statics, where the C spec says you get 0-init by
default and so there's no need to explicitly 0-init.

-- PMM

Re: [PATCH for-6.2 0/4] Zero sockaddr_in when initializing it

[Philippe Mathieu-Daudé]

On 8/15/21 5:44 PM, Peter Maydell wrote:
> On Sun, 15 Aug 2021 at 15:34, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> On 8/13/21 8:30 PM, Eric Blake wrote:
>>> FWIW, the POSIX wording is interesting - it requires portable
>>> applications to zero out sockaddr_in6 (and even states that memset()
>>> is not yet a portable way to do that on exotic hardware, although a
>>> future version of POSIX may add a zero-bit constraint on
>>> implementations; in practice we only use qemu on hardware where
>>> memset() to zero properly sets pointers to NULL and floating points to
>>> 0.0).
>>
>> So this checkpatch.pl error (inherited from Linux) is against POSIX?
>>
>> 2028 # check for static initialisers.
>> 2029         if ($line =~ /\bstatic\s.*=\s*(0|NULL|false)\s*;/) {
>> 2030             ERROR("do not initialise statics to 0 or NULL\n" .
>> 2031                 $herecurr);
>> 2032         }
> 
> That one is for statics, where the C spec says you get 0-init by
> default and so there's no need to explicitly 0-init.

Ah OK, thanks :)

Re: [PATCH for-6.2 0/4] Zero sockaddr_in when initializing it

[Peter Maydell]

On Fri, 13 Aug 2021 at 16:05, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> The POSIX spec for sockaddr_in says that implementations are allowed
> to have implementation-dependent extensions controlled by extra
> fields in the struct, and that the way to ensure these are not
> accidentally activated is to zero out the whole data structure.
> We have several places in our codebase where we don't zero-init
> sockaddr_in structs and so (at least in theory) might run into this.
> Coverity spotted the ones in the net code (CID 1005338); the
> others in this series I found by looking at all uses of sockaddr_in.
> (The gdbstub patch changes also a sockaddr_un use, for symmetry.)
>
> Thanks to Eric for the analysis of what the spec says and why
> Coverity is correct here.
>
> thanks
> -- PMM
>
> Peter Maydell (4):
>   net: Zero sockaddr_in in parse_host_port()
>   gdbstub: Zero-initialize sockaddr structs
>   tests/qtest/ipmi-bt-test: Zero-initialize sockaddr struct
>   tests/tcg/multiarch/linux-test: Zero-initialize sockaddr structs

I'll take this series via target-arm.next unless anybody objects.

thanks
-- PMM