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

[Michael Tokarev <mjt@tls.msk.ru>]

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