Re: [Qemu-devel] [PATCH] add fd limitations for avoiding a buffer overflow

[Michael S. Tsirkin]

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

> ---
>  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
>