[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2] qemu-sockets: fix unix socket path copy (again)
From: |
Daniel P . Berrangé |
Subject: |
Re: [PATCH v2] qemu-sockets: fix unix socket path copy (again) |
Date: |
Wed, 1 Sep 2021 14:01:53 +0100 |
User-agent: |
Mutt/2.0.7 (2021-05-04) |
On Wed, Sep 01, 2021 at 03:50:55PM +0300, Michael Tokarev wrote:
> Commit 4cfd970ec188558daa6214f26203fe553fb1e01f added an
> assert which ensures the path within an address of a unix
> socket returned from the kernel is at least one byte and
> does not exceed sun_path buffer. Both of this constraints
> are wrong:
>
> A unix socket can be unnamed, in this case the path is
> completely empty (not even \0)
>
> And some implementations (notable linux) can add extra
> trailing byte (\0) _after_ the sun_path buffer if we
> passed buffer larger than it (and we do).
>
> So remove the assertion (since it causes real-life breakage)
> but at the same time fix the usage of sun_path. Namely,
> we should not access sun_path[0] if kernel did not return
> it at all (this is the case for unnamed sockets),
> and use the returned salen when copyig actual path as an
> upper constraint for the amount of bytes to copy - this
> will ensure we wont exceed the information provided by
> the kernel, regardless whenever there is a trailing \0
> or not. This also helps with unnamed sockets.
>
> Note the case of abstract socket, the sun_path is actually
> a blob and can contain \0 characters, - it should not be
> passed to g_strndup and the like, it should be accessed by
> memcpy-like functions.
>
> Fixes: 4cfd970ec188558daa6214f26203fe553fb1e01f
> Fixes: http://bugs.debian.org/993145
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> CC: qemu-stable@nongnu.org
> ---
> util/qemu-sockets.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index f2f3676d1f..842bd707ac 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1345,13 +1345,11 @@ 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;
> + salen -= offsetof(struct sockaddr_un, sun_path);
Adjusted salen value....
> #ifdef CONFIG_LINUX
> - if (!su->sun_path[0]) {
> + if (salen > 0 && !su->sun_path[0]) {
> /* Linux abstract socket */
> addr->u.q_unix.path = g_strndup(su->sun_path + 1,
> salen - sizeof(su->sun_family) - 1);
....still assuming the original salen value.
> @@ -1363,7 +1361,7 @@ socket_sockaddr_to_address_unix(struct sockaddr_storage
> *sa,
> }
> #endif
>
> - addr->u.q_unix.path = g_strndup(su->sun_path, sizeof(su->sun_path));
> + addr->u.q_unix.path = g_strndup(su->sun_path, salen);
> return addr;
> }
> #endif /* WIN32 */
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 :|