Re: [Xen-devel] [PATCH v4 6/6] RFC: test/depriv: Add a tool to check process-level depriv

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----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: Stefano Stabellini <sstabellini@xxxxxxxxxx>; Wei Liu
> <wei.liu2@xxxxxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxx>; Ross
> Lagerwall <ross.lagerwall@xxxxxxxxxx>; Anthony Perard
> <anthony.perard@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>
> Subject: [Xen-devel] [PATCH v4 6/6] RFC: test/depriv: Add a tool to check
> process-level depriv
> 
> Add a tool to check whether the various process-level deprivileging
> operations have actually taken place on the process.
> 
> The tool takes a domname or domid, and returns success or failure.
> 
> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx>
> ---
> Changes since v3:
> - Use xen-qemuuser-range-base's gid rather than hard-coding `nobody`
> - Change FIXME about not handling other userid schemes into an NB.
> 
> Changes since v2:
> - Make grep for Uid line more strict
> - Fix Gid grep, make more strict
> - Match strictly more than one space
> - Look up the group ID for `nobody` rather than hard-coding it
> - Move tests from other patches into one patch
> - Remove suffix (in case we change the language)
> - Install in the path
> 
> NB this patch is included for reference only, while I consider whether
> to leave this as a stand-alone script, or whether to merge osstest's
> fd checker functionality into it (perhaps changing the language to
> perl at the same time).  Reviews of the general detection algorithm
> are welcome, but there's no need for a detailed review of the code
> until the script is in its final form.
> 
> CC: Ian Jackson <ian.jackson@xxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Anthony Perard <anthony.perard@xxxxxxxxxx>
> CC: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> ---
>  tools/tests/depriv/Makefile               |   2 +-
>  tools/tests/depriv/depriv-process-checker | 148 ++++++++++++++++++++++
>  2 files changed, 149 insertions(+), 1 deletion(-)
>  create mode 100755 tools/tests/depriv/depriv-process-checker
> 
> diff --git a/tools/tests/depriv/Makefile b/tools/tests/depriv/Makefile
> index 3cba28da25..1b3d09e97d 100644
> --- a/tools/tests/depriv/Makefile
> +++ b/tools/tests/depriv/Makefile
> @@ -23,7 +23,7 @@ LDLIBS += $(LDLIBS_libxendevicemodel)
>  LDLIBS += $(LDLIBS_libxentoolcore)
>  LDLIBS += $(LDLIBS_libxentoollog)
> 
> -INSTALL_PRIVBIN-y += depriv-fd-checker
> +INSTALL_PRIVBIN-y += depriv-fd-checker depriv-process-checker
>  INSTALL_PRIVBIN := $(INSTALL_PRIVBIN-y)
>  TARGETS += $(INSTALL_PRIVBIN)
> 
> diff --git a/tools/tests/depriv/depriv-process-checker
> b/tools/tests/depriv/depriv-process-checker
> new file mode 100755
> index 0000000000..4f9f0d7fbc
> --- /dev/null
> +++ b/tools/tests/depriv/depriv-process-checker
> @@ -0,0 +1,148 @@
> +#!/bin/bash
> +
> +domain="$1"
> +
> +if [[ "$domain" =~ ^[0-9]+$ ]] ; then
> +    domid="$domain"
> +else
> +    domid=$(xl domid "$domain")
> +fi
> +
> +dmpid=$(xenstore-read /local/domain/$domid/image/device-model-pid
> 2>/dev/null)
> +if [[ -z "$dmpid" ]] ; then
> +    echo "xenstore-read failed"
> +    exit 1
> +fi
> +
> +failed="false"
> +
> +# TEST: Process / group id
> +#
> +# Read /proc/<qpid>/status, checking Uid and Gid lines
> +#
> +# Uid should be xen-qemuuser-range-base+$domid
> +# Gid should be gid for xen-qemuuser-range-base
> +#
> +# NB this doesn't handle other configurations (e.g.,
> +# xen-qemuuser-shared).
> +echo -n "Process UID: "
> +tgt_uid=$(id -u xen-qemuuser-range-base)
> +tgt_uid=$(( $tgt_uid + $domid ))
> +
> +# Example input:
> +# Uid:       1193    1193    1193    1193
> +input=$(grep ^Uid: /proc/$dmpid/status)
> +if [[ "$input" =~ ^Uid:[[:space:]]+([0-9]+)[[:space:]]+([0-
> 9]+)[[:space:]]+([0-9]+)[[:space:]]+([0-9]+)$ ]] ; then
> +    result="PASSED"
> +    for i in {1..4}; do
> +     if [[ "${BASH_REMATCH[$i]}" != "$tgt_uid" ]] ; then
> +         result="FAILED"
> +         failed="true"

