[PATCH] util: fix abstract socket path copy

[PATCH] util: fix abstract socket path copy

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Commit 776b97d360 "qemu-sockets: add abstract UNIX domain socket
support" neglected to update socket_sockaddr_to_address_unix() and
copied the whole sun_path without taking "salen" into account.

Later, commit 3b14b4ec49 "sockets: Fix socket_sockaddr_to_address_unix()
for abstract sockets" handled the abstract UNIX path, by stripping the
leading \0 character and fixing address details, but didn't use salen
either.

Not taking "salen" into account may result in incorrect "path" being
returned in monitors commands, as we read past the address which is not
necessarily \0-terminated.

Fixes: 776b97d3605ed0fc94443048fdf988c7725e38a9
Fixes: 3b14b4ec49a801067da19d6b8469eb1c1911c020
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 util/qemu-sockets.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 080a240b74..f2f3676d1f 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -1345,13 +1345,16 @@ socket_sockaddr_to_address_unix(struct sockaddr_storage *sa,
     SocketAddress *addr;
     struct sockaddr_un *su = (struct sockaddr_un *)sa;
 
+    assert(salen >= sizeof(su->sun_family) + 1 &&
+           salen <= sizeof(struct sockaddr_un));
+
     addr = g_new0(SocketAddress, 1);
     addr->type = SOCKET_ADDRESS_TYPE_UNIX;
 #ifdef CONFIG_LINUX
     if (!su->sun_path[0]) {
         /* Linux abstract socket */
         addr->u.q_unix.path = g_strndup(su->sun_path + 1,
-                                        sizeof(su->sun_path) - 1);
+                                        salen - sizeof(su->sun_family) - 1);
         addr->u.q_unix.has_abstract = true;
         addr->u.q_unix.abstract = true;
         addr->u.q_unix.has_tight = true;
-- 
2.32.0.264.g75ae10bc75

Re: [PATCH] util: fix abstract socket path copy

[Daniel P. Berrangé]

On Mon, Jul 19, 2021 at 05:01:12PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Commit 776b97d360 "qemu-sockets: add abstract UNIX domain socket
> support" neglected to update socket_sockaddr_to_address_unix() and
> copied the whole sun_path without taking "salen" into account.
> 
> Later, commit 3b14b4ec49 "sockets: Fix socket_sockaddr_to_address_unix()
> for abstract sockets" handled the abstract UNIX path, by stripping the
> leading \0 character and fixing address details, but didn't use salen
> either.
> 
> Not taking "salen" into account may result in incorrect "path" being
> returned in monitors commands, as we read past the address which is not
> necessarily \0-terminated.

So IIUC, this is only affecting what is printed in the monitor
when querying chardevs, not the actual functional behaviour
between clients/servers connecting/listening ?

> 
> Fixes: 776b97d3605ed0fc94443048fdf988c7725e38a9
> Fixes: 3b14b4ec49a801067da19d6b8469eb1c1911c020
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  util/qemu-sockets.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 080a240b74..f2f3676d1f 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1345,13 +1345,16 @@ socket_sockaddr_to_address_unix(struct sockaddr_storage *sa,
>      SocketAddress *addr;
>      struct sockaddr_un *su = (struct sockaddr_un *)sa;
>  
> +    assert(salen >= sizeof(su->sun_family) + 1 &&
> +           salen <= sizeof(struct sockaddr_un));
> +
>      addr = g_new0(SocketAddress, 1);
>      addr->type = SOCKET_ADDRESS_TYPE_UNIX;
>  #ifdef CONFIG_LINUX
>      if (!su->sun_path[0]) {
>          /* Linux abstract socket */
>          addr->u.q_unix.path = g_strndup(su->sun_path + 1,
> -                                        sizeof(su->sun_path) - 1);
> +                                        salen - sizeof(su->sun_family) - 1);
>          addr->u.q_unix.has_abstract = true;
>          addr->u.q_unix.abstract = true;
>          addr->u.q_unix.has_tight = true;

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] util: fix abstract socket path copy

[Marc-André Lureau]

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

On Mon, Jul 19, 2021 at 5:49 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Mon, Jul 19, 2021 at 05:01:12PM +0400, marcandre.lureau@redhat.com
> wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Commit 776b97d360 "qemu-sockets: add abstract UNIX domain socket
> > support" neglected to update socket_sockaddr_to_address_unix() and
> > copied the whole sun_path without taking "salen" into account.
> >
> > Later, commit 3b14b4ec49 "sockets: Fix socket_sockaddr_to_address_unix()
> > for abstract sockets" handled the abstract UNIX path, by stripping the
> > leading \0 character and fixing address details, but didn't use salen
> > either.
> >
> > Not taking "salen" into account may result in incorrect "path" being
> > returned in monitors commands, as we read past the address which is not
> > necessarily \0-terminated.
>
> So IIUC, this is only affecting what is printed in the monitor
> when querying chardevs, not the actual functional behaviour
> between clients/servers connecting/listening ?
>
>
I think so, both hmp & qmp, and the info_report() in server_accept_sync().
But I didn't carefully review all the potential users (who else could they
be?).


