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

["Marc-André Lureau" <marcandre.lureau@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3092 bytes --]

Hi

On Tue, Aug 31, 2021 at 2:32 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

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

The minimal len check seems appropriate, since the function accesses at
least the first X bytes (3 I suppose).

While at it I probably added an upper bound that I thought made sense (the
size of sockaddr_un), but I did wrong.

But now, I also think we can remove the upper bound check.


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

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 4273 bytes --]