Re: [PATCH] qemu-sockets: fix unix socket path copy (again)

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, Aug 31, 2021 at 11:21:43PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Aug 31, 2021 at 10:26 PM Michael Tokarev <mjt@tls.msk.ru> wrote:
> 
> > We test whenever the path of unix-domain socket
> > address is non-empty and strictly-less than
> > the length of the path buffer. Both these
> > conditions are wrong: the socket can be unnamed,
> > with empty path, or socket can have pathname
> > null-terminated _after_ the sun_path buffer,
> > since we provided more room when asking kernel.
> >
> > So on one side, allow empty, unnamed sockets
> > (and adjust the test for abstract socket too -
> > only do that if the socket is not unnamed),
> > and on another side, allow path length to be
> > up to our own maximum, - we have up to size
> > of sockaddr_storage there.
> >
> > While at it, fix the duplication of regular
> > pathname socket to not require trailing \0
> > (since it can be missing for unnamed sockets).
> >
> > Fixes: 4cfd970ec188558daa6214f26203fe553fb1e01f (first in 6.1.0)
> > Fixes: http://bugs.debian.org/993145
> > Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> > Cc: qemu-stable@nongnu.org
> > --
> > Two questions.
> > 1. Why do we store the name of the socket to start with?
> > 2. The code in the abstract socket case should not use
> >    g_strndup but g_memdup instead, since the whole thing
> >    is a blob of the given length, not a \0-terminated string.
> > ---
> >  util/qemu-sockets.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> > index f2f3676d1f..7c83d81792 100644
> > --- a/util/qemu-sockets.c
> > +++ b/util/qemu-sockets.c
> > @@ -1345,13 +1345,20 @@ 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));
> > +    /* there's a corner case when trailing \0 does not fit into
> > +     * sockaddr_un. Compare length with sizeof(sockaddr_storage),
> > +     * not with sizeof(sockaddr_un), since this is what we actually
> > +     * provide, to ensure we had no truncation and a room for
> > +     * the trailing \0 which we add below.
> > +     * When salen == sizeof(sun_family) it is unnamed socket,
> > +     * and when first byte of sun_path is \0, it is abstract. */
> > +    assert(salen >= sizeof(su->sun_family) &&
> > +           salen <= sizeof(struct sockaddr_storage));
> >
> 
> Seems right to me, however there are some notes in libc bits/socket.h
> /* Structure large enough to hold any socket address (with the historical
>    exception of AF_UNIX).  */
> 
> And also this
> https://idea.popcount.org/2019-12-06-addressing/#fn:sockaddr_storage
> 
> I must say I feel confused by those comments :) Is it large enough or not??

From 'man unix(7)'

       When  binding a socket to an address, Linux is one of the im‐
       plementations that appends a null terminator if none is  sup‐
       plied 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 ad‐
       dition of the null terminator takes the length of  the  path‐
       name  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  struc‐
       ture won't have a null terminator in sun_path.

Essentially this is saying that if the kernel allocates the
sockaddr_un buffer, it will have intentionally *over-allocated*
by 1 byte.  So you can't assume salen <= sizeof(sockaddr_un)
nor  salen <= sizeof(sockadr_storage) unless *you* have allocated
the sockaddr buffer.

In this context of QEMU, I don't know whether the addr buffer
we dealing with was allocated by QEMU or the kernel, if the
former, then we're safe in terms of size, but cannot assume
NUL terminator.


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