qemu-sockets: account for trailing \0 byte in unix socket pathname

qemu-sockets: account for trailing \0 byte in unix socket pathname

[Michael Tokarev]

Linux kernel can return size of af_unix socket to be
one byte larger than sockaddr_un structure - adding
the trailing zero byte.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
Fixes: 4cfd970ec188558daa6214f26203fe553fb1e01f (first in 6.1.0)
Cc: qemu-stable@nongnu.org

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;

Re: qemu-sockets: account for trailing \0 byte in unix socket pathname

[Marc-André Lureau]

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

Hi

On Tue, Aug 31, 2021 at 3:00 AM Michael Tokarev <mjt@tls.msk.ru> wrote:

> Linux kernel can return size of af_unix socket to be
> one byte larger than sockaddr_un structure - adding
> the trailing zero byte.
>
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> Fixes: 4cfd970ec188558daa6214f26203fe553fb1e01f (first in 6.1.0)
> Cc: qemu-stable@nongnu.org
>
> 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);
>
>
Looks right, but we may want to drop the upper bound check altogether. I
thought the path must always fit the sockaddr_un, but since that's not the
case it's only harmful here.

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

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

-- 
Marc-André Lureau

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

Re: qemu-sockets: account for trailing \0 byte in unix socket pathname

[Daniel P. Berrangé]

On Tue, Aug 31, 2021 at 04:32:36PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Aug 31, 2021 at 3:00 AM Michael Tokarev <mjt@tls.msk.ru> wrote:
> 
> > Linux kernel can return size of af_unix socket to be
> > one byte larger than sockaddr_un structure - adding
> > the trailing zero byte.
> >
> > Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> > Fixes: 4cfd970ec188558daa6214f26203fe553fb1e01f (first in 6.1.0)
> > Cc: qemu-stable@nongnu.org
> >
> > 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);
> >
> >
> Looks right, but we may want to drop the upper bound check altogether. I
> thought the path must always fit the sockaddr_un, but since that's not the
> case it's only harmful here.

Yeah, I don't think either old or new code are good here. The assert
is validating Linux semantics here, other platforms don't add the
extra NUL

Also later in the same method the string copy is dubious too:

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

The man page says that to take account of non-portable semantics
wrt trailing NULs, the path length should be calculated using:

     strnlen(addr.sun_path, salen - offsetof(sockaddr_un, sun_path))


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: qemu-sockets: account for trailing \0 byte in unix socket pathname

[Michael Tokarev]

31.08.2021 01:54, Michael Tokarev wrote:
> Linux kernel can return size of af_unix socket to be
> one byte larger than sockaddr_un structure - adding
> the trailing zero byte.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> Fixes: 4cfd970ec188558daa6214f26203fe553fb1e01f (first in 6.1.0)
> Cc: qemu-stable@nongnu.org
> 
> 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;

Actually, this is not sufficient.

While this change fixes one issue (the famous trailing null byte \0),
the actual assertion failure occurs because salen = 2, ie, too SMALL,
not too large.

So it looks like libvirt provides an unnamed socket there, --
maybe from a socketpair(2)?

Hwell..

/mjt

Re: qemu-sockets: account for trailing \0 byte in unix socket pathname

[Marc-André Lureau]

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

Hi

On Tue, Aug 31, 2021 at 9:17 PM Michael Tokarev <mjt@tls.msk.ru> wrote:

> 31.08.2021 01:54, Michael Tokarev wrote:
> > Linux kernel can return size of af_unix socket to be
> > one byte larger than sockaddr_un structure - adding
> > the trailing zero byte.
> >
> > Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> > Fixes: 4cfd970ec188558daa6214f26203fe553fb1e01f (first in 6.1.0)
> > Cc: qemu-stable@nongnu.org
> >
> > 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;
>
> Actually, this is not sufficient.
>
> While this change fixes one issue (the famous trailing null byte \0),
> the actual assertion failure occurs because salen = 2, ie, too SMALL,
> not too large.
>
> So it looks like libvirt provides an unnamed socket there, --
> maybe from a socketpair(2)?
>

