Message-ID: <20161120220718.GJ28177@dastard>
Date:   Mon, 21 Nov 2016 09:07:18 +1100
From:   Dave Chinner <david@...morbit.com>
To:     Eric Biggers <ebiggers@...gle.com>
Cc:     fstests@...r.kernel.org, linux-ext4@...r.kernel.org,
        linux-f2fs@...r.kernel.org, "Theodore Y . Ts'o" <tytso@....edu>,
        Jaegeuk Kim <jaegeuk@...nel.org>,
        Richard Weinberger <richard@....at>,
        David Gstir <david@...ma-star.at>
Subject: Re: [PATCH 2/4] generic: test setting and getting encryption policies

On Thu, Nov 17, 2016 at 11:47:05AM -0800, Eric Biggers wrote:
> Several kernel bugs were recently fixed regarding the constraints for
> setting encryption policies.  Add tests for these cases and a few more.

more comments below, but in general this sort of test should be
driven through xfs_io command line parameters.

i.e. we put all the functionality into the xfs_io comaand interface,
and it just passes through whatever the test script tells it. In
this case, the set_policy command needs several options to set
different parts of the policy appropriately.

The reason we tend to put this sort of thing into xfs_io is that
when we need to write a new test, all the commands we need to
construct specific policies/contexts already exist and we don't have
to write new helpers for each test....

