[Qemu-devel] [PATCH] check return value of fcntl() to detect invalid fd

[Qemu-devel] [PATCH] check return value of fcntl() to detect invalid fd

[Amos Kong]

Passing some invalid fds in QEMU commandline, the fds don't exist.
QEMU will get error "TUNGETIFF ioctl() failed: Bad file descriptor",
and coredump in setting queues.

This patch checked return value of first operate to fd, QEMU will
report error and exit without coredump. It's effected for both netdev
fds and vhost_net fds.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 net/tap.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/net/tap.c b/net/tap.c
index bde6b58..039280a 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -688,7 +688,7 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
                  NetClientState *peer)
 {
     const NetdevTapOptions *tap;
-    int fd, vnet_hdr = 0, i = 0, queues;
+    int fd, vnet_hdr = 0, i = 0, queues, ret;
     /* for the no-fd, no-helper case */
     const char *script = NULL; /* suppress wrong "uninit'd use" gcc warning */
     const char *downscript = NULL;
@@ -722,7 +722,12 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
             return -1;
         }
 
-        fcntl(fd, F_SETFL, O_NONBLOCK);
+        ret = fcntl(fd, F_SETFL, O_NONBLOCK);
+        if (ret < 0) {
+            error_report("Fail to set file status to nonblock, "
+                         "%s", strerror(-ret));
+            return -1;
+        }
 
         vnet_hdr = tap_probe_vnet_hdr(fd);
 
@@ -761,7 +766,12 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
                 return -1;
             }
 
-            fcntl(fd, F_SETFL, O_NONBLOCK);
+            ret = fcntl(fd, F_SETFL, O_NONBLOCK);
+            if (ret < 0) {
+                error_report("Fail to set file status to nonblock, "
+                             "%s", strerror(-ret));
+                return -1;
+            }
 
             if (i == 0) {
                 vnet_hdr = tap_probe_vnet_hdr(fd);
-- 
2.1.0

Re: [Qemu-devel] [PATCH] check return value of fcntl() to detect invalid fd

[Eric Blake]

On 12/19/2014 06:25 AM, Amos Kong wrote:
> Passing some invalid fds in QEMU commandline, the fds don't exist.
> QEMU will get error "TUNGETIFF ioctl() failed: Bad file descriptor",
> and coredump in setting queues.
> 
> This patch checked return value of first operate to fd, QEMU will
> report error and exit without coredump. It's effected for both netdev

s/effected/needed/

> fds and vhost_net fds.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  net/tap.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 

>  
> -        fcntl(fd, F_SETFL, O_NONBLOCK);
> +        ret = fcntl(fd, F_SETFL, O_NONBLOCK);
> +        if (ret < 0) {
> +            error_report("Fail to set file status to nonblock, "
> +                         "%s", strerror(-ret));

Awkward grammar; I would suggest s/Fail/Failed/;
s/nonblock/nonblocking/, if you still end up reporting the error here.

Wrong, in multiple ways.  fcntl does not return negative errno values,
just -1, and strerror(1) is not what you want.  Furthermore, this code
is blindly clearing all other flags as part of setting O_NONBLOCK.  The
correct way to set O_NONBLOCK is to call F_GETFL first.  Which is what
qemu_set_nonblock() from oslib-posix.c already does (so reuse that
instead of reinventing it here).

Except _that_ function fails to report failure, so we need a bigger
audit to fix it and all callers.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Re: [Qemu-devel] [PATCH] check return value of fcntl() to detect invalid fd

[Jason Wang]

On 12/19/2014 09:25 PM, Amos Kong wrote:
> Passing some invalid fds in QEMU commandline, the fds don't exist.
> QEMU will get error "TUNGETIFF ioctl() failed: Bad file descriptor",
> and coredump in setting queues.
>
> This patch checked return value of first operate to fd, QEMU will
> report error and exit without coredump. It's effected for both netdev
> fds and vhost_net fds.
>
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  net/tap.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/net/tap.c b/net/tap.c
> index bde6b58..039280a 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -688,7 +688,7 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
>                   NetClientState *peer)
>  {
>      const NetdevTapOptions *tap;
> -    int fd, vnet_hdr = 0, i = 0, queues;
> +    int fd, vnet_hdr = 0, i = 0, queues, ret;
>      /* for the no-fd, no-helper case */
>      const char *script = NULL; /* suppress wrong "uninit'd use" gcc warning */
>      const char *downscript = NULL;
> @@ -722,7 +722,12 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
>              return -1;
>          }
>  
> -        fcntl(fd, F_SETFL, O_NONBLOCK);
> +        ret = fcntl(fd, F_SETFL, O_NONBLOCK);
> +        if (ret < 0) {
> +            error_report("Fail to set file status to nonblock, "
> +                         "%s", strerror(-ret));
> +            return -1;
> +        }

This may not work. There may be still some kinds of fd can pass this but
still fail at TUNGETIFF or other tun ioctls.

Probably you need to fail during TUNGETIFF, which can make sure it was
not a tap fd.
>  
>          vnet_hdr = tap_probe_vnet_hdr(fd);
>  
> @@ -761,7 +766,12 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
>                  return -1;
>              }
>  
> -            fcntl(fd, F_SETFL, O_NONBLOCK);
> +            ret = fcntl(fd, F_SETFL, O_NONBLOCK);
> +            if (ret < 0) {
> +                error_report("Fail to set file status to nonblock, "
> +                             "%s", strerror(-ret));
> +                return -1;
> +            }
>  
>              if (i == 0) {
>                  vnet_hdr = tap_probe_vnet_hdr(fd);

Re: [Qemu-devel] [PATCH] check return value of fcntl() to detect invalid fd

[Amos Kong]

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

On Mon, Dec 22, 2014 at 11:48:29AM +0800, Jason Wang wrote:
> 
> On 12/19/2014 09:25 PM, Amos Kong wrote:
> > Passing some invalid fds in QEMU commandline, the fds don't exist.
> > QEMU will get error "TUNGETIFF ioctl() failed: Bad file descriptor",
> > and coredump in setting queues.
> >
> > This patch checked return value of first operate to fd, QEMU will
> > report error and exit without coredump. It's effected for both netdev
> > fds and vhost_net fds.
> >
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> >  net/tap.c | 16 +++++++++++++---
> >  1 file changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/tap.c b/net/tap.c
> > index bde6b58..039280a 100644
> > --- a/net/tap.c
> > +++ b/net/tap.c
> > @@ -688,7 +688,7 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
> >                   NetClientState *peer)
> >  {
> >      const NetdevTapOptions *tap;
> > -    int fd, vnet_hdr = 0, i = 0, queues;
> > +    int fd, vnet_hdr = 0, i = 0, queues, ret;
> >      /* for the no-fd, no-helper case */
> >      const char *script = NULL; /* suppress wrong "uninit'd use" gcc warning */
> >      const char *downscript = NULL;
> > @@ -722,7 +722,12 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
> >              return -1;
> >          }
> >  
> > -        fcntl(fd, F_SETFL, O_NONBLOCK);
> > +        ret = fcntl(fd, F_SETFL, O_NONBLOCK);
> > +        if (ret < 0) {
> > +            error_report("Fail to set file status to nonblock, "
> > +                         "%s", strerror(-ret));
> > +            return -1;
> > +        }
> 
> This may not work. There may be still some kinds of fd can pass this but
> still fail at TUNGETIFF or other tun ioctls.