This patter occurs many times. Could you not just have a boolean 'failed' value 
and a supporting 'reason' (which might be empty) then you can just test 
'reason' at the end to decide what to echo?

> +         break
> +     fi
> +    done
> +else
> +    result="FAILED"
> +    failed="true"
> +fi
> +echo $result
> +
> +# Example input:
> +# Gid:       10020   10020   10020   10020
> +echo -n "Process GID: "
> +tgt_gid=$(id -g xen-qemuuser-range-base)
> +input=$(grep ^Gid: /proc/$dmpid/status)
> +if [[ "$input" =~ ^Gid:[[:space:]]+([0-9]+)[[:space:]]+([0-
> 9]+)[[:space:]]+([0-9]+)[[:space:]]+([0-9]+)$ ]] ; then
> +    result="PASSED"
> +    for i in {1..4}; do
> +     if [[ "${BASH_REMATCH[$i]}" != "$tgt_gid" ]] ; then
> +         result="FAILED"
> +         failed="true"
> +         break
> +     fi
> +    done
> +else
> +    result="FAILED"
> +    failed="true"
> +fi
> +echo $result
> +
> +# TEST: chroot
> +#
> +# Read /proc/<dmpid>/root to see if it's correct.
> +echo -n "Chroot: "
> +if [[ -n "$XEN_RUN_DIR" ]] ; then
> +    tgt_chroot=$XEN_RUN_DIR/qemu-root-$domid
> +    root=$(readlink /proc/$dmpid/root)
> +    if [[ "$root" != "$tgt_chroot" ]] ; then
> +     echo "FAILED"

You are using hard tabs here but 4 spaces in the blocks above.

> +     failed="true"
> +    else
> +     echo "PASSED"
> +    fi
> +else
> +    echo "FAILED (XEN_RUN_DIR undefined)"
> +    failed="true"
> +fi
> +
> +# TEST: Namespace unsharing
> +#
> +# Read /proc/<dmpid>/ns/<namespace> and make sure it's not equal to
> +# the current processes' value
> +for nsname in ipc mnt; do
> +    echo -n "Unshare namespace $nsname: "
> +    dmns=$(readlink /proc/$dmpid/ns/$nsname)
> +    myns=$(readlink /proc/self/ns/$nsname)
> +
> +    if [[ "$dmns" == "$myns" ]] ; then
> +     echo "FAILED"
> +     failed="true"
> +    else
> +     echo "PASSED"
> +    fi
> +done
> +
> +# TEST: RLIMITs
> +#
> +# Read /proc/<dmpid>/limits
> +function check_rlimit() {
> +    limit_name=$1
> +    limit_string=$2
> +    tgt=$3
> +
> +    echo -n "rlimit $limit_name: "
> +    input=$(grep "^$limit_string" /proc/$dmpid/limits)
> +
> +    if [[ -z "$input" ]] ; then
> +     echo "Couldn't find limit $limit"
> +     echo FAILED
> +     failed="true"
> +     return
> +    fi
> +
> +    if [[ "$input" =~
> ^$limit_string[[:space:]]*([^[:space:]]+)[[:space:]]*([^[:space:]]+)[[:spa
> ce:]]*[^[:space:]]+ ]] ; then
> +     if [[ "${BASH_REMATCH[1]}" != $tgt ||
> +               "${BASH_REMATCH[2]}" != $tgt ]] ; then
> +         echo "FAILED"
> +         failed="true"
> +     else
> +         echo "PASSED"
> +     fi
> +    else
> +     echo "Couldn't parse /proc/<dmpid>/limits"
> +     echo "FAILED"
> +     failed="true"
> +    fi
> +}
> +check_rlimit FSIZE "Max file size" "262144"
> +check_rlimit NPROC "Max processes" 0
> +check_rlimit CORE "Max core file size" "0"
> +check_rlimit MSGQUEUE "Max msgqueue size" 0
> +check_rlimit LOCKS "Max file locks" 0
> +check_rlimit MEMLOCK "Max locked memory" 0
> +
> +if $failed ; then

If you made 'failed' an integer value then you could just 'exit $failed'

  Paul

> +    exit 1
> +else
> +    exit 0
> +fi
> --
> 2.19.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel