* [PATCH v2 0/2] net: tap: check file descriptor can be used @ 2020-06-30 14:57 Laurent Vivier 2020-06-30 14:57 ` [PATCH v2 1/2] net: tap: check if the file descriptor is valid before using it Laurent Vivier ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Laurent Vivier @ 2020-06-30 14:57 UTC (permalink / raw) To: qemu-devel Cc: Laurent Vivier, Daniel P. Berrangé, Stefan Weil, Jason Wang, Markus Armbruster, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé v2: Add patch from Daniel to check the fd can be used I have updated Daniel's patch not to check for EINVAL on TUNGETIFF as I think we can avoid this special case because TUNGETIFF is available since kernel v2.6.27 (October 2008) Moreover I think the code was wrong as it was checking with -EINVAL and not EINVAL. Daniel P. Berrang�� (1): net: detect errors from probing vnet hdr flag for TAP devices Laurent Vivier (1): net: tap: check if the file descriptor is valid before using it include/qemu/sockets.h | 1 + net/tap-bsd.c | 2 +- net/tap-linux.c | 8 +++++--- net/tap-solaris.c | 2 +- net/tap-stub.c | 2 +- net/tap.c | 38 +++++++++++++++++++++++++++++++++----- net/tap_int.h | 2 +- util/oslib-posix.c | 5 +++++ util/oslib-win32.c | 6 ++++++ 9 files changed, 54 insertions(+), 12 deletions(-) -- 2.26.2 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/2] net: tap: check if the file descriptor is valid before using it 2020-06-30 14:57 [PATCH v2 0/2] net: tap: check file descriptor can be used Laurent Vivier @ 2020-06-30 14:57 ` Laurent Vivier 2020-06-30 15:04 ` Philippe Mathieu-Daudé ` (2 more replies) 2020-06-30 14:57 ` [PATCH v2 2/2] net: detect errors from probing vnet hdr flag for TAP devices Laurent Vivier 2020-06-30 15:12 ` [PATCH v2 0/2] net: tap: check file descriptor can be used Daniel P. Berrangé 2 siblings, 3 replies; 9+ messages in thread From: Laurent Vivier @ 2020-06-30 14:57 UTC (permalink / raw) To: qemu-devel Cc: Laurent Vivier, Daniel P. Berrangé, Stefan Weil, Jason Wang, Markus Armbruster, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé qemu_set_nonblock() checks that the file descriptor can be used and, if not, crashes QEMU. An assert() is used for that. The use of assert() is used to detect programming error and the coredump will allow to debug the problem. But in the case of the tap device, this assert() can be triggered by a misconfiguration by the user. At startup, it's not a real problem, but it can also happen during the hot-plug of a new device, and here it's a problem because we can crash a perfectly healthy system. For instance: # ip link add link virbr0 name macvtap0 type macvtap mode bridge # ip link set macvtap0 up # TAP=/dev/tap$(ip -o link show macvtap0 | cut -d: -f1) # qemu-system-x86_64 -machine q35 -device pcie-root-port,id=pcie-root-port-0 -monitor stdio 9<> $TAP (qemu) netdev_add type=tap,id=hostnet0,vhost=on,fd=9 (qemu) device_add driver=virtio-net-pci,netdev=hostnet0,id=net0,bus=pcie-root-port-0 (qemu) device_del net0 (qemu) netdev_del hostnet0 (qemu) netdev_add type=tap,id=hostnet1,vhost=on,fd=9 qemu-system-x86_64: .../util/oslib-posix.c:247: qemu_set_nonblock: Assertion `f != -1' failed. Aborted (core dumped) To avoid that, check the file descriptor is valid before passing it to qemu_set_non_block() for "fd=" and "fds=" parameters. Signed-off-by: Laurent Vivier <lvivier@redhat.com> --- include/qemu/sockets.h | 1 + net/tap.c | 13 +++++++++++++ util/oslib-posix.c | 5 +++++ util/oslib-win32.c | 6 ++++++ 4 files changed, 25 insertions(+) diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h index 57cd049d6edd..5b0c2d77ddad 100644 --- a/include/qemu/sockets.h +++ b/include/qemu/sockets.h @@ -17,6 +17,7 @@ int qemu_socket(int domain, int type, int protocol); int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen); int socket_set_cork(int fd, int v); int socket_set_nodelay(int fd); +bool qemu_fd_is_valid(int fd); void qemu_set_block(int fd); void qemu_set_nonblock(int fd); int socket_set_fast_reuse(int fd); diff --git a/net/tap.c b/net/tap.c index 6207f61f84ab..f65966aaccd8 100644 --- a/net/tap.c +++ b/net/tap.c @@ -795,6 +795,12 @@ int net_init_tap(const Netdev *netdev, const char *name, return -1; } + /* Check if fd is valid */ + if (!qemu_fd_is_valid(fd)) { + error_setg(errp, "Invalid file descriptor %d", fd); + return -1; + } + qemu_set_nonblock(fd); vnet_hdr = tap_probe_vnet_hdr(fd); @@ -843,6 +849,13 @@ int net_init_tap(const Netdev *netdev, const char *name, goto free_fail; } + /* Check if fd is valid */ + if (!qemu_fd_is_valid(fd)) { + error_setg(errp, "Invalid file descriptor %d", fd); + ret = -1; + goto free_fail; + } + qemu_set_nonblock(fd); if (i == 0) { diff --git a/util/oslib-posix.c b/util/oslib-posix.c index 916f1be2243a..8d5705f598d3 100644 --- a/util/oslib-posix.c +++ b/util/oslib-posix.c @@ -244,6 +244,11 @@ void qemu_anon_ram_free(void *ptr, size_t size) qemu_ram_munmap(-1, ptr, size); } +bool qemu_fd_is_valid(int fd) +{ + return fcntl(fd, F_GETFL) != -1; +} + void qemu_set_block(int fd) { int f; diff --git a/util/oslib-win32.c b/util/oslib-win32.c index e9b14ab17847..a6be9445cfdb 100644 --- a/util/oslib-win32.c +++ b/util/oslib-win32.c @@ -132,6 +132,12 @@ struct tm *localtime_r(const time_t *timep, struct tm *result) } #endif /* CONFIG_LOCALTIME_R */ +bool qemu_fd_is_valid(int fd) +{ + /* FIXME: how to check if fd is valid? */ + return true; +} + void qemu_set_block(int fd) { unsigned long opt = 0; -- 2.26.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] net: tap: check if the file descriptor is valid before using it 2020-06-30 14:57 ` [PATCH v2 1/2] net: tap: check if the file descriptor is valid before using it Laurent Vivier @ 2020-06-30 15:04 ` Philippe Mathieu-Daudé 2020-06-30 16:06 ` Greg Kurz 2020-07-01 5:50 ` Markus Armbruster 2 siblings, 0 replies; 9+ messages in thread From: Philippe Mathieu-Daudé @ 2020-06-30 15:04 UTC (permalink / raw) To: Laurent Vivier, qemu-devel Cc: Daniel P. Berrangé, Stefan Weil, Jason Wang, Markus Armbruster, Gerd Hoffmann, Paolo Bonzini On 6/30/20 4:57 PM, Laurent Vivier wrote: > qemu_set_nonblock() checks that the file descriptor can be used and, if > not, crashes QEMU. An assert() is used for that. The use of assert() is > used to detect programming error and the coredump will allow to debug > the problem. > > But in the case of the tap device, this assert() can be triggered by > a misconfiguration by the user. At startup, it's not a real problem, but it > can also happen during the hot-plug of a new device, and here it's a > problem because we can crash a perfectly healthy system. > > For instance: > # ip link add link virbr0 name macvtap0 type macvtap mode bridge > # ip link set macvtap0 up > # TAP=/dev/tap$(ip -o link show macvtap0 | cut -d: -f1) > # qemu-system-x86_64 -machine q35 -device pcie-root-port,id=pcie-root-port-0 -monitor stdio 9<> $TAP > (qemu) netdev_add type=tap,id=hostnet0,vhost=on,fd=9 > (qemu) device_add driver=virtio-net-pci,netdev=hostnet0,id=net0,bus=pcie-root-port-0 > (qemu) device_del net0 > (qemu) netdev_del hostnet0 > (qemu) netdev_add type=tap,id=hostnet1,vhost=on,fd=9 > qemu-system-x86_64: .../util/oslib-posix.c:247: qemu_set_nonblock: Assertion `f != -1' failed. > Aborted (core dumped) > > To avoid that, check the file descriptor is valid before passing it to > qemu_set_non_block() for "fd=" and "fds=" parameters. > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > include/qemu/sockets.h | 1 + > net/tap.c | 13 +++++++++++++ > util/oslib-posix.c | 5 +++++ > util/oslib-win32.c | 6 ++++++ > 4 files changed, 25 insertions(+) > > diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h > index 57cd049d6edd..5b0c2d77ddad 100644 > --- a/include/qemu/sockets.h > +++ b/include/qemu/sockets.h > @@ -17,6 +17,7 @@ int qemu_socket(int domain, int type, int protocol); > int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen); > int socket_set_cork(int fd, int v); > int socket_set_nodelay(int fd); > +bool qemu_fd_is_valid(int fd); > void qemu_set_block(int fd); > void qemu_set_nonblock(int fd); > int socket_set_fast_reuse(int fd); > diff --git a/net/tap.c b/net/tap.c > index 6207f61f84ab..f65966aaccd8 100644 > --- a/net/tap.c > +++ b/net/tap.c > @@ -795,6 +795,12 @@ int net_init_tap(const Netdev *netdev, const char *name, > return -1; > } > > + /* Check if fd is valid */ > + if (!qemu_fd_is_valid(fd)) { > + error_setg(errp, "Invalid file descriptor %d", fd); > + return -1; > + } > + > qemu_set_nonblock(fd); > > vnet_hdr = tap_probe_vnet_hdr(fd); > @@ -843,6 +849,13 @@ int net_init_tap(const Netdev *netdev, const char *name, > goto free_fail; > } > > + /* Check if fd is valid */ > + if (!qemu_fd_is_valid(fd)) { > + error_setg(errp, "Invalid file descriptor %d", fd); > + ret = -1; > + goto free_fail; > + } > + > qemu_set_nonblock(fd); > > if (i == 0) { > diff --git a/util/oslib-posix.c b/util/oslib-posix.c > index 916f1be2243a..8d5705f598d3 100644 > --- a/util/oslib-posix.c > +++ b/util/oslib-posix.c > @@ -244,6 +244,11 @@ void qemu_anon_ram_free(void *ptr, size_t size) > qemu_ram_munmap(-1, ptr, size); > } > > +bool qemu_fd_is_valid(int fd) > +{ > + return fcntl(fd, F_GETFL) != -1; > +} > + > void qemu_set_block(int fd) > { > int f; > diff --git a/util/oslib-win32.c b/util/oslib-win32.c > index e9b14ab17847..a6be9445cfdb 100644 > --- a/util/oslib-win32.c > +++ b/util/oslib-win32.c > @@ -132,6 +132,12 @@ struct tm *localtime_r(const time_t *timep, struct tm *result) > } > #endif /* CONFIG_LOCALTIME_R */ > > +bool qemu_fd_is_valid(int fd) > +{ > + /* FIXME: how to check if fd is valid? */ > + return true; > +} > + > void qemu_set_block(int fd) > { > unsigned long opt = 0; > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] net: tap: check if the file descriptor is valid before using it 2020-06-30 14:57 ` [PATCH v2 1/2] net: tap: check if the file descriptor is valid before using it Laurent Vivier 2020-06-30 15:04 ` Philippe Mathieu-Daudé @ 2020-06-30 16:06 ` Greg Kurz 2020-06-30 16:46 ` Laurent Vivier 2020-07-01 5:50 ` Markus Armbruster 2 siblings, 1 reply; 9+ messages in thread From: Greg Kurz @ 2020-06-30 16:06 UTC (permalink / raw) To: Laurent Vivier Cc: Daniel P. Berrangé, Stefan Weil, Jason Wang, qemu-devel, Markus Armbruster, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé On Tue, 30 Jun 2020 16:57:36 +0200 Laurent Vivier <lvivier@redhat.com> wrote: > qemu_set_nonblock() checks that the file descriptor can be used and, if > not, crashes QEMU. An assert() is used for that. The use of assert() is > used to detect programming error and the coredump will allow to debug > the problem. > > But in the case of the tap device, this assert() can be triggered by > a misconfiguration by the user. At startup, it's not a real problem, but it > can also happen during the hot-plug of a new device, and here it's a > problem because we can crash a perfectly healthy system. > > For instance: > # ip link add link virbr0 name macvtap0 type macvtap mode bridge > # ip link set macvtap0 up > # TAP=/dev/tap$(ip -o link show macvtap0 | cut -d: -f1) > # qemu-system-x86_64 -machine q35 -device pcie-root-port,id=pcie-root-port-0 -monitor stdio 9<> $TAP > (qemu) netdev_add type=tap,id=hostnet0,vhost=on,fd=9 > (qemu) device_add driver=virtio-net-pci,netdev=hostnet0,id=net0,bus=pcie-root-port-0 > (qemu) device_del net0 > (qemu) netdev_del hostnet0 > (qemu) netdev_add type=tap,id=hostnet1,vhost=on,fd=9 > qemu-system-x86_64: .../util/oslib-posix.c:247: qemu_set_nonblock: Assertion `f != -1' failed. > Aborted (core dumped) > > To avoid that, check the file descriptor is valid before passing it to > qemu_set_non_block() for "fd=" and "fds=" parameters. > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > --- > include/qemu/sockets.h | 1 + > net/tap.c | 13 +++++++++++++ > util/oslib-posix.c | 5 +++++ > util/oslib-win32.c | 6 ++++++ > 4 files changed, 25 insertions(+) > > diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h > index 57cd049d6edd..5b0c2d77ddad 100644 > --- a/include/qemu/sockets.h > +++ b/include/qemu/sockets.h > @@ -17,6 +17,7 @@ int qemu_socket(int domain, int type, int protocol); > int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen); > int socket_set_cork(int fd, int v); > int socket_set_nodelay(int fd); > +bool qemu_fd_is_valid(int fd); > void qemu_set_block(int fd); > void qemu_set_nonblock(int fd); > int socket_set_fast_reuse(int fd); > diff --git a/net/tap.c b/net/tap.c > index 6207f61f84ab..f65966aaccd8 100644 > --- a/net/tap.c > +++ b/net/tap.c > @@ -795,6 +795,12 @@ int net_init_tap(const Netdev *netdev, const char *name, > return -1; > } > > + /* Check if fd is valid */ > + if (!qemu_fd_is_valid(fd)) { > + error_setg(errp, "Invalid file descriptor %d", fd); > + return -1; > + } > + > qemu_set_nonblock(fd); > > vnet_hdr = tap_probe_vnet_hdr(fd); > @@ -843,6 +849,13 @@ int net_init_tap(const Netdev *netdev, const char *name, > goto free_fail; > } > > + /* Check if fd is valid */ > + if (!qemu_fd_is_valid(fd)) { > + error_setg(errp, "Invalid file descriptor %d", fd); > + ret = -1; > + goto free_fail; > + } > + > qemu_set_nonblock(fd); > > if (i == 0) { > diff --git a/util/oslib-posix.c b/util/oslib-posix.c > index 916f1be2243a..8d5705f598d3 100644 > --- a/util/oslib-posix.c > +++ b/util/oslib-posix.c > @@ -244,6 +244,11 @@ void qemu_anon_ram_free(void *ptr, size_t size) > qemu_ram_munmap(-1, ptr, size); > } > > +bool qemu_fd_is_valid(int fd) > +{ > + return fcntl(fd, F_GETFL) != -1; > +} > + > void qemu_set_block(int fd) > { > int f; > diff --git a/util/oslib-win32.c b/util/oslib-win32.c > index e9b14ab17847..a6be9445cfdb 100644 > --- a/util/oslib-win32.c > +++ b/util/oslib-win32.c > @@ -132,6 +132,12 @@ struct tm *localtime_r(const time_t *timep, struct tm *result) > } > #endif /* CONFIG_LOCALTIME_R */ > > +bool qemu_fd_is_valid(int fd) > +{ > + /* FIXME: how to check if fd is valid? */ > + return true; > +} > + Maybe the following ? bool qemu_fd_is_valid(int fd) { return _get_osfhandle(fd) != INVALID_HANDLE_VALUE; } https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/get-osfhandle?view=vs-2019 Anyway, Reviewed-by: Greg Kurz <groug@kaod.org> > void qemu_set_block(int fd) > { > unsigned long opt = 0; ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] net: tap: check if the file descriptor is valid before using it 2020-06-30 16:06 ` Greg Kurz @ 2020-06-30 16:46 ` Laurent Vivier 0 siblings, 0 replies; 9+ messages in thread From: Laurent Vivier @ 2020-06-30 16:46 UTC (permalink / raw) To: Greg Kurz Cc: Daniel P. Berrangé, Stefan Weil, Jason Wang, qemu-devel, Markus Armbruster, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé On 30/06/2020 18:06, Greg Kurz wrote: > On Tue, 30 Jun 2020 16:57:36 +0200 > Laurent Vivier <lvivier@redhat.com> wrote: > >> qemu_set_nonblock() checks that the file descriptor can be used and, if >> not, crashes QEMU. An assert() is used for that. The use of assert() is >> used to detect programming error and the coredump will allow to debug >> the problem. >> >> But in the case of the tap device, this assert() can be triggered by >> a misconfiguration by the user. At startup, it's not a real problem, but it >> can also happen during the hot-plug of a new device, and here it's a >> problem because we can crash a perfectly healthy system. >> >> For instance: >> # ip link add link virbr0 name macvtap0 type macvtap mode bridge >> # ip link set macvtap0 up >> # TAP=/dev/tap$(ip -o link show macvtap0 | cut -d: -f1) >> # qemu-system-x86_64 -machine q35 -device pcie-root-port,id=pcie-root-port-0 -monitor stdio 9<> $TAP >> (qemu) netdev_add type=tap,id=hostnet0,vhost=on,fd=9 >> (qemu) device_add driver=virtio-net-pci,netdev=hostnet0,id=net0,bus=pcie-root-port-0 >> (qemu) device_del net0 >> (qemu) netdev_del hostnet0 >> (qemu) netdev_add type=tap,id=hostnet1,vhost=on,fd=9 >> qemu-system-x86_64: .../util/oslib-posix.c:247: qemu_set_nonblock: Assertion `f != -1' failed. >> Aborted (core dumped) >> >> To avoid that, check the file descriptor is valid before passing it to >> qemu_set_non_block() for "fd=" and "fds=" parameters. >> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >> --- >> include/qemu/sockets.h | 1 + >> net/tap.c | 13 +++++++++++++ >> util/oslib-posix.c | 5 +++++ >> util/oslib-win32.c | 6 ++++++ >> 4 files changed, 25 insertions(+) >> >> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h >> index 57cd049d6edd..5b0c2d77ddad 100644 >> --- a/include/qemu/sockets.h >> +++ b/include/qemu/sockets.h >> @@ -17,6 +17,7 @@ int qemu_socket(int domain, int type, int protocol); >> int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen); >> int socket_set_cork(int fd, int v); >> int socket_set_nodelay(int fd); >> +bool qemu_fd_is_valid(int fd); >> void qemu_set_block(int fd); >> void qemu_set_nonblock(int fd); >> int socket_set_fast_reuse(int fd); >> diff --git a/net/tap.c b/net/tap.c >> index 6207f61f84ab..f65966aaccd8 100644 >> --- a/net/tap.c >> +++ b/net/tap.c >> @@ -795,6 +795,12 @@ int net_init_tap(const Netdev *netdev, const char *name, >> return -1; >> } >> >> + /* Check if fd is valid */ >> + if (!qemu_fd_is_valid(fd)) { >> + error_setg(errp, "Invalid file descriptor %d", fd); >> + return -1; >> + } >> + >> qemu_set_nonblock(fd); >> >> vnet_hdr = tap_probe_vnet_hdr(fd); >> @@ -843,6 +849,13 @@ int net_init_tap(const Netdev *netdev, const char *name, >> goto free_fail; >> } >> >> + /* Check if fd is valid */ >> + if (!qemu_fd_is_valid(fd)) { >> + error_setg(errp, "Invalid file descriptor %d", fd); >> + ret = -1; >> + goto free_fail; >> + } >> + >> qemu_set_nonblock(fd); >> >> if (i == 0) { >> diff --git a/util/oslib-posix.c b/util/oslib-posix.c >> index 916f1be2243a..8d5705f598d3 100644 >> --- a/util/oslib-posix.c >> +++ b/util/oslib-posix.c >> @@ -244,6 +244,11 @@ void qemu_anon_ram_free(void *ptr, size_t size) >> qemu_ram_munmap(-1, ptr, size); >> } >> >> +bool qemu_fd_is_valid(int fd) >> +{ >> + return fcntl(fd, F_GETFL) != -1; >> +} >> + >> void qemu_set_block(int fd) >> { >> int f; >> diff --git a/util/oslib-win32.c b/util/oslib-win32.c >> index e9b14ab17847..a6be9445cfdb 100644 >> --- a/util/oslib-win32.c >> +++ b/util/oslib-win32.c >> @@ -132,6 +132,12 @@ struct tm *localtime_r(const time_t *timep, struct tm *result) >> } >> #endif /* CONFIG_LOCALTIME_R */ >> >> +bool qemu_fd_is_valid(int fd) >> +{ >> + /* FIXME: how to check if fd is valid? */ >> + return true; >> +} >> + > > Maybe the following ? > > bool qemu_fd_is_valid(int fd) > { > return _get_osfhandle(fd) != INVALID_HANDLE_VALUE; > } > > https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/get-osfhandle?view=vs-2019 For the v1, Philippe proposed: bool qemu_fd_is_valid(int fd) { unsigned long res; /* ignored */ return ioctlsocket(fd, FIONREAD, &res) == NO_ERROR; } https://docs.microsoft.com/en-us/windows/win32/winsock/winsock-ioctls The problem is I can't test. So I let the change to someone that will need it, fix it and test it... Thanks, Laurent ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] net: tap: check if the file descriptor is valid before using it 2020-06-30 14:57 ` [PATCH v2 1/2] net: tap: check if the file descriptor is valid before using it Laurent Vivier 2020-06-30 15:04 ` Philippe Mathieu-Daudé 2020-06-30 16:06 ` Greg Kurz @ 2020-07-01 5:50 ` Markus Armbruster 2020-07-01 9:31 ` Laurent Vivier 2 siblings, 1 reply; 9+ messages in thread From: Markus Armbruster @ 2020-07-01 5:50 UTC (permalink / raw) To: Laurent Vivier Cc: Daniel P. Berrangé, Stefan Weil, Jason Wang, qemu-devel, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé Laurent Vivier <lvivier@redhat.com> writes: > qemu_set_nonblock() checks that the file descriptor can be used and, if > not, crashes QEMU. An assert() is used for that. The use of assert() is > used to detect programming error and the coredump will allow to debug > the problem. > > But in the case of the tap device, this assert() can be triggered by > a misconfiguration by the user. At startup, it's not a real problem, but it > can also happen during the hot-plug of a new device, and here it's a > problem because we can crash a perfectly healthy system. > > For instance: > # ip link add link virbr0 name macvtap0 type macvtap mode bridge > # ip link set macvtap0 up > # TAP=/dev/tap$(ip -o link show macvtap0 | cut -d: -f1) > # qemu-system-x86_64 -machine q35 -device pcie-root-port,id=pcie-root-port-0 -monitor stdio 9<> $TAP > (qemu) netdev_add type=tap,id=hostnet0,vhost=on,fd=9 > (qemu) device_add driver=virtio-net-pci,netdev=hostnet0,id=net0,bus=pcie-root-port-0 > (qemu) device_del net0 > (qemu) netdev_del hostnet0 > (qemu) netdev_add type=tap,id=hostnet1,vhost=on,fd=9 > qemu-system-x86_64: .../util/oslib-posix.c:247: qemu_set_nonblock: Assertion `f != -1' failed. > Aborted (core dumped) > > To avoid that, check the file descriptor is valid before passing it to > qemu_set_non_block() for "fd=" and "fds=" parameters. > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > --- > include/qemu/sockets.h | 1 + > net/tap.c | 13 +++++++++++++ > util/oslib-posix.c | 5 +++++ > util/oslib-win32.c | 6 ++++++ > 4 files changed, 25 insertions(+) > > diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h > index 57cd049d6edd..5b0c2d77ddad 100644 > --- a/include/qemu/sockets.h > +++ b/include/qemu/sockets.h > @@ -17,6 +17,7 @@ int qemu_socket(int domain, int type, int protocol); > int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen); > int socket_set_cork(int fd, int v); > int socket_set_nodelay(int fd); > +bool qemu_fd_is_valid(int fd); > void qemu_set_block(int fd); > void qemu_set_nonblock(int fd); > int socket_set_fast_reuse(int fd); > diff --git a/net/tap.c b/net/tap.c > index 6207f61f84ab..f65966aaccd8 100644 > --- a/net/tap.c > +++ b/net/tap.c > @@ -795,6 +795,12 @@ int net_init_tap(const Netdev *netdev, const char *name, > return -1; > } > > + /* Check if fd is valid */ > + if (!qemu_fd_is_valid(fd)) { > + error_setg(errp, "Invalid file descriptor %d", fd); > + return -1; > + } > + > qemu_set_nonblock(fd); > > vnet_hdr = tap_probe_vnet_hdr(fd); > @@ -843,6 +849,13 @@ int net_init_tap(const Netdev *netdev, const char *name, > goto free_fail; > } > > + /* Check if fd is valid */ > + if (!qemu_fd_is_valid(fd)) { > + error_setg(errp, "Invalid file descriptor %d", fd); > + ret = -1; > + goto free_fail; > + } > + > qemu_set_nonblock(fd); > > if (i == 0) { > diff --git a/util/oslib-posix.c b/util/oslib-posix.c > index 916f1be2243a..8d5705f598d3 100644 > --- a/util/oslib-posix.c > +++ b/util/oslib-posix.c > @@ -244,6 +244,11 @@ void qemu_anon_ram_free(void *ptr, size_t size) > qemu_ram_munmap(-1, ptr, size); > } > > +bool qemu_fd_is_valid(int fd) > +{ > + return fcntl(fd, F_GETFL) != -1; > +} > + > void qemu_set_block(int fd) > { > int f; > diff --git a/util/oslib-win32.c b/util/oslib-win32.c > index e9b14ab17847..a6be9445cfdb 100644 > --- a/util/oslib-win32.c > +++ b/util/oslib-win32.c > @@ -132,6 +132,12 @@ struct tm *localtime_r(const time_t *timep, struct tm *result) > } > #endif /* CONFIG_LOCALTIME_R */ > > +bool qemu_fd_is_valid(int fd) > +{ > + /* FIXME: how to check if fd is valid? */ Please explain the FIXME's impact in the commit message. > + return true; > +} > + > void qemu_set_block(int fd) > { > unsigned long opt = 0; This patch is okay as it is. But I'd solve the problem differently, avoiding the FIXME: have qemu_try_set_block() return -errno on failure, have qemu_set_block() wrap around it and assert it succeeds. Then in net_init_tap(): ret = qemu_try_set_block(fd); if (ret < 0) { error_setg_errno(errp, -ret, "Can't use file descriptor %d", fd); } When @fd is bad (say -1), @ret is set to EBADF, and the error message looks like Can't use file descriptor -1: Bad file descriptor You should of course test the error message to see whether it makes sense even with a complex command line. If it doesn't, having it mention @name could perhaps help. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] net: tap: check if the file descriptor is valid before using it 2020-07-01 5:50 ` Markus Armbruster @ 2020-07-01 9:31 ` Laurent Vivier 0 siblings, 0 replies; 9+ messages in thread From: Laurent Vivier @ 2020-07-01 9:31 UTC (permalink / raw) To: Markus Armbruster Cc: Daniel P. Berrangé, Stefan Weil, Jason Wang, qemu-devel, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé On 01/07/2020 07:50, Markus Armbruster wrote: > Laurent Vivier <lvivier@redhat.com> writes: > >> qemu_set_nonblock() checks that the file descriptor can be used and, if >> not, crashes QEMU. An assert() is used for that. The use of assert() is >> used to detect programming error and the coredump will allow to debug >> the problem. >> >> But in the case of the tap device, this assert() can be triggered by >> a misconfiguration by the user. At startup, it's not a real problem, but it >> can also happen during the hot-plug of a new device, and here it's a >> problem because we can crash a perfectly healthy system. >> >> For instance: >> # ip link add link virbr0 name macvtap0 type macvtap mode bridge >> # ip link set macvtap0 up >> # TAP=/dev/tap$(ip -o link show macvtap0 | cut -d: -f1) >> # qemu-system-x86_64 -machine q35 -device pcie-root-port,id=pcie-root-port-0 -monitor stdio 9<> $TAP >> (qemu) netdev_add type=tap,id=hostnet0,vhost=on,fd=9 >> (qemu) device_add driver=virtio-net-pci,netdev=hostnet0,id=net0,bus=pcie-root-port-0 >> (qemu) device_del net0 >> (qemu) netdev_del hostnet0 >> (qemu) netdev_add type=tap,id=hostnet1,vhost=on,fd=9 >> qemu-system-x86_64: .../util/oslib-posix.c:247: qemu_set_nonblock: Assertion `f != -1' failed. >> Aborted (core dumped) >> >> To avoid that, check the file descriptor is valid before passing it to >> qemu_set_non_block() for "fd=" and "fds=" parameters. >> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >> --- >> include/qemu/sockets.h | 1 + >> net/tap.c | 13 +++++++++++++ >> util/oslib-posix.c | 5 +++++ >> util/oslib-win32.c | 6 ++++++ >> 4 files changed, 25 insertions(+) >> >> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h >> index 57cd049d6edd..5b0c2d77ddad 100644 >> --- a/include/qemu/sockets.h >> +++ b/include/qemu/sockets.h >> @@ -17,6 +17,7 @@ int qemu_socket(int domain, int type, int protocol); >> int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen); >> int socket_set_cork(int fd, int v); >> int socket_set_nodelay(int fd); >> +bool qemu_fd_is_valid(int fd); >> void qemu_set_block(int fd); >> void qemu_set_nonblock(int fd); >> int socket_set_fast_reuse(int fd); >> diff --git a/net/tap.c b/net/tap.c >> index 6207f61f84ab..f65966aaccd8 100644 >> --- a/net/tap.c >> +++ b/net/tap.c >> @@ -795,6 +795,12 @@ int net_init_tap(const Netdev *netdev, const char *name, >> return -1; >> } >> >> + /* Check if fd is valid */ >> + if (!qemu_fd_is_valid(fd)) { >> + error_setg(errp, "Invalid file descriptor %d", fd); >> + return -1; >> + } >> + >> qemu_set_nonblock(fd); >> >> vnet_hdr = tap_probe_vnet_hdr(fd); >> @@ -843,6 +849,13 @@ int net_init_tap(const Netdev *netdev, const char *name, >> goto free_fail; >> } >> >> + /* Check if fd is valid */ >> + if (!qemu_fd_is_valid(fd)) { >> + error_setg(errp, "Invalid file descriptor %d", fd); >> + ret = -1; >> + goto free_fail; >> + } >> + >> qemu_set_nonblock(fd); >> >> if (i == 0) { >> diff --git a/util/oslib-posix.c b/util/oslib-posix.c >> index 916f1be2243a..8d5705f598d3 100644 >> --- a/util/oslib-posix.c >> +++ b/util/oslib-posix.c >> @@ -244,6 +244,11 @@ void qemu_anon_ram_free(void *ptr, size_t size) >> qemu_ram_munmap(-1, ptr, size); >> } >> >> +bool qemu_fd_is_valid(int fd) >> +{ >> + return fcntl(fd, F_GETFL) != -1; >> +} >> + >> void qemu_set_block(int fd) >> { >> int f; >> diff --git a/util/oslib-win32.c b/util/oslib-win32.c >> index e9b14ab17847..a6be9445cfdb 100644 >> --- a/util/oslib-win32.c >> +++ b/util/oslib-win32.c >> @@ -132,6 +132,12 @@ struct tm *localtime_r(const time_t *timep, struct tm *result) >> } >> #endif /* CONFIG_LOCALTIME_R */ >> >> +bool qemu_fd_is_valid(int fd) >> +{ >> + /* FIXME: how to check if fd is valid? */ > > Please explain the FIXME's impact in the commit message. > >> + return true; >> +} >> + >> void qemu_set_block(int fd) >> { >> unsigned long opt = 0; > > This patch is okay as it is. But I'd solve the problem differently, > avoiding the FIXME: have qemu_try_set_block() return -errno on failure, > have qemu_set_block() wrap around it and assert it succeeds. Then in > net_init_tap(): > > ret = qemu_try_set_block(fd); > if (ret < 0) { > error_setg_errno(errp, -ret, "Can't use file descriptor %d", > fd); > } > > When @fd is bad (say -1), @ret is set to EBADF, and the error message > looks like > > Can't use file descriptor -1: Bad file descriptor > > You should of course test the error message to see whether it makes > sense even with a complex command line. If it doesn't, having > it mention @name could perhaps help. > Yes, it's a good idea. I'm going to do that. Thanks, Laurent ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 2/2] net: detect errors from probing vnet hdr flag for TAP devices 2020-06-30 14:57 [PATCH v2 0/2] net: tap: check file descriptor can be used Laurent Vivier 2020-06-30 14:57 ` [PATCH v2 1/2] net: tap: check if the file descriptor is valid before using it Laurent Vivier @ 2020-06-30 14:57 ` Laurent Vivier 2020-06-30 15:12 ` [PATCH v2 0/2] net: tap: check file descriptor can be used Daniel P. Berrangé 2 siblings, 0 replies; 9+ messages in thread From: Laurent Vivier @ 2020-06-30 14:57 UTC (permalink / raw) To: qemu-devel Cc: Laurent Vivier, Daniel P. Berrangé, Stefan Weil, Jason Wang, Markus Armbruster, Dr . David Alan Gilbert, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé From: "Daniel P. Berrange" <berrange@redhat.com> When QEMU sets up a tap based network device backend, it mostly ignores errors reported from various ioctl() calls it makes, assuming the TAP file descriptor is valid. This assumption can easily be violated when the user is passing in a pre-opened file descriptor. At best, the ioctls may fail with a -EBADF, but if the user passes in a bogus FD number that happens to clash with a FD number that QEMU has opened internally for another reason, a wide variety of errnos may result, as the TUNGETIFF ioctl number may map to a completely different command on a different type of file. By ignoring all these errors, QEMU sets up a zombie network backend that will never pass any data. Even worse, when QEMU shuts down, or that network backend is hot-removed, it will close this bogus file descriptor, which could belong to another QEMU device backend. There's no obvious guaranteed reliable way to detect that a FD genuinely is a TAP device, as opposed to a UNIX socket, or pipe, or something else. Checking the errno from probing vnet hdr flag though, does catch the big common cases. ie calling TUNGETIFF will return EBADF for an invalid FD, and ENOTTY when FD is a UNIX socket, or pipe which catches accidental collisions with FDs used for stdio, or monitor socket. Previously the example below where bogus fd 9 collides with the FD used for the chardev saw: $ ./x86_64-softmmu/qemu-system-x86_64 -netdev tap,id=hostnet0,fd=9 \ -chardev socket,id=charchannel0,path=/tmp/qga,server,nowait \ -monitor stdio -vnc :0 qemu-system-x86_64: -netdev tap,id=hostnet0,fd=9: TUNGETIFF ioctl() failed: Inappropriate ioctl for device TUNSETOFFLOAD ioctl() failed: Bad address QEMU 2.9.1 monitor - type 'help' for more information (qemu) Warning: netdev hostnet0 has no peer which gives a running QEMU with a zombie network backend. With this change applied we get an error message and QEMU immediately exits before carrying on and making a bigger disaster: $ ./x86_64-softmmu/qemu-system-x86_64 -netdev tap,id=hostnet0,fd=9 \ -chardev socket,id=charchannel0,path=/tmp/qga,server,nowait \ -monitor stdio -vnc :0 qemu-system-x86_64: -netdev tap,id=hostnet0,vhost=on,fd=9: Unable to query TUNGETIFF on FD 9: Inappropriate ioctl for device Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Signed-off-by: Daniel P. Berrange <berrange@redhat.com> Tested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Message-id: 20171027085548.3472-1-berrange@redhat.com [lv: to simplify, don't check on EINVAL with TUNGETIFF as it exists since v2.6.27] Signed-off-by: Laurent Vivier <lvivier@redhat.com> --- net/tap-bsd.c | 2 +- net/tap-linux.c | 8 +++++--- net/tap-solaris.c | 2 +- net/tap-stub.c | 2 +- net/tap.c | 25 ++++++++++++++++++++----- net/tap_int.h | 2 +- 6 files changed, 29 insertions(+), 12 deletions(-) diff --git a/net/tap-bsd.c b/net/tap-bsd.c index a5c3707f806d..77aaf674b19d 100644 --- a/net/tap-bsd.c +++ b/net/tap-bsd.c @@ -211,7 +211,7 @@ void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp) { } -int tap_probe_vnet_hdr(int fd) +int tap_probe_vnet_hdr(int fd, Error **errp) { return 0; } diff --git a/net/tap-linux.c b/net/tap-linux.c index e0dd442ee34f..b0635e9e32ce 100644 --- a/net/tap-linux.c +++ b/net/tap-linux.c @@ -147,13 +147,15 @@ void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp) } } -int tap_probe_vnet_hdr(int fd) +int tap_probe_vnet_hdr(int fd, Error **errp) { struct ifreq ifr; if (ioctl(fd, TUNGETIFF, &ifr) != 0) { - error_report("TUNGETIFF ioctl() failed: %s", strerror(errno)); - return 0; + /* TUNGETIFF is available since kernel v2.6.27 */ + error_setg_errno(errp, errno, + "Unable to query TUNGETIFF on FD %d", fd); + return -1; } return ifr.ifr_flags & IFF_VNET_HDR; diff --git a/net/tap-solaris.c b/net/tap-solaris.c index 4725d2314eef..ae2ba6828415 100644 --- a/net/tap-solaris.c +++ b/net/tap-solaris.c @@ -206,7 +206,7 @@ void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp) { } -int tap_probe_vnet_hdr(int fd) +int tap_probe_vnet_hdr(int fd, Error **errp) { return 0; } diff --git a/net/tap-stub.c b/net/tap-stub.c index a9ab8f829362..de525a2e69d4 100644 --- a/net/tap-stub.c +++ b/net/tap-stub.c @@ -37,7 +37,7 @@ void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp) { } -int tap_probe_vnet_hdr(int fd) +int tap_probe_vnet_hdr(int fd, Error **errp) { return 0; } diff --git a/net/tap.c b/net/tap.c index f65966aaccd8..f0ba0ae069cb 100644 --- a/net/tap.c +++ b/net/tap.c @@ -597,7 +597,11 @@ int net_init_bridge(const Netdev *netdev, const char *name, } qemu_set_nonblock(fd); - vnet_hdr = tap_probe_vnet_hdr(fd); + vnet_hdr = tap_probe_vnet_hdr(fd, errp); + if (vnet_hdr < 0) { + close(fd); + return -1; + } s = net_tap_fd_init(peer, "bridge", name, fd, vnet_hdr); snprintf(s->nc.info_str, sizeof(s->nc.info_str), "helper=%s,br=%s", helper, @@ -803,7 +807,11 @@ int net_init_tap(const Netdev *netdev, const char *name, qemu_set_nonblock(fd); - vnet_hdr = tap_probe_vnet_hdr(fd); + vnet_hdr = tap_probe_vnet_hdr(fd, errp); + if (vnet_hdr < 0) { + close(fd); + return -1; + } net_init_tap_one(tap, peer, "tap", name, NULL, script, downscript, @@ -859,8 +867,11 @@ int net_init_tap(const Netdev *netdev, const char *name, qemu_set_nonblock(fd); if (i == 0) { - vnet_hdr = tap_probe_vnet_hdr(fd); - } else if (vnet_hdr != tap_probe_vnet_hdr(fd)) { + vnet_hdr = tap_probe_vnet_hdr(fd, errp); + if (vnet_hdr < 0) { + goto free_fail; + } + } else if (vnet_hdr != tap_probe_vnet_hdr(fd, NULL)) { error_setg(errp, "vnet_hdr not consistent across given tap fds"); ret = -1; @@ -905,7 +916,11 @@ free_fail: } qemu_set_nonblock(fd); - vnet_hdr = tap_probe_vnet_hdr(fd); + vnet_hdr = tap_probe_vnet_hdr(fd, errp); + if (vnet_hdr < 0) { + close(fd); + return -1; + } net_init_tap_one(tap, peer, "bridge", name, ifname, script, downscript, vhostfdname, diff --git a/net/tap_int.h b/net/tap_int.h index e3194b23f47d..225a49ea4843 100644 --- a/net/tap_int.h +++ b/net/tap_int.h @@ -34,7 +34,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr, ssize_t tap_read_packet(int tapfd, uint8_t *buf, int maxlen); void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp); -int tap_probe_vnet_hdr(int fd); +int tap_probe_vnet_hdr(int fd, Error **errp); int tap_probe_vnet_hdr_len(int fd, int len); int tap_probe_has_ufo(int fd); void tap_fd_set_offload(int fd, int csum, int tso4, int tso6, int ecn, int ufo); -- 2.26.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 0/2] net: tap: check file descriptor can be used 2020-06-30 14:57 [PATCH v2 0/2] net: tap: check file descriptor can be used Laurent Vivier 2020-06-30 14:57 ` [PATCH v2 1/2] net: tap: check if the file descriptor is valid before using it Laurent Vivier 2020-06-30 14:57 ` [PATCH v2 2/2] net: detect errors from probing vnet hdr flag for TAP devices Laurent Vivier @ 2020-06-30 15:12 ` Daniel P. Berrangé 2 siblings, 0 replies; 9+ messages in thread From: Daniel P. Berrangé @ 2020-06-30 15:12 UTC (permalink / raw) To: Laurent Vivier Cc: Stefan Weil, Jason Wang, qemu-devel, Markus Armbruster, Gerd Hoffmann, Paolo Bonzini, Philippe Mathieu-Daudé On Tue, Jun 30, 2020 at 04:57:35PM +0200, Laurent Vivier wrote: > v2: Add patch from Daniel to check the fd can be used > > I have updated Daniel's patch not to check for EINVAL on TUNGETIFF > as I think we can avoid this special case because TUNGETIFF > is available since kernel v2.6.27 (October 2008) > Moreover I think the code was wrong as it was checking with -EINVAL and > not EINVAL. Agreed, given our documented OS platform targets, we don't need to care about such old kernels. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-07-01 9:34 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-30 14:57 [PATCH v2 0/2] net: tap: check file descriptor can be used Laurent Vivier 2020-06-30 14:57 ` [PATCH v2 1/2] net: tap: check if the file descriptor is valid before using it Laurent Vivier 2020-06-30 15:04 ` Philippe Mathieu-Daudé 2020-06-30 16:06 ` Greg Kurz 2020-06-30 16:46 ` Laurent Vivier 2020-07-01 5:50 ` Markus Armbruster 2020-07-01 9:31 ` Laurent Vivier 2020-06-30 14:57 ` [PATCH v2 2/2] net: detect errors from probing vnet hdr flag for TAP devices Laurent Vivier 2020-06-30 15:12 ` [PATCH v2 0/2] net: tap: check file descriptor can be used Daniel P. Berrangé
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.