| Message ID | 1479412027-34416-3-git-send-email-ebiggers@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@google.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.
On Mon, Nov 21, 2016 at 09:07:18AM +1100, Dave Chinner wrote: > > 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.... > Thanks, I'll consider this. > > Also, shouldn't a get without an args parameter always return > EINVAL, regardless of whether the underlying file is encrypted or > not? > For most syscalls/ioctls, including this one (FS_IOC_GET_ENCRYPTION_POLICY), it's not expected for the kernel to check that a userspace pointer is NULL as opposed to some other random invalid value. It will only notice at the very end of the operation, when copying data to userspace, in which case it's expected to fail with EFAULT. It would still make sense to pass in a valid pointer when testing some other failure case, though, to avoid confusion. > > + 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. This is specifically testing the EFAULT case. The directory has been assigned an encryption policy, so it does indeed get to this case. > > These should all be in a single xfstest that "tests ioctl validity", > rather than appended to a "set_policy behaviour" test. Yes this may make sense. It gets a little blurry when you talk about testing behavior like "an encryption policy cannot be set on a directory", since that's a type of validation too. > > > + > > +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. > Will do. I think it's pretty riduculous for xfstests to potentially erase your root filesystem if you don't set some random variable though... What would be nice is for there to be a preamble that all the tests source that handles these boilerplate tasks. > > +# 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.... As I noted in the comment above this is creating a readonly mount for a read-write filesystem. This is the way to tell the kernel to change the mount flags to readonly but not the filesystem. Think of "bind" as meaning "operate on the mount, not on the filesystem". Yes, mount(8) isn't clear that you can do this. It does document setting the readonly flag in the context of setting up a new mount: mount --bind olddir newdir mount -o remount,ro,bind olddir newdir The second command is misleading because 'olddir' isn't actually used at all; the command is really just operating on 'newdir'. Newer versions of mount also support 'mount -o bind,ro olddir newdir', which is really only a shorthand for the above two commands. mount can still only set the readonly flag on the new mount using the mount syscall with MS_RDONLY|MS_BIND|MS_REMOUNT. Perhaps using the new mount syntax to set up a bind mount on a different directory, rather than reusing the same directory, would make things less confusing? Eric -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 21, 2016 at 11:11:45AM -0800, Eric Biggers wrote: > On Mon, Nov 21, 2016 at 09:07:18AM +1100, Dave Chinner wrote: > > Also, shouldn't a get without an args parameter always return > > EINVAL, regardless of whether the underlying file is encrypted or > > not? > > > > For most syscalls/ioctls, including this one (FS_IOC_GET_ENCRYPTION_POLICY), > it's not expected for the kernel to check that a userspace pointer is NULL as > opposed to some other random invalid value. It will only notice at the very end > of the operation, when copying data to userspace, in which case it's expected to > fail with EFAULT. Ok, makes sense. > > These should all be in a single xfstest that "tests ioctl > > validity", rather than appended to a "set_policy behaviour" > > test. > > Yes this may make sense. It gets a little blurry when you talk > about testing behavior like "an encryption policy cannot be set on > a directory", since that's a type of validation too. Yes, it's a a type of validation, but I see it as a completely different class of validation. That is, one set of tests is doing boundary condition testing on the ioctl (i.e. does it reject invalid input?), the other is doing behavioural tests (i.e. does it behave correctly on different types of inodes given otherwise valid input?). > > > +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. > > > > Will do. I think it's pretty riduculous for xfstests to potentially erase your > root filesystem if you don't set some random variable though... $tmp is /not a random variable/. It is required to be set in every test. The common code is unlikely to need to do something like "rm -rf $tmp/*" but it does require it to be set. The rm -rf commands are more likely to be found in test specific _cleanup functions which should be defined immediately after $tmp.... > What would be nice is for there to be a preamble that all the tests source that > handles these boilerplate tasks. Just use the 'new' script to generate your test templates, and you don't have to care. > > > +_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.... > > As I noted in the comment above this is creating a readonly mount for a > read-write filesystem. I realise that. I was commenting on it not being done in the documented way... [snip the crazy] > Perhaps using the new mount syntax to set up a bind mount on a different > directory, rather than reusing the same directory, would make things less > confusing? Yes, that's a good idea... Cheers, Dave.
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"); + } + + /* 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"); + } + + /* 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"); + } + + /* 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"); + } + + /* 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"); + } + + /* 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"); + + 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"); + } + + close(fd); + + printf("%s: test_ioctl_validation passed\n", dir); + return 0; +} + static const struct command { const char *name; int (*func)(int, char **); @@ -283,6 +364,7 @@ static const struct command { {"gen_key", gen_key}, {"rm_key", rm_key}, {"set_policy", set_policy}, + {"test_ioctl_validation", test_ioctl_validation}, {NULL, NULL} }; diff --git a/tests/generic/400 b/tests/generic/400 new file mode 100755 index 0000000..b077612 --- /dev/null +++ b/tests/generic/400 @@ -0,0 +1,88 @@ +#!/bin/bash +# FS QA Test generic/400 +# +# Test setting and getting encryption policies. +# +# This test only exercises the ioctls; it does not set up encryption keys. +# +#----------------------------------------------------------------------- +# Copyright (C) 2016 Google, Inc. +# +# Author: Eric Biggers <ebiggers@google.com> +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program 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 General Public License for more details. +# +# 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 + +_require_user +_begin_encryption_test + +cd $SCRATCH_MNT + +# Should be able to set an encryption policy on an empty directory +echo -e "\n*** Setting encryption policy on empty directory ***" +mkdir empty_dir +$FSCRYPT_UTIL set_policy 0000111122223333 empty_dir + +# Should be able to set the same policy again, but not a different one +echo -e "\n*** Setting same encryption policy again ***" +$FSCRYPT_UTIL set_policy 0000111122223333 empty_dir +$FSCRYPT_UTIL set_policy 4444555566667777 empty_dir + +# Should *not* be able to set an encryption policy on a nonempty directory +echo -e "\n*** Setting encryption policy on nonempty directory ***" +mkdir nonempty_dir +touch nonempty_dir/file +$FSCRYPT_UTIL set_policy 0000111122223333 nonempty_dir + +# Should *not* be able to set an encryption policy on a nondirectory file, even +# an empty one. Regression test for 002ced4be642: "fscrypto: only allow setting +# encryption policy on directories". +echo -e "\n*** Setting encryption policy on nondirectory ***" +touch nondirectory +$FSCRYPT_UTIL set_policy 0000111122223333 nondirectory + +# Should *not* be able to set an encryption policy on another user's directory. +# Regression test for 163ae1c6ad62: "fscrypto: add authorization check for +# setting encryption policy". +echo -e "\n*** Setting encryption policy on another user's directory ***" +mkdir unauthorized_dir +su $qa_user -c "$FSCRYPT_UTIL set_policy 0000111122223333 unauthorized_dir" + +# 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 +$FSCRYPT_UTIL set_policy 0000111122223333 readonly_mnt_dir +_scratch_mount -o rw,remount +_scratch_mount -o remount,ro,bind +$FSCRYPT_UTIL set_policy 0000111122223333 readonly_mnt_dir +_scratch_mount -o rw,remount + +# Test basic validation of set_policy / get_policy ioctl arguments +echo -e "\n*** ioctl validation ***" +mkdir validation_dir +$FSCRYPT_UTIL test_ioctl_validation validation_dir + +exit 0 diff --git a/tests/generic/400.out b/tests/generic/400.out new file mode 100644 index 0000000..dbae79d --- /dev/null +++ b/tests/generic/400.out @@ -0,0 +1,24 @@ +QA output created by 400 + +*** Setting encryption policy on empty directory *** +empty_dir: Successfully assigned encryption key 0000111122223333 + +*** Setting same encryption policy again *** +empty_dir: Successfully assigned encryption key 0000111122223333 +empty_dir: Unable to set encryption policy: Invalid argument + +*** Setting encryption policy on nonempty directory *** +nonempty_dir: Unable to set encryption policy: Directory not empty + +*** Setting encryption policy on nondirectory *** +nondirectory: Unable to set encryption policy: Invalid argument + +*** Setting encryption policy on another user's directory *** +unauthorized_dir: Unable to set encryption policy: Permission denied + +*** Setting encryption policy on readonly filesystem *** +readonly_mnt_dir: Unable to set encryption policy: Read-only file system +readonly_mnt_dir: Unable to set encryption policy: Read-only file system + +*** ioctl validation *** +validation_dir: test_ioctl_validation passed diff --git a/tests/generic/group b/tests/generic/group index 08007d7..cf89f06 100644 --- a/tests/generic/group +++ b/tests/generic/group @@ -392,3 +392,4 @@ 387 auto clone 388 auto log metadata 389 auto quick acl +400 auto quick encrypt
Several kernel bugs were recently fixed regarding the constraints for setting encryption policies. Add tests for these cases and a few more. Signed-off-by: Eric Biggers <ebiggers@google.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