Re: [Xen-devel] [PATCH 03/22] xentoolcore, _restrict_all: Introduce new library and implementation

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Fri, Sep 15, 2017 at 07:48:40PM +0100, Ian Jackson wrote:
> diff --git a/tools/libs/toolcore/include/xentoolcore.h 
> b/tools/libs/toolcore/include/xentoolcore.h
> new file mode 100644
> index 0000000..1ab646e
> --- /dev/null
> +++ b/tools/libs/toolcore/include/xentoolcore.h
> @@ -0,0 +1,71 @@
> +/*
> + * xentoolcore.h
> + *
> + * Copyright (c) 2017 Citrix
> + * 
> + * Common features used/provided by all Xen tools libraries
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation;
> + * version 2.1 of the License.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; If not, see 
> <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef XENTOOLCORE_H
> +#define XENTOOLCORE_H
> +
> +#include <stdint.h>
> +
> +/*
> + * int xentoolcore_restrict_all(uint32_t domid);
> + *
> + * Arranges that Xen library handles (fds etc.) which are currently held
> + * by Xen libraries, can no longer be used other than to affect domid.
> + *
> + * If this cannot be achieved, returns -1 and sets errno.
> + * Idempotent if domid is always the same.
> + *
> + *  ====================================================================
> + *  IMPORTANT - IMPLEMENTATION STATUS
> + *
> + *  This function will be implemented insofar as it appears necessary
> + *  for the purposes of running a deprivileged qemu.
> + *
> + *  However, this function is NOT implemented for all Xen libraries.
> + *  For each use case of this function, the designer must evaluate and
> + *  audit whether the implementation is sufficient in their specific
> + *  context.
> + *
> + *  Of course, patches to extend the implementation are very welcome.
> + *  ====================================================================
> + *
> + * Thread safe.
> + *
> + * We expect that no callers do the following:
> + *   - in one thread call xen_somelibrary_open|close
> + *   - in another thread call fork
> + *   - in the child of the fork, before exec, call
> + *     xen_some[other]library_open|close or xentoolcore_restrict_all
> + *
> + */
> +int xentoolcore_restrict_all(uint32_t domid);
> +
> +#endif /* XENTOOLCORE_H */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/tools/libs/toolcore/include/xentoolcore_internal.h 
> b/tools/libs/toolcore/include/xentoolcore_internal.h
> new file mode 100644
> index 0000000..670e29d
> --- /dev/null
> +++ b/tools/libs/toolcore/include/xentoolcore_internal.h
> @@ -0,0 +1,102 @@
> +/*
> + * xentoolcore_internal.h
> + *
> + * Interfaces of xentoolcore directed internally at other Xen libraries
> + *
> + * Copyright (c) 2017 Citrix
> + * 
> + * Common code used by all Xen tools libraries
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation;
> + * version 2.1 of the License.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; If not, see 
> <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef XENTOOLCORE_INTERNAL_H
> +#define XENTOOLCORE_INTERNAL_H
> +
> +#include "xentoolcore.h"
> +#include "_xentoolcore_list.h"
> +
> +/*---------- active handle registration ----------*/
> +
> +/*
> + * This is all to support xentoolcore_restrict_all
> + *
> + * Any libxl library that opens a Xen control handle of any kind which
> + * might allow manipulation of dom0, of other domains, or of the whole
> + * machine, must:
> + *   I. arrange that their own datastructure contains a
> + *          Xentoolcore__Active_Handle
> + * 
> + *   II. during the "open handle" function
> + *     1. allocate the memory for the own datastructure and initialise it
> + *     2. set Xentoolcore__Active_Handle.restrict_callback
> + *     3. call xentoolcore__register_active_handle
> + *       3a. if the open fails, call xentoolcore__deregister_active_handle
> + *     4. ONLY THEN actually open the relevant fd or whatever
> + *
> + *   III. during the "close handle" function
> + *     1. FIRST close the relevant fd or whatever
> + *     2. call xentoolcore__deregister_active_handle
> + *
> + *   IV. in the restrict_callback function
> + *     * Arrange that the fd (or other handle) can no longer by used
> + *       other than with respect to domain domid.
> + *     * Future attempts to manipulate other domains (or the whole
> + *       host) via this handle must cause an error return (and
> + *       perhaps a log message), not a crash
> + *     * If selective restriction is not possible, the handle must
> + *       be completely invalidated so that it is not useable;
> + *       subsequent manipulations may not crash
> + *     * The restrict_callback function should not normally fail
> + *       if this can be easily avoided - it is better to make the
> + *       handle nonfunction instead.
> + *     * NB that restrict_callback might be called again.  That must
> + *       work properly: if the domid is the same, it is idempotent.
> + *       If the domid is different. then either the handle must be
> + *       completely invalidated, or restrict_callback must fail.)
> + *
> + * Thread safety:
> + *    xentoolcore__[de]register_active_handle are threadsafe
> + *      but MUST NOT be called within restrict_callback
> + *
> + * Fork safety:
> + *    Libraries which use these functions do not on that account
> + *    need to take any special care over forks occurring in
> + *    other threads, provided that they obey the rules above.
> + */
> +
> +typedef struct Xentoolcore__Active_Handle Xentoolcore__Active_Handle;
> +
> +typedef int Xentoolcore__Restrict_Callback(Xentoolcore__Active_Handle*,
> +                                           uint32_t domid);
> +
> +struct Xentoolcore__Active_Handle {
> +    Xentoolcore__Restrict_Callback *restrict_callback;
> +    XENTOOLCORE_LIST_ENTRY(Xentoolcore__Active_Handle) entry;
> +};
> +
> +void xentoolcore__register_active_handle(Xentoolcore__Active_Handle*);
> +void xentoolcore__deregister_active_handle(Xentoolcore__Active_Handle*);

Why use two underscores in those function names?

Is this library supposed to be stable? We only expect this to be tech
review, right?  I think it is worth explicitly stating that if that's
the case.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel