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

[Amos Kong]

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 <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?
 
Stefan posted some selected solution, let's see others comments.