> 
> Signed-off-by: Eric Biggers <ebiggers@...gle.com>
> ---
>  src/fscrypt_util.c    | 82 +++++++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/400     | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/400.out | 24 ++++++++++++++
>  tests/generic/group   |  1 +
>  4 files changed, 195 insertions(+)
>  create mode 100755 tests/generic/400
>  create mode 100644 tests/generic/400.out
> 
> diff --git a/src/fscrypt_util.c b/src/fscrypt_util.c
> index de63667..9428cb4 100644
> --- a/src/fscrypt_util.c
> +++ b/src/fscrypt_util.c
> @@ -96,6 +96,7 @@ usage(void)
>  "    fscrypt_util gen_key\n"
>  "    fscrypt_util rm_key KEYDESC\n"
>  "    fscrypt_util set_policy KEYDESC DIR\n"
> +"    fscrypt_util test_ioctl_validation DIR\n"
>  );
>  	exit(2);
>  }
> @@ -276,6 +277,86 @@ static int set_policy(int argc, char **argv)
>  	return 0;
>  }
>  
> +/*
> + * Test that the kernel does basic validation of the arguments to
> + * FS_IOC_SET_ENCRYPTION_POLICY and FS_IOC_GET_ENCRYPTION_POLICY.
> + */
> +static int test_ioctl_validation(int argc, char **argv)
> +{
> +	const char *dir;
> +	int fd;
> +	struct fscrypt_policy policy;
> +
> +	if (argc != 1)
> +		usage();
> +	dir = argv[0];
> +
> +	fd = open(dir, O_RDONLY);
> +	if (fd < 0)
> +		die_errno("%s: Unable to open", dir);
> +
> +	/* trying to get encryption policy for unencrypted file */
> +	if (ioctl(fd, FS_IOC_GET_ENCRYPTION_POLICY, NULL) != -1 ||
> +	    (errno != ENODATA && errno != ENOENT)) {
> +		die("expected FS_IOC_GET_ENCRYPTION_POLICY to fail with "
> +		    "ENODATA or ENOENT when unencrypted file specified");
> +	}

Can we format these in the normal way? i.e.

	error = ioctl();
	if (error < 0 &&
	    (errno exceptions))
		die()

Also, shouldn't a get without an args parameter always return
EINVAL, regardless of whether the underlying file is encrypted or
not?

> +	/* invalid pointer */
> +	if (ioctl(fd, FS_IOC_SET_ENCRYPTION_POLICY, NULL) != -1 ||
> +	    errno != EFAULT) {
> +		die("expected FS_IOC_SET_ENCRYPTION_POLICY to fail with "
> +		    "EFAULT when invalid pointer specified");
> +	}

>From the command line, shouldn't this be triggered by "set_policy
NULL"?

> +	/* invalid flags */
> +	init_policy_default(&policy);
> +	policy.flags = 0xFF;
> +	if (ioctl(fd, FS_IOC_SET_ENCRYPTION_POLICY, &policy) != -1 ||
> +	    errno != EINVAL) {
> +		die("expected FS_IOC_SET_ENCRYPTION_POLICY to fail with "
> +		    "EINVAL when invalid flags specified");
> +	}

"set_policy -f 0xff"

> +
> +	/* invalid encryption modes */
> +	init_policy_default(&policy);
> +	policy.contents_encryption_mode = 0xFF;
> +	policy.filenames_encryption_mode = 0xFF;
> +	if (ioctl(fd, FS_IOC_SET_ENCRYPTION_POLICY, &policy) != -1 ||
> +	    errno != EINVAL) {
> +		die("expected FS_IOC_SET_ENCRYPTION_POLICY to fail with "
> +		    "EINVAL when invalid encryption modes specified");
> +	}

"set_policy -c 0xff -n 0xff"

> +
> +	/* invalid policy version */
> +	init_policy_default(&policy);
> +	policy.version = 0xFF;
> +	if (ioctl(fd, FS_IOC_SET_ENCRYPTION_POLICY, &policy) != -1 ||
> +	    errno != EINVAL) {
> +		die("expected FS_IOC_SET_ENCRYPTION_POLICY to fail with "
> +		    "EINVAL when invalid policy version specified");
> +	}

"set_policy -v 0xff"

> +
> +	/* success case */
> +	init_policy_default(&policy);
> +	if (ioctl(fd, FS_IOC_SET_ENCRYPTION_POLICY, &policy) != 0)
> +		die_errno("expected FS_IOC_SET_ENCRYPTION_POLICY to succeed");

"set_policy default"

> +	verify_policy(dir, fd, &policy);
> +
> +	/* invalid pointer (get) */
> +	if (ioctl(fd, FS_IOC_GET_ENCRYPTION_POLICY, NULL) != -1 ||
> +	    errno != EFAULT) {
> +		die("expected FS_IOC_GET_ENCRYPTION_POLICY to fail with "
> +		    "EFAULT when invalid pointer specified");
> +	}

EINVAL - this should never get to copyout to generate EFAULT, so
should not require separate tests for having no policy vs a valid
policy.

These should all be in a single xfstest that "tests ioctl validity",
rather than appended to a "set_policy behaviour" test.

> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, see <http://www.gnu.org/licenses/>.
> +#-----------------------------------------------------------------------
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +here=`pwd`
> +echo "QA output created by $seq"
> +
> +. ./common/encrypt

This is not the way to include all the required scripts, as I
mentioned in my last email....

Also, please do not gut the test script preamble - it's there in the
new test template for good reason and that is that all the common
code that is included relies on the setup it does. e.g. this means $tmp
is not properly set, so any common code that has been included that
does 'rm -rf $tmp/*' if going to erase your root filesystem.

> +_require_user
> +_begin_encryption_test
> +
> +cd $SCRATCH_MNT
> +
> +_require_user
> +_begin_encryption_test
> +
> +cd $SCRATCH_MNT

... because mounting scratch without having first run _scratch_mkfs
is just wrong. People familiar with xfstests setup are going to look
at this and think the test is broken, because it doesn't
_require_scratch, it doesn't run mkfs or mount, etc....

> +# Should *not* be able to set an encryption policy on a directory on a
> +# filesystem mounted readonly.  Regression test for ba63f23d69a3: "fscrypto:
> +# require write access to mount to set encryption policy".  Test both a regular
> +# readonly filesystem and a read-write filesystem remounted with "ro,bind",
> +# which creates a readonly mount for a read-write filesystem.
> +echo -e "\n*** Setting encryption policy on readonly filesystem ***"
> +mkdir readonly_mnt_dir
> +_scratch_mount -o ro,remount

scratch_remount ro

> +$FSCRYPT_UTIL set_policy 0000111122223333 readonly_mnt_dir
> +_scratch_mount -o rw,remount

scratch_remount rw

> +_scratch_mount -o remount,ro,bind

Umm, what does a bind mount do when there's no source/target
directory? Whatever you are doing here is not documented in the
mount(8) man page....

Cheers,

Dave.
-- 
Dave Chinner
david@...morbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists