Re: [PATCH v4 5/6] tools/dm_depriv: Add first cut RLIMITs

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf
> Of George Dunlap
> Sent: 05 November 2018 18:07
> To: xen-devel@lists.xenproject.org
> Cc: Anthony Perard <anthony.perard@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Wei Liu <wei.liu2@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>
> Subject: [Xen-devel] [PATCH v4 5/6] tools/dm_depriv: Add first cut RLIMITs
> 
> Limit the ability of a potentially compromised QEMU to consume system
> resources.  Key limits:
>  - RLIMIT_FSIZE (file size): 256KiB
>  - RLIMIT_NPROC (after uid changes to a unique uid)
> 
> Probably unnecessary limits but why not:
>  - RLIMIT_CORE: 0
>  - RLIMIT_MSGQUEUE: 0
>  - RLIMIT_LOCKS: 0
>  - RLIMIT_MEMLOCK: 0
> 
> NB that we do not yet set RLIMIT_AS (total virtual memory) or
> RLIMIT_NOFILES (number of open files), since these require more care
> and/or more coordination with QEMU to implement.
> 
> Suggested-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
> Changes since v3:
> - Align RLIMIT_ENTRY list for easier reading
> - Fix wrong format string specifier
> - Get rid of some trailing whitespace
> 
> Changes since v2:
> - Use a macro to define rlimit entries
> - Use RLIMIT_NLIMITS as an end-of-list marker, rather than -1
> - Various style clean-ups
> 
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Anthony Perard <anthony.perard@citrix.com>
> ---
>  docs/designs/qemu-deprivilege.md | 12 ++++-----
>  tools/libxl/libxl_linux.c        | 42 ++++++++++++++++++++++++++++++--
>  2 files changed, 46 insertions(+), 8 deletions(-)
> 
> diff --git a/docs/designs/qemu-deprivilege.md b/docs/designs/qemu-
> deprivilege.md
> index a461ebbadd..e984064da6 100644
> --- a/docs/designs/qemu-deprivilege.md
> +++ b/docs/designs/qemu-deprivilege.md
> @@ -105,12 +105,6 @@ call:
> 
>  [qemu-namespaces]: https://lists.gnu.org/archive/html/qemu-devel/2017-
> 10/msg04723.html
> 
> -# Restrictions / improvements still to do
> -
> -This lists potential restrictions still to do.  It is meant to be
> -listed in order of ease of implementation, with low-hanging fruit
> -first.
> -
>  ### Basic RLIMITs
> 
>  '''Description''': A number of limits on the resources that a given
> @@ -137,6 +131,12 @@ are specified; this does not apply to QEMU running as
> a Xen DM.
> 
>  '''Tested''': Not tested
> 
> +# Restrictions / improvements still to do
> +
> +This lists potential restrictions still to do.  It is meant to be
> +listed in order of ease of implementation, with low-hanging fruit
> +first.
> +
>  ### Further RLIMITs
> 
>  RLIMIT_AS limits the total amount of memory; but this includes the
> diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
> index c7a345f4bb..ac9526d731 100644
> --- a/tools/libxl/libxl_linux.c
> +++ b/tools/libxl/libxl_linux.c
> @@ -12,11 +12,12 @@
>   * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>   * GNU Lesser General Public License for more details.
>   */
> -
> +

Stray whitespace change?

>  #include "libxl_osdeps.h" /* must come before any other headers */
> 
>  #include "libxl_internal.h"
> -
> +#include <sys/resource.h>
> +

Personally I tend to put local includes after ones from the include path. Is there a reason it needs to come afterwards?

>  int libxl__try_phy_backend(mode_t st_mode)
>  {
>      if (S_ISBLK(st_mode) || S_ISREG(st_mode)) {
> @@ -307,9 +308,31 @@ int libxl__pci_topology_init(libxl__gc *gc,
>      return err;
>  }
> 
> +static struct {
> +    int resource;
> +    rlim_t limit;
> +} rlimits[] = {
> +#define RLIMIT_ENTRY(r, l) \
> +    { .resource = r, .limit = l }
> +    /* Big enough for log files, not big enough for a DoS */
> +    RLIMIT_ENTRY(RLIMIT_FSIZE,    256*1024),
> +
> +    /* Shouldn't need any of these */
> +    RLIMIT_ENTRY(RLIMIT_NPROC,    0),
> +    RLIMIT_ENTRY(RLIMIT_CORE,     0),
> +    RLIMIT_ENTRY(RLIMIT_MSGQUEUE, 0),
> +    RLIMIT_ENTRY(RLIMIT_LOCKS,    0),
> +    RLIMIT_ENTRY(RLIMIT_MEMLOCK,  0),
> +
> +    /* End-of-list marker */
> +    RLIMIT_ENTRY(RLIMIT_NLIMITS,  0),
> +};
> +#undef RLIMIT_ENTRY

<pedantic> The undef should come before the brace to get the scoping correct. </pedantic>

> +
>  int libxl__local_dm_preexec_restrict(libxl__gc *gc)
>  {
>      int r;
> +    unsigned i;
> 
>      /* Unshare mount and IPC namespaces.  These are unused by QEMU. */
>      r = unshare(CLONE_NEWNS | CLONE_NEWIPC);
> @@ -318,6 +341,21 @@ int libxl__local_dm_preexec_restrict(libxl__gc *gc)
>          return ERROR_FAIL;
>      }
> 
> +    /* Set various "easy" rlimits */
> +    for (i = 0; rlimits[i].resource != RLIMIT_NLIMITS; i++) {
> +        struct rlimit rlim;
> +
> +        rlim.rlim_cur = rlim.rlim_max = rlimits[i].limit;
> +
> +        r = setrlimit(rlimits[i].resource, &rlim);
> +        if (r < 0) {
> +            LOGE(ERROR, "Setting rlimit %d to %llu failed\n",
> +                                  rlimits[i].resource,
> +                                  (unsigned long long)rlimits[i].limit);

Indentation of the continuation lines looks odd (although libxl's coding style is a mystery to me so they may be correct).

  Paul

> +            return ERROR_FAIL;
> +        }
> +    }
> +
>      return 0;
>  }
> 
> --
> 2.19.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel