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

[Amos Kong]

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 <address@hidden>
> > ---
> >  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.

signature.asc
Description: Digital signature