All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Daniel P. Berrangé" <berrange@redhat.com>
To: "Marc-André Lureau" <marcandre.lureau@redhat.com>
Cc: Michael Tokarev <mjt@tls.msk.ru>,
	qemu-devel <qemu-devel@nongnu.org>,
	qemu-stable <qemu-stable@nongnu.org>
Subject: Re: [PATCH] qemu-sockets: fix unix socket path copy (again)
Date: Wed, 1 Sep 2021 10:12:32 +0100	[thread overview]
Message-ID: <YS9EAOX4g7zIjxhp@redhat.com> (raw)
In-Reply-To: <CAMxuvayrgXYBU0dcmmO2=Po1fBLFugxP7JS7KrR83iVQZE9fKg@mail.gmail.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 :|



  parent reply	other threads:[~2021-09-01  9:13 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-31 18:26 Michael Tokarev
2021-08-31 19:21 ` Marc-André Lureau
2021-08-31 19:26   ` Michael Tokarev
2021-09-01  9:12   ` Daniel P. Berrangé [this message]
2021-09-01  9:20     ` Michael Tokarev
2021-08-31 19:47 ` Peter Maydell
2021-09-01  8:29   ` Michael Tokarev
2021-09-01  9:52     ` Peter Maydell
2021-09-01 11:45       ` Michael Tokarev
2021-09-01 11:58         ` Daniel P. Berrangé
2021-09-01 11:57     ` Daniel P. Berrangé

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YS9EAOX4g7zIjxhp@redhat.com \
    --to=berrange@redhat.com \
    --cc=marcandre.lureau@redhat.com \
    --cc=mjt@tls.msk.ru \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-stable@nongnu.org \
    --subject='Re: [PATCH] qemu-sockets: fix unix socket path copy (again)' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.