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

[Peter Maydell]

On Tue, 31 Aug 2021 at 11:17, Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> 31.08.2021 12:53, Peter Maydell wrote:
> > On Mon, 30 Aug 2021 at 23:30, Michael Tokarev <mjt@tls.msk.ru> wrote:
> >>
> >> 31.08.2021 01:06, Michael Tokarev wrote:
> >> ...
> >>> And this is the value used to be returned in the getsockname/getpeername
> >>> calls.
> >>>
> >>> So this has nothing to do with socket being abstract or not. We asked for
> >>> larger storage for the sockaddr structure, and the kernel was able to 
> >>> build
> >>> one for us, including the trailing \0 byte.
> >
> >> 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);
> >
> > Q: Why are we imposing an upper limit on salen anyway?
> > We need the lower limit because salen is supposed to include
> > the whole of the 'struct sockaddr_un' and we assume that.
> > But what's the upper limit here protecting?
>
> It is not about protection really, it is about correctness.
> This is actually a grey area. This single trailing \0 byte
> depends on the implementation. Please read man 7 unix -
> especially the "Pathname sockets" and BUGS sections.

Yes, I know about that. Why are we assert()ing ? Our
implementation here doesn't care whether the struct
we're passed is exactly the size of a sockaddr_un,
a bit bigger than it, or 5 bytes bigger. We're not going
to crash or misbehave if the caller passes us in an oversized
buffer.

> > Q2: why does our required upper limit change depending on whether
> > there happens to be a string in the sun_path array or not ?
>
> Because for abstract sockets (the ones whos name starts with \0
> byte) the sun_path is treated as a blob of given length, without
> the additional trailing \0, and neither the kernel nor userspace
> is trying to add the terminator, while for pathname sockets this
> is not the case and someone has to add the trailing \0 somewhere.

Ah, I hadn't realized about the abstract-sockets case. Thanks.

-- PMM