Yes

Ok, I guess it should still check for salen >= sizeof(su->sun_family)

and then modify if (salen > sizeof(su->sun_family) && !su->sun_path[0]) {


> Hwell..
>

hmm, too bad we didn't catch it during RC!

(strange that it seems to hit Debian libvirt/virt-manager users and
apparently not on Fedora)

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

Re: qemu-sockets: account for trailing \0 byte in unix socket pathname

[Daniel P. Berrangé]

On Tue, Aug 31, 2021 at 09:22:01PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Aug 31, 2021 at 9:17 PM Michael Tokarev <mjt@tls.msk.ru> wrote:
> 
> > 31.08.2021 01:54, Michael Tokarev wrote:
> > > Linux kernel can return size of af_unix socket to be
> > > one byte larger than sockaddr_un structure - adding
> > > the trailing zero byte.
> > >
> > > Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> > > Fixes: 4cfd970ec188558daa6214f26203fe553fb1e01f (first in 6.1.0)
> > > Cc: qemu-stable@nongnu.org
> > >
> > > 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;
> >
> > Actually, this is not sufficient.
> >
> > While this change fixes one issue (the famous trailing null byte \0),
> > the actual assertion failure occurs because salen = 2, ie, too SMALL,
> > not too large.
> >
> > So it looks like libvirt provides an unnamed socket there, --
> > maybe from a socketpair(2)?
> >
> 
> Yes

No, libvirt binds to a named socket path and passes in a pre-opened
FD for the listener socket. There shouldn't be any socketpair involved.


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: qemu-sockets: account for trailing \0 byte in unix socket pathname

[Michael Tokarev]

31.08.2021 20:38, Daniel P. Berrangé wrote:
...
>>> So it looks like libvirt provides an unnamed socket there, --
>>> maybe from a socketpair(2)?
>>>
>>
>> Yes
> 
> No, libvirt binds to a named socket path and passes in a pre-opened
> FD for the listener socket. There shouldn't be any socketpair involved.

Here's some more info from the original bugreport:

31.08.2021 00:20, dann frazier wrote:
 > Aha! It seems that the important difference is whether or not the
 > virt-manager GUI window for the VM is active. If it is active, the VM
 > crashes regardless of how it is started (virsh console start/clicking
 > "play" button). If the GUI is not active, the VM always works.
 >
 > With this knowledge I am able to confidently say that reverting
 > 4cfd970ec1 *does* reliably avoid the problem.

We'll try to figure out the calltrace, where this socket is coming from..

/mjt

Re: qemu-sockets: account for trailing \0 byte in unix socket pathname

[Daniel P. Berrangé]

On Tue, Aug 31, 2021 at 08:47:43PM +0300, Michael Tokarev wrote:
> 31.08.2021 20:38, Daniel P. Berrangé wrote:
> ...
> > > > So it looks like libvirt provides an unnamed socket there, --
> > > > maybe from a socketpair(2)?
> > > > 
> > > 
> > > Yes
> > 
> > No, libvirt binds to a named socket path and passes in a pre-opened
> > FD for the listener socket. There shouldn't be any socketpair involved.
> 
> Here's some more info from the original bugreport:
> 
> 31.08.2021 00:20, dann frazier wrote:
> > Aha! It seems that the important difference is whether or not the
> > virt-manager GUI window for the VM is active. If it is active, the VM
> > crashes regardless of how it is started (virsh console start/clicking
> > "play" button). If the GUI is not active, the VM always works.
> >
> > With this knowledge I am able to confidently say that reverting
> > 4cfd970ec1 *does* reliably avoid the problem.
> 
> We'll try to figure out the calltrace, where this socket is coming from..

Oh, it is probably from the graphical console connection to SPICE or VNC.
For those virt-manager will pass in a socket creted with socketpair()
via libvirt, in order to bypass the need for authentication when running
locally.

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 :|