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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 11/06/2018 09:22 AM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf
>> Of George Dunlap
>> Sent: 05 November 2018 18:07
>> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
>> Cc: Anthony Perard <anthony.perard@xxxxxxxxxx>; Ian Jackson
>> <Ian.Jackson@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; George Dunlap
>> <George.Dunlap@xxxxxxxxxx>
>> 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@xxxxxxxxxx>
>> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxx>
>> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
>> CC: Anthony Perard <anthony.perard@xxxxxxxxxx>
>> ---
>>  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?

Got rid of trailing witespace; I mentioned it under "Changes since v3".

>>  #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?

No reason; just habit to add things to the end.  I'll move it earlier
unless Ian objects.

>> +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>

Sure.

>> +
>>  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).

Don't think it says anything about this case; I find having the
arguments indented beyond the format string easier to read.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel