| Message ID | 20210719130112.932069-1-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | util: fix abstract socket path copy | expand |
On Mon, Jul 19, 2021 at 05:01:12PM +0400, 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. So IIUC, this is only affecting what is printed in the monitor when querying chardevs, not the actual functional behaviour between clients/servers connecting/listening ? > > 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)); > + > 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; Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel
On Mon, Jul 19, 2021 at 5:49 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > On Mon, Jul 19, 2021 at 05:01:12PM +0400, 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. > > So IIUC, this is only affecting what is printed in the monitor > when querying chardevs, not the actual functional behaviour > between clients/servers connecting/listening ? > > I think so, both hmp & qmp, and the info_report() in server_accept_sync(). But I didn't carefully review all the potential users (who else could they be?). > > > > 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)); > > + > > 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; > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > > > 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 :| > >
在 2021/7/19 21:01, 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)); > + > 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; Reviewed-by: xiaoqiang zhao <zxq_yx_007@163.com>
marcandre.lureau@redhat.com writes: > 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)); > + > 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; Is this for 6.1?
Hi On Wed, Aug 4, 2021 at 12:39 PM Markus Armbruster <armbru@redhat.com> wrote: > marcandre.lureau@redhat.com writes: > > > 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)); > > + > > 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; > > Is this for 6.1? > That would make sense, along with a few chardev fixes. Either Daniel (or someone else) queue this, or I will eventually prepare a PR. thanks
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; >
31.08.2021 00:38, Michael Tokarev wrote: ... >> @@ -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.. Here's the linux kernel code: net/unix/af_unix.c tatic int unix_mkname(struct sockaddr_un *sunaddr, int len, unsigned int *hashp) { *hashp = 0; if (len <= sizeof(short) || len > sizeof(*sunaddr)) return -EINVAL; if (!sunaddr || sunaddr->sun_family != AF_UNIX) return -EINVAL; if (sunaddr->sun_path[0]) { /* * This may look like an off by one error but it is a bit more * subtle. 108 is the longest valid AF_UNIX path for a binding. * sun_path[108] doesn't as such exist. However in kernel space * we are guaranteed that it is a valid memory location in our * kernel address buffer. */ ((char *)sunaddr)[len] = 0; len = strlen(sunaddr->sun_path)+1+sizeof(short); return len; } .. Suppose we have sun_path of exactly 108 chars (not counting the trailing zero terminator), so the total length of the sockaddr is 110 bytes. After the len = recalculation above, it will be 111 bytes - strlen(sun_path)=108 + 1 + sizeof(short)=2 = 111. 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. Currently kernel will only return +1 byte for non-abstract sockets, but this is an implementation detail. We asked for it. So we - I think - should account for this +1 byte here. /mjt
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. Currently kernel will only > return +1 byte for non-abstract sockets, but this is an implementation > detail. We asked for it. So we - I think - should account for this +1 > byte here. And here's the note from man 7 unix: BUGS When binding a socket to an address, Linux is one of the implementations that appends a null termina‐ tor if none is supplied 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 addition of the null terminator takes the length of the pathname 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 structure won't have a null terminator in sun_path. So how about this: 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); addr = g_new0(SocketAddress, 1); addr->type = SOCKET_ADDRESS_TYPE_UNIX; ? /mjt
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? Q2: why does our required upper limit change depending on whether there happens to be a string in the sun_path array or not ? Q3: when we copy the sun_path, why do we skip the first character? addr->u.q_unix.path = g_strndup(su->sun_path + 1, salen - sizeof(su->sun_family) - 1); -- PMM
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. > 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. > Q3: when we copy the sun_path, why do we skip the first character? > > addr->u.q_unix.path = g_strndup(su->sun_path + 1, > salen - sizeof(su->sun_family) - 1); Because this is the case of abstract socket. Actual name is a blob starting from the sun_path+1. I dunno if this is correct to use g_strndup there - will it terminate at first \0 encountered or not, but this is another question. The difference between this place and regular pathname socket below is this: addr->u.q_unix.abstract = true; We have the issue only whith regular traditional unix pathname sockets whose name is exactly 108 bytes long. I provided the code from the kernel in another thread showing how it increases the len by one in this case and returns it in getsockname() if userspace provided enough room. /mjt
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
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 > >
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)); + 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;