Early catching the error is better. This only help to check if the fd
exists.

 
> Probably you need to fail during TUNGETIFF, which can make sure it was
> not a tap fd.

Currently if ioctl fails, we treat the IFF_VNET_HDR flag isn't set.
We can return -1 in this case, and checking return value of tap_probe_vnet_hdr(),
and fail qemu.

qemu/net/tap-linux.c:
int tap_probe_vnet_hdr(int fd)
{
    struct ifreq ifr;

    if (ioctl(fd, TUNGETIFF, &ifr) != 0) {
        error_report("TUNGETIFF ioctl() failed: %s", strerror(errno));
        return 0;        <============
    }

    return ifr.ifr_flags & IFF_VNET_HDR;
}


I think we can fix tap_probe_vnet_hdr() and add checking return value of fcntl().

-- 
			Amos.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH] check return value of fcntl() to detect invalid fd

[Jason Wang]

On 12/22/2014 01:28 PM, Amos Kong wrote:
> On Mon, Dec 22, 2014 at 11:48:29AM +0800, Jason Wang wrote:
>> On 12/19/2014 09:25 PM, Amos Kong wrote:
>>> Passing some invalid fds in QEMU commandline, the fds don't exist.
>>> QEMU will get error "TUNGETIFF ioctl() failed: Bad file descriptor",
>>> and coredump in setting queues.
>>>
>>> This patch checked return value of first operate to fd, QEMU will
>>> report error and exit without coredump. It's effected for both netdev
>>> fds and vhost_net fds.
>>>
>>> Signed-off-by: Amos Kong <akong@redhat.com>
>>> ---
>>>  net/tap.c | 16 +++++++++++++---
>>>  1 file changed, 13 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/net/tap.c b/net/tap.c
>>> index bde6b58..039280a 100644
>>> --- a/net/tap.c
>>> +++ b/net/tap.c
>>> @@ -688,7 +688,7 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
>>>                   NetClientState *peer)
>>>  {
>>>      const NetdevTapOptions *tap;
>>> -    int fd, vnet_hdr = 0, i = 0, queues;
>>> +    int fd, vnet_hdr = 0, i = 0, queues, ret;
>>>      /* for the no-fd, no-helper case */
>>>      const char *script = NULL; /* suppress wrong "uninit'd use" gcc warning */
>>>      const char *downscript = NULL;
>>> @@ -722,7 +722,12 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
>>>              return -1;
>>>          }
>>>  
>>> -        fcntl(fd, F_SETFL, O_NONBLOCK);
>>> +        ret = fcntl(fd, F_SETFL, O_NONBLOCK);
>>> +        if (ret < 0) {
>>> +            error_report("Fail to set file status to nonblock, "
>>> +                         "%s", strerror(-ret));
>>> +            return -1;
>>> +        }
>> This may not work. There may be still some kinds of fd can pass this but
>> still fail at TUNGETIFF or other tun ioctls.
> Early catching the error is better. This only help to check if the fd
> exists.

If you just want to check the existence. Why don't you do it in
monitor_handle_fd_param() to let other case to benefit also? And
probably F_GETFL is better in this case.

But doing this does not solve the issue you mention in the commit log.
Even if fd exists, if it was not a tap fd, qemu may still abort.