> >
> > Fixes: 776b97d3605ed0fc94443048fdf988c7725e38a9
> > Fixes: 3b14b4ec49a801067da19d6b8469eb1c1911c020
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  util/qemu-sockets.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> > index 080a240b74..f2f3676d1f 100644
> > --- a/util/qemu-sockets.c
> > +++ b/util/qemu-sockets.c
> > @@ -1345,13 +1345,16 @@ socket_sockaddr_to_address_unix(struct
> sockaddr_storage *sa,
> >      SocketAddress *addr;
> >      struct sockaddr_un *su = (struct sockaddr_un *)sa;
> >
> > +    assert(salen >= sizeof(su->sun_family) + 1 &&
> > +           salen <= sizeof(struct sockaddr_un));
> > +
> >      addr = g_new0(SocketAddress, 1);
> >      addr->type = SOCKET_ADDRESS_TYPE_UNIX;
> >  #ifdef CONFIG_LINUX
> >      if (!su->sun_path[0]) {
> >          /* Linux abstract socket */
> >          addr->u.q_unix.path = g_strndup(su->sun_path + 1,
> > -                                        sizeof(su->sun_path) - 1);
> > +                                        salen - sizeof(su->sun_family)
> - 1);
> >          addr->u.q_unix.has_abstract = true;
> >          addr->u.q_unix.abstract = true;
> >          addr->u.q_unix.has_tight = true;
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>
>
> Regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>
>

[-- Attachment #2: Type: text/html, Size: 4559 bytes --]

Re: [PATCH] util: fix abstract socket path copy

[zhao xiao qiang]

在 2021/7/19 21:01, marcandre.lureau@redhat.com 写道:
> Commit 776b97d360 "qemu-sockets: add abstract UNIX domain socket
> support" neglected to update socket_sockaddr_to_address_unix() and
> copied the whole sun_path without taking "salen" into account.
> 
> Later, commit 3b14b4ec49 "sockets: Fix socket_sockaddr_to_address_unix()
> for abstract sockets" handled the abstract UNIX path, by stripping the
> leading \0 character and fixing address details, but didn't use salen
> either.
> 
> Not taking "salen" into account may result in incorrect "path" being
> returned in monitors commands, as we read past the address which is not
> necessarily \0-terminated.
> 
> Fixes: 776b97d3605ed0fc94443048fdf988c7725e38a9
> Fixes: 3b14b4ec49a801067da19d6b8469eb1c1911c020
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  util/qemu-sockets.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 080a240b74..f2f3676d1f 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1345,13 +1345,16 @@ socket_sockaddr_to_address_unix(struct sockaddr_storage *sa,
>      SocketAddress *addr;
>      struct sockaddr_un *su = (struct sockaddr_un *)sa;
>  
> +    assert(salen >= sizeof(su->sun_family) + 1 &&
> +           salen <= sizeof(struct sockaddr_un));
> +
>      addr = g_new0(SocketAddress, 1);
>      addr->type = SOCKET_ADDRESS_TYPE_UNIX;
>  #ifdef CONFIG_LINUX
>      if (!su->sun_path[0]) {
>          /* Linux abstract socket */
>          addr->u.q_unix.path = g_strndup(su->sun_path + 1,
> -                                        sizeof(su->sun_path) - 1);
> +                                        salen - sizeof(su->sun_family) - 1);
>          addr->u.q_unix.has_abstract = true;
>          addr->u.q_unix.abstract = true;
>          addr->u.q_unix.has_tight = true;


Reviewed-by: xiaoqiang zhao <zxq_yx_007@163.com>

Re: [PATCH] util: fix abstract socket path copy

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Commit 776b97d360 "qemu-sockets: add abstract UNIX domain socket
> support" neglected to update socket_sockaddr_to_address_unix() and
> copied the whole sun_path without taking "salen" into account.
>
> Later, commit 3b14b4ec49 "sockets: Fix socket_sockaddr_to_address_unix()
> for abstract sockets" handled the abstract UNIX path, by stripping the
> leading \0 character and fixing address details, but didn't use salen
> either.
>
> Not taking "salen" into account may result in incorrect "path" being
> returned in monitors commands, as we read past the address which is not
> necessarily \0-terminated.
>
> Fixes: 776b97d3605ed0fc94443048fdf988c7725e38a9
> Fixes: 3b14b4ec49a801067da19d6b8469eb1c1911c020
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  util/qemu-sockets.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 080a240b74..f2f3676d1f 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1345,13 +1345,16 @@ socket_sockaddr_to_address_unix(struct sockaddr_storage *sa,
>      SocketAddress *addr;
>      struct sockaddr_un *su = (struct sockaddr_un *)sa;
>  
> +    assert(salen >= sizeof(su->sun_family) + 1 &&
> +           salen <= sizeof(struct sockaddr_un));
> +
>      addr = g_new0(SocketAddress, 1);
>      addr->type = SOCKET_ADDRESS_TYPE_UNIX;
>  #ifdef CONFIG_LINUX
>      if (!su->sun_path[0]) {
>          /* Linux abstract socket */
>          addr->u.q_unix.path = g_strndup(su->sun_path + 1,
> -                                        sizeof(su->sun_path) - 1);
> +                                        salen - sizeof(su->sun_family) - 1);
>          addr->u.q_unix.has_abstract = true;
>          addr->u.q_unix.abstract = true;
>          addr->u.q_unix.has_tight = true;

Is this for 6.1?

Re: [PATCH] util: fix abstract socket path copy

[Marc-André Lureau]

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

Hi

On Wed, Aug 4, 2021 at 12:39 PM Markus Armbruster <armbru@redhat.com> wrote:

> marcandre.lureau@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Commit 776b97d360 "qemu-sockets: add abstract UNIX domain socket
> > support" neglected to update socket_sockaddr_to_address_unix() and
> > copied the whole sun_path without taking "salen" into account.
> >
> > Later, commit 3b14b4ec49 "sockets: Fix socket_sockaddr_to_address_unix()
> > for abstract sockets" handled the abstract UNIX path, by stripping the
> > leading \0 character and fixing address details, but didn't use salen
> > either.
> >
> > Not taking "salen" into account may result in incorrect "path" being
> > returned in monitors commands, as we read past the address which is not
> > necessarily \0-terminated.
> >
> > Fixes: 776b97d3605ed0fc94443048fdf988c7725e38a9
> > Fixes: 3b14b4ec49a801067da19d6b8469eb1c1911c020
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  util/qemu-sockets.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> > index 080a240b74..f2f3676d1f 100644
> > --- a/util/qemu-sockets.c
> > +++ b/util/qemu-sockets.c
> > @@ -1345,13 +1345,16 @@ socket_sockaddr_to_address_unix(struct
> sockaddr_storage *sa,
> >      SocketAddress *addr;
> >      struct sockaddr_un *su = (struct sockaddr_un *)sa;
> >
> > +    assert(salen >= sizeof(su->sun_family) + 1 &&
> > +           salen <= sizeof(struct sockaddr_un));
> > +
> >      addr = g_new0(SocketAddress, 1);
> >      addr->type = SOCKET_ADDRESS_TYPE_UNIX;
> >  #ifdef CONFIG_LINUX
> >      if (!su->sun_path[0]) {
> >          /* Linux abstract socket */
> >          addr->u.q_unix.path = g_strndup(su->sun_path + 1,
> > -                                        sizeof(su->sun_path) - 1);
> > +                                        salen - sizeof(su->sun_family)
> - 1);
> >          addr->u.q_unix.has_abstract = true;
> >          addr->u.q_unix.abstract = true;
> >          addr->u.q_unix.has_tight = true;
>
> Is this for 6.1?
>

That would make sense, along with a few chardev fixes. Either Daniel (or
someone else) queue this, or I will eventually prepare a PR.

thanks

[-- Attachment #2: Type: text/html, Size: 3289 bytes --]

Re: [PATCH] util: fix abstract socket path copy

[Michael Tokarev]

19.07.2021 16:01, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Commit 776b97d360 "qemu-sockets: add abstract UNIX domain socket
> support" neglected to update socket_sockaddr_to_address_unix() and
> copied the whole sun_path without taking "salen" into account.
> 
> Later, commit 3b14b4ec49 "sockets: Fix socket_sockaddr_to_address_unix()
> for abstract sockets" handled the abstract UNIX path, by stripping the
> leading \0 character and fixing address details, but didn't use salen
> either.
> 
> Not taking "salen" into account may result in incorrect "path" being
> returned in monitors commands, as we read past the address which is not
> necessarily \0-terminated.
> 
> Fixes: 776b97d3605ed0fc94443048fdf988c7725e38a9
> Fixes: 3b14b4ec49a801067da19d6b8469eb1c1911c020
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   util/qemu-sockets.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 080a240b74..f2f3676d1f 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1345,13 +1345,16 @@ socket_sockaddr_to_address_unix(struct sockaddr_storage *sa,
>       SocketAddress *addr;
>       struct sockaddr_un *su = (struct sockaddr_un *)sa;
>   
> +    assert(salen >= sizeof(su->sun_family) + 1 &&
> +           salen <= sizeof(struct sockaddr_un));
> +

This seems to be wrong and causes this assert in the existing qemu code to fire up.
I can't say for *sure* but it *seems* the issue is the trailing null terminator
in the case of abstract sockets on linux where the path name is exactly equal
to 108 bytes (including the leading \0).

The prob seems to be that socket_local_address() initially allocates
sockaddr_storage bytes for the getsockname() call - which is larger
than sizeof(sockaddr_un). So the kernel is able to add the zero terminator,
and return 111 bytes in salen, not 110 as the size of sockaddr_un.
And later on the above assert will fire up..

We have a bug in debian about this very issue:
http://bugs.debian.org/993145

The fix is a bit more complex than adding a +1 to the sizeof in the last
condition of the assert...

Thanks,

/mjt

>       addr = g_new0(SocketAddress, 1);
>       addr->type = SOCKET_ADDRESS_TYPE_UNIX;
>   #ifdef CONFIG_LINUX
>       if (!su->sun_path[0]) {
>           /* Linux abstract socket */
>           addr->u.q_unix.path = g_strndup(su->sun_path + 1,
> -                                        sizeof(su->sun_path) - 1);
> +                                        salen - sizeof(su->sun_family) - 1);
>           addr->u.q_unix.has_abstract = true;
>           addr->u.q_unix.abstract = true;
>           addr->u.q_unix.has_tight = true;
> 

Re: [PATCH] util: fix abstract socket path copy

[Michael Tokarev]

31.08.2021 00:38, Michael Tokarev wrote:
...
>> @@ -1345,13 +1345,16 @@ socket_sockaddr_to_address_unix(struct sockaddr_storage *sa,
>>       SocketAddress *addr;
>>       struct sockaddr_un *su = (struct sockaddr_un *)sa;
>> +    assert(salen >= sizeof(su->sun_family) + 1 &&
>> +           salen <= sizeof(struct sockaddr_un));
>> +
> 
> This seems to be wrong and causes this assert in the existing qemu code to fire up.
> I can't say for *sure* but it *seems* the issue is the trailing null terminator
> in the case of abstract sockets on linux where the path name is exactly equal
> to 108 bytes (including the leading \0).
> 
> The prob seems to be that socket_local_address() initially allocates
> sockaddr_storage bytes for the getsockname() call - which is larger
> than sizeof(sockaddr_un). So the kernel is able to add the zero terminator,
> and return 111 bytes in salen, not 110 as the size of sockaddr_un.
> And later on the above assert will fire up..

Here's the linux kernel code:

net/unix/af_unix.c
tatic int unix_mkname(struct sockaddr_un *sunaddr, int len, unsigned int *hashp)
{
         *hashp = 0;

         if (len <= sizeof(short) || len > sizeof(*sunaddr))
                 return -EINVAL;
         if (!sunaddr || sunaddr->sun_family != AF_UNIX)
                 return -EINVAL;
         if (sunaddr->sun_path[0]) {
                 /*
                  * This may look like an off by one error but it is a bit more
                  * subtle. 108 is the longest valid AF_UNIX path for a binding.
                  * sun_path[108] doesn't as such exist.  However in kernel space
                  * we are guaranteed that it is a valid memory location in our
                  * kernel address buffer.
                  */
                 ((char *)sunaddr)[len] = 0;
                 len = strlen(sunaddr->sun_path)+1+sizeof(short);
                 return len;
         }
..

Suppose we have sun_path of exactly 108 chars (not counting the trailing
zero terminator), so the total length of the sockaddr is 110 bytes.

After the len = recalculation above, it will be 111 bytes -
  strlen(sun_path)=108 + 1 + sizeof(short)=2 = 111.

And this is the value used to be returned in the getsockname/getpeername
calls.

So this has nothing to do with socket being abstract or not. We asked for
larger storage for the sockaddr structure, and the kernel was able to build
one for us, including the trailing \0 byte. Currently kernel will only
return +1 byte for non-abstract sockets, but this is an implementation
detail. We asked for it. So we - I think - should account for this +1
byte here.

/mjt

Re: [PATCH] util: fix abstract socket path copy

[Michael Tokarev]

31.08.2021 01:06, Michael Tokarev wrote:
...
> And this is the value used to be returned in the getsockname/getpeername
> calls.
> 
> So this has nothing to do with socket being abstract or not. We asked for
> larger storage for the sockaddr structure, and the kernel was able to build
> one for us, including the trailing \0 byte. Currently kernel will only
> return +1 byte for non-abstract sockets, but this is an implementation
> detail. We asked for it. So we - I think - should account for this +1
> byte here.

And here's the note from man 7 unix:

BUGS
        When binding a socket to an address, Linux is one of the implementations that appends a null termina‐
        tor if none is supplied in sun_path.  In most cases this is unproblematic: when the socket address is
        retrieved, it will be one byte longer than that supplied when the socket was bound.   However,  there
        is  one case where confusing behavior can result: if 108 non-null bytes are supplied when a socket is
        bound,  then  the  addition  of  the  null  terminator  takes  the  length  of  the  pathname  beyond
        sizeof(sun_path).   Consequently, when retrieving the socket address (for example, via accept(2)), if
        the input addrlen argument for the retrieving call is specified as sizeof(struct  sockaddr_un),  then
        the returned address structure won't have a null terminator in sun_path.

So how about this:

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index f2f3676d1f..83926dc2bc 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -1345,8 +1345,9 @@ socket_sockaddr_to_address_unix(struct sockaddr_storage *sa,
      SocketAddress *addr;
      struct sockaddr_un *su = (struct sockaddr_un *)sa;

+    /* kernel might have added \0 terminator to non-abstract socket */
      assert(salen >= sizeof(su->sun_family) + 1 &&
-           salen <= sizeof(struct sockaddr_un));
+           salen <= sizeof(struct sockaddr_un) + su->sun_path[0] ? 1 : 0);

      addr = g_new0(SocketAddress, 1);
      addr->type = SOCKET_ADDRESS_TYPE_UNIX;

?

/mjt

Re: [PATCH] util: fix abstract socket path copy

[Peter Maydell]

On Mon, 30 Aug 2021 at 23:30, Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> 31.08.2021 01:06, Michael Tokarev wrote:
> ...
> > And this is the value used to be returned in the getsockname/getpeername
> > calls.
> >
> > So this has nothing to do with socket being abstract or not. We asked for
> > larger storage for the sockaddr structure, and the kernel was able to build
> > one for us, including the trailing \0 byte.

> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index f2f3676d1f..83926dc2bc 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1345,8 +1345,9 @@ socket_sockaddr_to_address_unix(struct sockaddr_storage *sa,
>       SocketAddress *addr;
>       struct sockaddr_un *su = (struct sockaddr_un *)sa;
>
> +    /* kernel might have added \0 terminator to non-abstract socket */
>       assert(salen >= sizeof(su->sun_family) + 1 &&
> -           salen <= sizeof(struct sockaddr_un));
> +           salen <= sizeof(struct sockaddr_un) + su->sun_path[0] ? 1 : 0);

Q: Why are we imposing an upper limit on salen anyway?
We need the lower limit because salen is supposed to include
the whole of the 'struct sockaddr_un' and we assume that.
But what's the upper limit here protecting?

Q2: why does our required upper limit change depending on whether
there happens to be a string in the sun_path array or not ?

Q3: when we copy the sun_path, why do we skip the first character?

        addr->u.q_unix.path = g_strndup(su->sun_path + 1,
                                        salen - sizeof(su->sun_family) - 1);

-- PMM

Re: [PATCH] util: fix abstract socket path copy

[Michael Tokarev]

31.08.2021 12:53, Peter Maydell wrote:
> On Mon, 30 Aug 2021 at 23:30, Michael Tokarev <mjt@tls.msk.ru> wrote:
>>
>> 31.08.2021 01:06, Michael Tokarev wrote:
>> ...
>>> And this is the value used to be returned in the getsockname/getpeername
>>> calls.
>>>
>>> So this has nothing to do with socket being abstract or not. We asked for
>>> larger storage for the sockaddr structure, and the kernel was able to build
>>> one for us, including the trailing \0 byte.
> 
>> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
>> index f2f3676d1f..83926dc2bc 100644
>> --- a/util/qemu-sockets.c
>> +++ b/util/qemu-sockets.c
>> @@ -1345,8 +1345,9 @@ socket_sockaddr_to_address_unix(struct sockaddr_storage *sa,
>>        SocketAddress *addr;
>>        struct sockaddr_un *su = (struct sockaddr_un *)sa;
>>
>> +    /* kernel might have added \0 terminator to non-abstract socket */
>>        assert(salen >= sizeof(su->sun_family) + 1 &&
>> -           salen <= sizeof(struct sockaddr_un));
>> +           salen <= sizeof(struct sockaddr_un) + su->sun_path[0] ? 1 : 0);
> 
> Q: Why are we imposing an upper limit on salen anyway?
> We need the lower limit because salen is supposed to include
> the whole of the 'struct sockaddr_un' and we assume that.
> But what's the upper limit here protecting?

It is not about protection really, it is about correctness.
This is actually a grey area. This single trailing \0 byte
depends on the implementation. Please read man 7 unix -
especially the "Pathname sockets" and BUGS sections.


> Q2: why does our required upper limit change depending on whether
> there happens to be a string in the sun_path array or not ?

Because for abstract sockets (the ones whos name starts with \0
byte) the sun_path is treated as a blob of given length, without
the additional trailing \0, and neither the kernel nor userspace
is trying to add the terminator, while for pathname sockets this
is not the case and someone has to add the trailing \0 somewhere.

> Q3: when we copy the sun_path, why do we skip the first character?
> 
>          addr->u.q_unix.path = g_strndup(su->sun_path + 1,
>                                          salen - sizeof(su->sun_family) - 1);

Because this is the case of abstract socket. Actual name is a blob
starting from the sun_path+1. I dunno if this is correct to use
g_strndup there - will it terminate at first \0 encountered or not,
but this is another question. The difference between this place
and regular pathname socket below is this: addr->u.q_unix.abstract = true;

We have the issue only whith regular traditional unix pathname sockets
whose name is exactly 108 bytes long.  I provided the code from
the kernel in another thread showing how it increases the len by
one in this case and returns it in getsockname() if userspace
provided enough room.

/mjt

Re: [PATCH] util: fix abstract socket path copy

[Peter Maydell]

On Tue, 31 Aug 2021 at 11:17, Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> 31.08.2021 12:53, Peter Maydell wrote:
> > On Mon, 30 Aug 2021 at 23:30, Michael Tokarev <mjt@tls.msk.ru> wrote:
> >>
> >> 31.08.2021 01:06, Michael Tokarev wrote:
> >> ...
> >>> And this is the value used to be returned in the getsockname/getpeername
> >>> calls.
> >>>
> >>> So this has nothing to do with socket being abstract or not. We asked for
> >>> larger storage for the sockaddr structure, and the kernel was able to build
> >>> one for us, including the trailing \0 byte.
> >
> >> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> >> index f2f3676d1f..83926dc2bc 100644
> >> --- a/util/qemu-sockets.c
> >> +++ b/util/qemu-sockets.c
> >> @@ -1345,8 +1345,9 @@ socket_sockaddr_to_address_unix(struct sockaddr_storage *sa,
> >>        SocketAddress *addr;
> >>        struct sockaddr_un *su = (struct sockaddr_un *)sa;
> >>
> >> +    /* kernel might have added \0 terminator to non-abstract socket */
> >>        assert(salen >= sizeof(su->sun_family) + 1 &&
> >> -           salen <= sizeof(struct sockaddr_un));
> >> +           salen <= sizeof(struct sockaddr_un) + su->sun_path[0] ? 1 : 0);
> >
> > Q: Why are we imposing an upper limit on salen anyway?
> > We need the lower limit because salen is supposed to include
> > the whole of the 'struct sockaddr_un' and we assume that.
> > But what's the upper limit here protecting?
>
> It is not about protection really, it is about correctness.
> This is actually a grey area. This single trailing \0 byte
> depends on the implementation. Please read man 7 unix -
> especially the "Pathname sockets" and BUGS sections.

Yes, I know about that. Why are we assert()ing ? Our
implementation here doesn't care whether the struct
we're passed is exactly the size of a sockaddr_un,
a bit bigger than it, or 5 bytes bigger. We're not going
to crash or misbehave if the caller passes us in an oversized
buffer.

> > Q2: why does our required upper limit change depending on whether
> > there happens to be a string in the sun_path array or not ?
>
> Because for abstract sockets (the ones whos name starts with \0
> byte) the sun_path is treated as a blob of given length, without
> the additional trailing \0, and neither the kernel nor userspace
> is trying to add the terminator, while for pathname sockets this
> is not the case and someone has to add the trailing \0 somewhere.

Ah, I hadn't realized about the abstract-sockets case. Thanks.

-- PMM

Re: [PATCH] util: fix abstract socket path copy

[Marc-André Lureau]

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

Hi

On Tue, Aug 31, 2021 at 2:32 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Tue, 31 Aug 2021 at 11:17, Michael Tokarev <mjt@tls.msk.ru> wrote:
> >
> > 31.08.2021 12:53, Peter Maydell wrote:
> > > On Mon, 30 Aug 2021 at 23:30, Michael Tokarev <mjt@tls.msk.ru> wrote:
> > >>
> > >> 31.08.2021 01:06, Michael Tokarev wrote:
> > >> ...
> > >>> And this is the value used to be returned in the
> getsockname/getpeername
> > >>> calls.
> > >>>
> > >>> So this has nothing to do with socket being abstract or not. We
> asked for
> > >>> larger storage for the sockaddr structure, and the kernel was able
> to build
> > >>> one for us, including the trailing \0 byte.
> > >
> > >> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> > >> index f2f3676d1f..83926dc2bc 100644
> > >> --- a/util/qemu-sockets.c
> > >> +++ b/util/qemu-sockets.c
> > >> @@ -1345,8 +1345,9 @@ socket_sockaddr_to_address_unix(struct
> sockaddr_storage *sa,
> > >>        SocketAddress *addr;
> > >>        struct sockaddr_un *su = (struct sockaddr_un *)sa;
> > >>
> > >> +    /* kernel might have added \0 terminator to non-abstract socket
> */
> > >>        assert(salen >= sizeof(su->sun_family) + 1 &&
> > >> -           salen <= sizeof(struct sockaddr_un));
> > >> +           salen <= sizeof(struct sockaddr_un) + su->sun_path[0] ? 1
> : 0);
> > >
> > > Q: Why are we imposing an upper limit on salen anyway?
> > > We need the lower limit because salen is supposed to include
> > > the whole of the 'struct sockaddr_un' and we assume that.
> > > But what's the upper limit here protecting?
> >
> > It is not about protection really, it is about correctness.
> > This is actually a grey area. This single trailing \0 byte
> > depends on the implementation. Please read man 7 unix -
> > especially the "Pathname sockets" and BUGS sections.
>
> Yes, I know about that. Why are we assert()ing ? Our
> implementation here doesn't care whether the struct
> we're passed is exactly the size of a sockaddr_un,
> a bit bigger than it, or 5 bytes bigger. We're not going
> to crash or misbehave if the caller passes us in an oversized
> buffer.
>

The minimal len check seems appropriate, since the function accesses at
least the first X bytes (3 I suppose).

While at it I probably added an upper bound that I thought made sense (the
size of sockaddr_un), but I did wrong.

But now, I also think we can remove the upper bound check.


> > > Q2: why does our required upper limit change depending on whether
> > > there happens to be a string in the sun_path array or not ?
> >
> > Because for abstract sockets (the ones whos name starts with \0
> > byte) the sun_path is treated as a blob of given length, without
> > the additional trailing \0, and neither the kernel nor userspace
> > is trying to add the terminator, while for pathname sockets this
> > is not the case and someone has to add the trailing \0 somewhere.
>
> Ah, I hadn't realized about the abstract-sockets case. Thanks.
>
> -- PMM
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 4273 bytes --]