add fd limitations for avoiding a buffer overflow

FD_SET() and FD_CLR() are used to add and remove one descriptor from a
set, the 'fd' should be less than FD_SETSIZE. Glibc will give a warning
and crash the qemu when we set a fd (1024) to a set.

 # qemu -device virtio-net-pci,netdev=macvtap_netdev,mac=92:ff:8a:11:fe:57
   -netdev tap,id=macvtap_netdev,fd=1024 1024<>/dev/tap4

*** buffer overflow detected ***: x86_64-softmmu/qemu-system-x86_64
terminated

On 01/25/13 09:14, Amos Kong wrote:
> FD_SET() and FD_CLR() are used to add and remove one descriptor from a
> set, the 'fd' should be less than FD_SETSIZE. Glibc will give a warning
> and crash the qemu when we set a fd (1024) to a set.
> 
>  # qemu -device virtio-net-pci,netdev=macvtap_netdev,mac=92:ff:8a:11:fe:57
>    -netdev tap,id=macvtap_netdev,fd=1024 1024<>/dev/tap4
> 
> *** buffer overflow detected ***: x86_64-softmmu/qemu-system-x86_64
> terminated
> ======= Backtrace: =========
> /lib64/libc.so.6(__fortify_fail+0x37)[0x7f842a2134a7]
> /lib64/libc.so.6(+0x35e9d08620)[0x7f842a211620]
> /lib64/libc.so.6(+0x35e9d0a417)[0x7f842a213417]
> x86_64-softmmu/qemu-system-x86_64(+0x1901fd)[0x7f842f09f1fd]
> x86_64-softmmu/qemu-system-x86_64(+0x198388)[0x7f842f0a7388]
> x86_64-softmmu/qemu-system-x86_64(main+0xfa9)[0x7f842ef897a9]
> /lib64/libc.so.6(__libc_start_main+0xf5)[0x7f842a12aa05]
> x86_64-softmmu/qemu-system-x86_64(+0x7ed49)[0x7f842ef8dd49]
> ======= Memory map: ========
> ....
> 
> This patch added limitations when init tap device and set fd handler
> for synchronous IO.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  iohandler.c |    3 +++
>  net/tap.c   |    3 ++-
>  2 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/iohandler.c b/iohandler.c
> index 2523adc..c22edab 100644
> --- a/iohandler.c
> +++ b/iohandler.c
> @@ -66,6 +66,9 @@ int qemu_set_fd_handler2(int fd,
>              }
>          }
>      } else {
> +        if (fd >= FD_SETSIZE) {
> +            return 1;
> +        }

qemu_set_fd_handler2() -- and consequently, qemu_set_fd_handler() --
could never fail before. I tried to check their call sites, and most of
those don't bother to check for the return value; they assume these
functions always succeed.

Wouldn't it be better to abort() here (or exit with an error message)
instead of returning 1? (Not suggesting, just asking.)

Thanks,
Laszlo

>          QLIST_FOREACH(ioh, &io_handlers, next) {
>              if (ioh->fd == fd)
>                  goto found;
> diff --git a/net/tap.c b/net/tap.c
> index eb40c42..be856dd 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -618,7 +618,8 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
>          }
>  
>          fd = monitor_handle_fd_param(cur_mon, tap->fd);
> -        if (fd == -1) {
> +        if (fd == -1 || fd >= FD_SETSIZE) {
> +            error_report("Invalid fd : %d", fd);
>              return -1;
>          }
>

On 01/25/2013 04:14 PM, Amos Kong wrote:
> FD_SET() and FD_CLR() are used to add and remove one descriptor from a
> set, the 'fd' should be less than FD_SETSIZE. Glibc will give a warning
> and crash the qemu when we set a fd (1024) to a set.
>
>  # qemu -device virtio-net-pci,netdev=macvtap_netdev,mac=92:ff:8a:11:fe:57
>    -netdev tap,id=macvtap_netdev,fd=1024 1024<>/dev/tap4
>
> *** buffer overflow detected ***: x86_64-softmmu/qemu-system-x86_64
> terminated
> ======= Backtrace: =========
> /lib64/libc.so.6(__fortify_fail+0x37)[0x7f842a2134a7]
> /lib64/libc.so.6(+0x35e9d08620)[0x7f842a211620]
> /lib64/libc.so.6(+0x35e9d0a417)[0x7f842a213417]
> x86_64-softmmu/qemu-system-x86_64(+0x1901fd)[0x7f842f09f1fd]
> x86_64-softmmu/qemu-system-x86_64(+0x198388)[0x7f842f0a7388]
> x86_64-softmmu/qemu-system-x86_64(main+0xfa9)[0x7f842ef897a9]
> /lib64/libc.so.6(__libc_start_main+0xf5)[0x7f842a12aa05]
> x86_64-softmmu/qemu-system-x86_64(+0x7ed49)[0x7f842ef8dd49]
> ======= Memory map: ========
> ....
>
> This patch added limitations when init tap device and set fd handler
> for synchronous IO.
>
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  iohandler.c |    3 +++
>  net/tap.c   |    3 ++-
>  2 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/iohandler.c b/iohandler.c
> index 2523adc..c22edab 100644
> --- a/iohandler.c
> +++ b/iohandler.c
> @@ -66,6 +66,9 @@ int qemu_set_fd_handler2(int fd,
>              }
>          }
>      } else {
> +        if (fd >= FD_SETSIZE) {
> +            return 1;
> +        }
>          QLIST_FOREACH(ioh, &io_handlers, next) {
>              if (ioh->fd == fd)
>                  goto found;
> diff --git a/net/tap.c b/net/tap.c
> index eb40c42..be856dd 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -618,7 +618,8 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
>          }
>  
>          fd = monitor_handle_fd_param(cur_mon, tap->fd);
> -        if (fd == -1) {
> +        if (fd == -1 || fd >= FD_SETSIZE) {
> +            error_report("Invalid fd : %d", fd);
>              return -1;
>          }
>  

I think at least you need check all user of monitor_handler_fd_param()
or just does the check inside it.

Laszlo Ersek <lersek@redhat.com> writes:

> On 01/25/13 09:14, Amos Kong wrote:
>> FD_SET() and FD_CLR() are used to add and remove one descriptor from a
>> set, the 'fd' should be less than FD_SETSIZE. Glibc will give a warning
>> and crash the qemu when we set a fd (1024) to a set.
>> 
>>  # qemu -device virtio-net-pci,netdev=macvtap_netdev,mac=92:ff:8a:11:fe:57
>>    -netdev tap,id=macvtap_netdev,fd=1024 1024<>/dev/tap4
>> 
>> *** buffer overflow detected ***: x86_64-softmmu/qemu-system-x86_64
>> terminated
>> ======= Backtrace: =========
>> /lib64/libc.so.6(__fortify_fail+0x37)[0x7f842a2134a7]
>> /lib64/libc.so.6(+0x35e9d08620)[0x7f842a211620]
>> /lib64/libc.so.6(+0x35e9d0a417)[0x7f842a213417]
>> x86_64-softmmu/qemu-system-x86_64(+0x1901fd)[0x7f842f09f1fd]
>> x86_64-softmmu/qemu-system-x86_64(+0x198388)[0x7f842f0a7388]
>> x86_64-softmmu/qemu-system-x86_64(main+0xfa9)[0x7f842ef897a9]
>> /lib64/libc.so.6(__libc_start_main+0xf5)[0x7f842a12aa05]
>> x86_64-softmmu/qemu-system-x86_64(+0x7ed49)[0x7f842ef8dd49]
>> ======= Memory map: ========
>> ....
>> 
>> This patch added limitations when init tap device and set fd handler
>> for synchronous IO.
>> 
>> Signed-off-by: Amos Kong <akong@redhat.com>
>> ---
>>  iohandler.c |    3 +++
>>  net/tap.c   |    3 ++-
>>  2 files changed, 5 insertions(+), 1 deletions(-)
>> 
>> diff --git a/iohandler.c b/iohandler.c
>> index 2523adc..c22edab 100644
>> --- a/iohandler.c
>> +++ b/iohandler.c
>> @@ -66,6 +66,9 @@ int qemu_set_fd_handler2(int fd,
>>              }
>>          }
>>      } else {
>> +        if (fd >= FD_SETSIZE) {
>> +            return 1;
>> +        }
>
> qemu_set_fd_handler2() -- and consequently, qemu_set_fd_handler() --
> could never fail before. I tried to check their call sites, and most of
> those don't bother to check for the return value; they assume these
> functions always succeed.
>
> Wouldn't it be better to abort() here (or exit with an error message)
> instead of returning 1? (Not suggesting, just asking.)

Never check for an error you don't know how to handle ;-)

What we've done for other functions where some callers can't be bothered
to check for errors[*] is to create a wrapper void FOO_nofail() around
int FOO() that aborts on error, then slap QEMU_WARN_UNUSED_RESULT onto
FOO(), and fix the resulting warnings, either by adding error handling,
or by switching to FOO_nofail().


[*] Sometimes even legitimately, because we *know* errors can't happen.

On Fri, Jan 25, 2013 at 04:14:49PM +0800, Amos Kong wrote:
> FD_SET() and FD_CLR() are used to add and remove one descriptor from a
> set, the 'fd' should be less than FD_SETSIZE. Glibc will give a warning
> and crash the qemu when we set a fd (1024) to a set.
> 
>  # qemu -device virtio-net-pci,netdev=macvtap_netdev,mac=92:ff:8a:11:fe:57
>    -netdev tap,id=macvtap_netdev,fd=1024 1024<>/dev/tap4
> 
> *** buffer overflow detected ***: x86_64-softmmu/qemu-system-x86_64
> terminated
> ======= Backtrace: =========
> /lib64/libc.so.6(__fortify_fail+0x37)[0x7f842a2134a7]
> /lib64/libc.so.6(+0x35e9d08620)[0x7f842a211620]
> /lib64/libc.so.6(+0x35e9d0a417)[0x7f842a213417]
> x86_64-softmmu/qemu-system-x86_64(+0x1901fd)[0x7f842f09f1fd]
> x86_64-softmmu/qemu-system-x86_64(+0x198388)[0x7f842f0a7388]
> x86_64-softmmu/qemu-system-x86_64(main+0xfa9)[0x7f842ef897a9]
> /lib64/libc.so.6(__libc_start_main+0xf5)[0x7f842a12aa05]
> x86_64-softmmu/qemu-system-x86_64(+0x7ed49)[0x7f842ef8dd49]
> ======= Memory map: ========
> ....
> 
> This patch added limitations when init tap device and set fd handler
> for synchronous IO.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  iohandler.c |    3 +++
>  net/tap.c   |    3 ++-
>  2 files changed, 5 insertions(+), 1 deletions(-)

It would be much nicer to avoid fd_set than to add error code paths for
this API limitation.

With increased use of thread eventfds/pipe notification as seen in
vhost-net and virtio-blk-data-plane, the chance of really hitting 1024
file descriptors is growing.  I've already seen a PCI multifunction test
case where we exceed 1024 fds :(.


Anthony, Paolo: Is there an alternative to select(2)?  I think this was
discussed a bit during the glib event loop integration.

The two requirements I can think of are:

1. Portable so that we don't have to write OS-specific versions (epoll,
kqueue, etc).

2. Sub-millisecond timeouts.

Maybe we can use timerfd (and emulate it on non-Linux hosts) and then
fully use the glib event loop?

Internally glib prefers poll(2) but will fall back to select(2) on weird
OSes.  On Windows it has dedicated code.

Stefan

On Fri, Jan 25, 2013 at 04:14:49PM +0800, Amos Kong wrote:
> FD_SET() and FD_CLR() are used to add and remove one descriptor from a
> set, the 'fd' should be less than FD_SETSIZE. Glibc will give a warning
> and crash the qemu when we set a fd (1024) to a set.
> 
>  # qemu -device virtio-net-pci,netdev=macvtap_netdev,mac=92:ff:8a:11:fe:57
>    -netdev tap,id=macvtap_netdev,fd=1024 1024<>/dev/tap4
> 
> *** buffer overflow detected ***: x86_64-softmmu/qemu-system-x86_64
> terminated
> ======= Backtrace: =========
> /lib64/libc.so.6(__fortify_fail+0x37)[0x7f842a2134a7]
> /lib64/libc.so.6(+0x35e9d08620)[0x7f842a211620]
> /lib64/libc.so.6(+0x35e9d0a417)[0x7f842a213417]
> x86_64-softmmu/qemu-system-x86_64(+0x1901fd)[0x7f842f09f1fd]
> x86_64-softmmu/qemu-system-x86_64(+0x198388)[0x7f842f0a7388]
> x86_64-softmmu/qemu-system-x86_64(main+0xfa9)[0x7f842ef897a9]
> /lib64/libc.so.6(__libc_start_main+0xf5)[0x7f842a12aa05]
> x86_64-softmmu/qemu-system-x86_64(+0x7ed49)[0x7f842ef8dd49]
> ======= Memory map: ========
> ....

This is simply because we are using select for polling, right?

> 
> This patch added limitations when init tap device and set fd handler
> for synchronous IO.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>

> ---
>  iohandler.c |    3 +++
>  net/tap.c   |    3 ++-
>  2 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/iohandler.c b/iohandler.c
> index 2523adc..c22edab 100644
> --- a/iohandler.c
> +++ b/iohandler.c
> @@ -66,6 +66,9 @@ int qemu_set_fd_handler2(int fd,
>              }
>          }
>      } else {
> +        if (fd >= FD_SETSIZE) {
> +            return 1;
> +        }
>          QLIST_FOREACH(ioh, &io_handlers, next) {
>              if (ioh->fd == fd)
>                  goto found;
> diff --git a/net/tap.c b/net/tap.c
> index eb40c42..be856dd 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -618,7 +618,8 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
>          }
>  
>          fd = monitor_handle_fd_param(cur_mon, tap->fd);
> -        if (fd == -1) {
> +        if (fd == -1 || fd >= FD_SETSIZE) {
> +            error_report("Invalid fd : %d", fd);
>              return -1;
>          }

The fd is actually valid. It's simply too high.
And if this triggered it's quite possibly that
the fd passed in is 1023 but the moment we try to
allocate our own fd, it will be 1024 so boom.

So maybe, the right thing to do here is to use poll or epoll?

> -- 
> 1.7.1
>

On Sun, Jan 27, 2013 at 01:41:57PM +0200, Michael S. Tsirkin wrote:
> On Fri, Jan 25, 2013 at 04:14:49PM +0800, Amos Kong wrote:
> > FD_SET() and FD_CLR() are used to add and remove one descriptor from a
> > set, the 'fd' should be less than FD_SETSIZE. Glibc will give a warning
> > and crash the qemu when we set a fd (1024) to a set.
> > 
> >  # qemu -device virtio-net-pci,netdev=macvtap_netdev,mac=92:ff:8a:11:fe:57
> >    -netdev tap,id=macvtap_netdev,fd=1024 1024<>/dev/tap4
> > 
> > *** buffer overflow detected ***: x86_64-softmmu/qemu-system-x86_64
> > terminated
> > ======= Backtrace: =========
> > /lib64/libc.so.6(__fortify_fail+0x37)[0x7f842a2134a7]
> > /lib64/libc.so.6(+0x35e9d08620)[0x7f842a211620]
> > /lib64/libc.so.6(+0x35e9d0a417)[0x7f842a213417]
> > x86_64-softmmu/qemu-system-x86_64(+0x1901fd)[0x7f842f09f1fd]
> > x86_64-softmmu/qemu-system-x86_64(+0x198388)[0x7f842f0a7388]
> > x86_64-softmmu/qemu-system-x86_64(main+0xfa9)[0x7f842ef897a9]
> > /lib64/libc.so.6(__libc_start_main+0xf5)[0x7f842a12aa05]
> > x86_64-softmmu/qemu-system-x86_64(+0x7ed49)[0x7f842ef8dd49]
> > ======= Memory map: ========
> > ....
> 
> This is simply because we are using select for polling, right?
 
Yes.

> > This patch added limitations when init tap device and set fd handler
> > for synchronous IO.
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> 
> > ---
> >  iohandler.c |    3 +++
> >  net/tap.c   |    3 ++-
> >  2 files changed, 5 insertions(+), 1 deletions(-)
> > 
> > diff --git a/iohandler.c b/iohandler.c
> > index 2523adc..c22edab 100644
> > --- a/iohandler.c
> > +++ b/iohandler.c
> > @@ -66,6 +66,9 @@ int qemu_set_fd_handler2(int fd,
> >              }
> >          }
> >      } else {
> > +        if (fd >= FD_SETSIZE) {
> > +            return 1;
> > +        }
> >          QLIST_FOREACH(ioh, &io_handlers, next) {
> >              if (ioh->fd == fd)
> >                  goto found;
> > diff --git a/net/tap.c b/net/tap.c
> > index eb40c42..be856dd 100644
> > --- a/net/tap.c
> > +++ b/net/tap.c
> > @@ -618,7 +618,8 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
> >          }
> >  
> >          fd = monitor_handle_fd_param(cur_mon, tap->fd);
> > -        if (fd == -1) {
> > +        if (fd == -1 || fd >= FD_SETSIZE) {
> > +            error_report("Invalid fd : %d", fd);
> >              return -1;
> >          }
> 
> The fd is actually valid. It's simply too high.
> And if this triggered it's quite possibly that
> the fd passed in is 1023 but the moment we try to
> allocate our own fd, it will be 1024 so boom.
> 
> So maybe, the right thing to do here is to use poll or epoll?
 
Stefan posted some selected solution, let's see others comments.

Il 25/01/2013 15:30, Stefan Hajnoczi ha scritto:
> Anthony, Paolo: Is there an alternative to select(2)?  I think this was
> discussed a bit during the glib event loop integration.
> 
> The two requirements I can think of are:
> 
> 1. Portable so that we don't have to write OS-specific versions (epoll,
> kqueue, etc).
> 
> 2. Sub-millisecond timeouts.
> 
> Maybe we can use timerfd (and emulate it on non-Linux hosts) and then
> fully use the glib event loop?

Switching the alarm timer to a timerfd-based GSource is indeed easy.  We
do not absolutely need sub-millisecond timeouts; non-Linux hosts can
just use poll's millisecond resolution in the GSource.

However, perhaps it would be good also to move the timer mechanism to
AioContext.  Then qemu_new_timer becomes a special case of
aio_new_timer, just like qemu_bh_new.  This would let BlockDriverStates
have timers that fire during a qemu_aio_wait().  It would fix the busy
waiting in bdrv_drain_all(), and it would help threaded device models
too.  But more refactoring is needed for this.

> Internally glib prefers poll(2) but will fall back to select(2) on weird
> OSes.  On Windows it has dedicated code.

The main obstacle for poll() is slirp, which likes to do random accesses
on fd_sets.

Perhaps it's a good topic for tomorrow's call.

Paolo