Re: [Qemu-devel] [PATCH 1/2] qemu-char: BUGFIX, don't call FD_ISSET with negative fd

[Anthony Liguori <aliguori@us.ibm.com>]

David Gibson <david@gibson.dropbear.id.au> writes:

> tcp_chr_connect(), unlike for example udp_chr_update_read_handler() does
> not check if the fd it is using is valid (>= 0) before passing it to
> qemu_set_fd_handler2().  If using e.g. a TCP serial port, which is not
> initially connected, this can result in -1 being passed to FD_ISSET, which
> has undefined behaviour.  On x86 it seems to harmlessly return 0, but on
> PowerPC, it causes a fortify buffer overflow error to be thrown.
>
> This patch fixes this by putting an extra test in tcp_chr_connect(), and
> also adds an assert qemu_set_fd_handler2() to catch other such errors on
> all platforms, rather than just some.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Applied. Thanks.

Regards,

Anthony Liguori


> ---
>  iohandler.c |    2 ++
>  qemu-char.c |    6 ++++--
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/iohandler.c b/iohandler.c
> index dea4355..a2d871b 100644
> --- a/iohandler.c
> +++ b/iohandler.c
> @@ -56,6 +56,8 @@ int qemu_set_fd_handler2(int fd,
>  {
>      IOHandlerRecord *ioh;
>  
> +    assert(fd >= 0);
> +
>      if (!fd_read && !fd_write) {
>          QLIST_FOREACH(ioh, &io_handlers, next) {
>              if (ioh->fd == fd) {
> diff --git a/qemu-char.c b/qemu-char.c
> index 398baf1..73e48ff 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2332,8 +2332,10 @@ static void tcp_chr_connect(void *opaque)
>      TCPCharDriver *s = chr->opaque;
>  
>      s->connected = 1;
> -    qemu_set_fd_handler2(s->fd, tcp_chr_read_poll,
> -                         tcp_chr_read, NULL, chr);
> +    if (s->fd >= 0) {
> +        qemu_set_fd_handler2(s->fd, tcp_chr_read_poll,
> +                             tcp_chr_read, NULL, chr);
> +    }
>      qemu_chr_generic_open(chr);
>  }
>  
> -- 
> 1.7.10.4