[Qemu-devel] [PATCH] sockets: report error if UNIX socket path is too long

Daniel P. Berrange posted 1 patch 6 years, 10 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20170524154205.1914-1-berrange@redhat.com
Test checkpatch passed
Test docker passed
Test s390x passed
util/qemu-sockets.c | 6 ++++++
1 file changed, 6 insertions(+)
[Qemu-devel] [PATCH] sockets: report error if UNIX socket path is too long
Posted by Daniel P. Berrange 6 years, 10 months ago
The 'struct sockaddr_un' only allows 108 bytes for the socket
path. Currently QEMU uses snprintf() and so silently truncates
the socket path provided by the user. This is undesirable because
the user will then be unable to connect to the path they asked
for. This change makes QEMU bounds check and report an explicit
error message.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 util/qemu-sockets.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index d8183f7..e249dbe 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -855,6 +855,12 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
     memset(&un, 0, sizeof(un));
     un.sun_family = AF_UNIX;
     if (saddr->path && strlen(saddr->path)) {
+        if (strlen(saddr->path) > sizeof(un.sun_path)) {
+            error_setg(errp, "UNIX socket path '%s' is too long", saddr->path);
+            error_append_hint(errp, "Path must be less than %zu bytes\n",
+                              sizeof(un.sun_path));
+            return -1;
+        }
         snprintf(un.sun_path, sizeof(un.sun_path), "%s", saddr->path);
     } else {
         const char *tmpdir = getenv("TMPDIR");
-- 
2.9.3


Re: [Qemu-devel] [PATCH] sockets: report error if UNIX socket path is too long
Posted by Eric Blake 6 years, 10 months ago
On 05/24/2017 10:42 AM, Daniel P. Berrange wrote:
> The 'struct sockaddr_un' only allows 108 bytes for the socket
> path. Currently QEMU uses snprintf() and so silently truncates
> the socket path provided by the user. This is undesirable because
> the user will then be unable to connect to the path they asked
> for. This change makes QEMU bounds check and report an explicit
> error message.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  util/qemu-sockets.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index d8183f7..e249dbe 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -855,6 +855,12 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
>      memset(&un, 0, sizeof(un));
>      un.sun_family = AF_UNIX;
>      if (saddr->path && strlen(saddr->path)) {
> +        if (strlen(saddr->path) > sizeof(un.sun_path)) {

Feels odd to be computing strlen(saddr->path) multiple times instead of
storing it in a temporary.

> +            error_setg(errp, "UNIX socket path '%s' is too long", saddr->path);
> +            error_append_hint(errp, "Path must be less than %zu bytes\n",
> +                              sizeof(un.sun_path));
> +            return -1;
> +        }
>          snprintf(un.sun_path, sizeof(un.sun_path), "%s", saddr->path);

Pre-existing, but now that we know things fit, isn't it faster to use
strcpy (or strpcpy or strncpy) instead of the overhead of snprintf?

You're on the right track, but it may be worth a v2 for further cleanups.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH] sockets: report error if UNIX socket path is too long
Posted by Eric Blake 6 years, 10 months ago
On 05/24/2017 10:54 AM, Eric Blake wrote:
> On 05/24/2017 10:42 AM, Daniel P. Berrange wrote:
>> The 'struct sockaddr_un' only allows 108 bytes for the socket
>> path. Currently QEMU uses snprintf() and so silently truncates
>> the socket path provided by the user. This is undesirable because
>> the user will then be unable to connect to the path they asked
>> for. This change makes QEMU bounds check and report an explicit
>> error message.
>>
>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>> ---
>>  util/qemu-sockets.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>

>>          snprintf(un.sun_path, sizeof(un.sun_path), "%s", saddr->path);
> 
> Pre-existing, but now that we know things fit, isn't it faster to use
> strcpy (or strpcpy or strncpy) instead of the overhead of snprintf?
> 
> You're on the right track, but it may be worth a v2 for further cleanups.

Oh, and while there, I noticed:

        tmpdir = tmpdir ? tmpdir : "/tmp";
        if (snprintf(un.sun_path, sizeof(un.sun_path),
"%s/qemu-socket-XXXXXX",
                     tmpdir) >= sizeof(un.sun_path)) {
            error_setg_errno(errp, errno,
                             "TMPDIR environment variable (%s) too
large", tmpdir);

but 'errno' is NOT required to be set when snprintf() returns a larger
value, so our error message is (likely) garbage.  Better might be
calling error_setg_errno(errp, ENAMETOOLONG, ...), or just living with
the simpler error_setg().

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org