Re: [Qemu-devel] [PATCH] add fd limitations for avoiding a buffer overflow
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
Re: [Qemu-devel] [PATCH] add fd limitations for avoiding a buffer overflow
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 > > > --- > > 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.
Re: [Qemu-devel] [PATCH] add fd limitations for avoiding a buffer overflow
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 > --- > 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 >
Re: [Qemu-devel] [PATCH] add fd limitations for avoiding a buffer overflow
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 > --- > 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
Re: [Qemu-devel] [PATCH] add fd limitations for avoiding a buffer overflow
Laszlo Ersek 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 >> --- >> 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.
Re: [Qemu-devel] [PATCH] add fd limitations for avoiding a buffer overflow
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 > --- > 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.
Re: [Qemu-devel] [PATCH] add fd limitations for avoiding a buffer overflow
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 > --- > 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; > } >
[Qemu-devel] [PATCH] 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 === 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 --- 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; } -- 1.7.1