Re: xenstored file descriptor leak

[Jürgen Groß <jgross@xxxxxxxx>]

On 02.02.21 19:37, Manuel Bouyer wrote:
> Hello,
> on NetBSD I'm tracking down an issue where xenstored never closes its
> file descriptor to /var/run/xenstored/socket and instead loops at 100%
> CPU on these descriptors.
>
> xenstored loops because poll(2) returns a POLLIN event for these
> descriptors but they are marked is_ignored = true.
>
> I'm seeing this with xen 4.15, 4.13 and has also been reported with 4.11
> with latest security patches.
> It seems to have started with patches for XSA-115 (A user reported this
> for 4.11)
>
> I've tracked it down to a difference in poll(2) implementation; it seems
> that linux will return something that is not (POLLIN|POLLOUT) when a
> socket if closed; while NetBSD returns POLLIN (this matches the NetBSD's
> man page).
Yeah, Linux seems to return POLLHUP additionally.

> First I think there may be a security issue here, as, even on linux it should
> be possible for a client to cause a socket to go to the is_ignored state,
> while still sending data and cause xenstored to go to a 100% CPU loop.
No security issue, as sockets are restricted to dom0 and user root.

In case you are suspecting a security issue, please don't send such
mails to xen-devel in future, but to security@xxxxxxxxxxxxxxxxxxxx.

> I think this is needed anyway:
>
> --- xenstored_core.c.orig       2021-02-02 18:06:33.389316841 +0100
> +++ xenstored_core.c    2021-02-02 19:27:43.761877371 +0100
> @@ -397,9 +397,12 @@
>                              !list_empty(&conn->out_list)))
>                                 *ptimeout = 0;
>                 } else {
> -                       short events = POLLIN|POLLPRI;
> -                       if (!list_empty(&conn->out_list))
> -                               events |= POLLOUT;
> +                       short events = 0;
> +                       if (!conn->is_ignored) {
> +                               events |= POLLIN|POLLPRI;
> +                               if (!list_empty(&conn->out_list))
> +                                       events |= POLLOUT;
> +                       }
>                         conn->pollfd_idx = set_fd(conn->fd, events);
>                 }
>         }
Yes, I think this is a good idea.

> Now I wonder if, on NetBSD at last, a read error or short read shouldn't
> cause the socket to be closed, as with:
>
> @@ -1561,6 +1565,8 @@
>   
>   bad_client:
>         ignore_connection(conn);
> +       /* we don't want to keep this connection alive */
> +       talloc_free(conn);
>   }
This is wrong for non-socket connections, as we want to keep the domain
in question to be known to xenstored.

For socket connections this should be okay, though.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys

Attachment: OpenPGP_signature
Description: OpenPGP digital signature