| Message ID | 20220814192603.7387-3-gnoack3000@gmail.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | landlock: truncate support | expand |
On 14/08/2022 21:26, Günther Noack wrote: > These tests exercise the following truncation operations: > > * truncate() (truncate by path) > * ftruncate() (truncate by file descriptor) > * open with the O_TRUNC flag > * special case: creat(), which is open with O_CREAT|O_WRONLY|O_TRUNC. > > in the following scenarios: > > * Files with read, write and truncate rights. > * Files with read and truncate rights. > * Files with the truncate right. > * Files without the truncate right. > > In particular, the following scenarios are enforced with the test: > > * The truncate right is required to use ftruncate, > even when the thread already has the right to write to the file. > * open() with O_TRUNC requires the truncate right, if it truncates a file. > open() already checks security_path_truncate() in this case, > and it required no additional check in the Landlock LSM's file_open hook. > * creat() requires the truncate right > when called with an existing filename. > * creat() does *not* require the truncate right > when it's creating a new file. > > Signed-off-by: Günther Noack <gnoack3000@gmail.com> > --- > tools/testing/selftests/landlock/fs_test.c | 219 +++++++++++++++++++++ > 1 file changed, 219 insertions(+) > > diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c > index cb77eaa01c91..7a2ce6cd1a5a 100644 > --- a/tools/testing/selftests/landlock/fs_test.c > +++ b/tools/testing/selftests/landlock/fs_test.c > @@ -58,6 +58,7 @@ static const char file1_s2d3[] = TMP_DIR "/s2d1/s2d2/s2d3/f1"; > static const char file2_s2d3[] = TMP_DIR "/s2d1/s2d2/s2d3/f2"; > > static const char dir_s3d1[] = TMP_DIR "/s3d1"; > +static const char file1_s3d1[] = TMP_DIR "/s3d1/f1"; > /* dir_s3d2 is a mount point. */ > static const char dir_s3d2[] = TMP_DIR "/s3d1/s3d2"; > static const char dir_s3d3[] = TMP_DIR "/s3d1/s3d2/s3d3"; > @@ -83,6 +84,7 @@ static const char dir_s3d3[] = TMP_DIR "/s3d1/s3d2/s3d3"; > * │ ├── f1 > * │ └── f2 > * └── s3d1 > + * ├── f1 > * └── s3d2 > * └── s3d3 > */ > @@ -208,6 +210,7 @@ static void create_layout1(struct __test_metadata *const _metadata) > create_file(_metadata, file1_s2d3); > create_file(_metadata, file2_s2d3); > > + create_file(_metadata, file1_s3d1); > create_directory(_metadata, dir_s3d2); > set_cap(_metadata, CAP_SYS_ADMIN); > ASSERT_EQ(0, mount("tmp", dir_s3d2, "tmpfs", 0, "size=4m,mode=700")); > @@ -230,6 +233,7 @@ static void remove_layout1(struct __test_metadata *const _metadata) > EXPECT_EQ(0, remove_path(file1_s2d2)); > EXPECT_EQ(0, remove_path(file1_s2d1)); > > + EXPECT_EQ(0, remove_path(file1_s3d1)); > EXPECT_EQ(0, remove_path(dir_s3d3)); > set_cap(_metadata, CAP_SYS_ADMIN); > umount(dir_s3d2); > @@ -3023,6 +3027,221 @@ TEST_F_FORK(layout1, proc_pipe) > ASSERT_EQ(0, close(pipe_fds[1])); > } > > +/* > + * Opens the file and invokes ftruncate(2). > + * > + * Returns the errno of ftruncate if ftruncate() fails. > + * Returns EBADFD if open() or close() fail (should not happen). > + * Returns 0 if ftruncate(), open() and close() were successful. > + */ > +static int test_ftruncate(struct __test_metadata *const _metadata, _metadata is no longer needed. Same for test_creat(). > + const char *const path, int flags) > +{ > + int res, err, fd; > + > + fd = open(path, flags | O_CLOEXEC); > + if (fd < 0) > + return EBADFD; Instead of EBADFD, which is a valid open(2) error, you can use ENOSYS and add a comment explaining that we are not interested by this open(2) error code but only the ftruncate(2) one because we are sure opening such path is allowed or because open(2) is already tested before calls to test_ftruncate(). > + > + res = ftruncate(fd, 10); > + err = errno; > + > + if (close(fd) != 0) > + return EBADFD; Why not returning errno? See test_open_rel() comments. > + > + if (res < 0) > + return err; > + return 0; > +} > + > +/* Invokes truncate(2) and returns the errno or 0. */ > +static int test_truncate(const char *const path) > +{ > + if (truncate(path, 10) < 0) > + return errno; > + return 0; > +} > + > +/* > + * Invokes creat(2) and returns its errno or 0. > + * Closes the opened file descriptor on success. > + * Returns EBADFD if close() returns an error (should not happen). > + */ > +static int test_creat(struct __test_metadata *const _metadata, > + const char *const path, mode_t mode) > +{ > + int fd = creat(path, mode); > + > + if (fd < 0) > + return errno; > + > + if (close(fd) < 0) > + return EBADFD; Why not returning errno? > + return 0; > +} > + > +TEST_F_FORK(layout1, truncate) > +{ > + const char *const file_rwt = file1_s1d1; > + const char *const file_rw = file2_s1d1; > + const char *const file_rt = file1_s1d2; > + const char *const file_t = file2_s1d2; > + const char *const file_none = file1_s1d3; > + const char *const dir_t = dir_s2d1; > + const char *const file_in_dir_t = file1_s2d1; > + const char *const dir_w = dir_s3d1; > + const char *const file_in_dir_w = file1_s3d1; > + const struct rule rules[] = { > + { > + .path = file_rwt, > + .access = LANDLOCK_ACCESS_FS_READ_FILE | > + LANDLOCK_ACCESS_FS_WRITE_FILE | > + LANDLOCK_ACCESS_FS_TRUNCATE, > + }, > + { > + .path = file_rw, > + .access = LANDLOCK_ACCESS_FS_READ_FILE | > + LANDLOCK_ACCESS_FS_WRITE_FILE, > + }, > + { > + .path = file_rt, > + .access = LANDLOCK_ACCESS_FS_READ_FILE | > + LANDLOCK_ACCESS_FS_TRUNCATE, > + }, > + { > + .path = file_t, > + .access = LANDLOCK_ACCESS_FS_TRUNCATE, > + }, > + // Implicitly: No access rights for file_none. Please use /* */ comments everywhere. > + { > + .path = dir_t, > + .access = LANDLOCK_ACCESS_FS_TRUNCATE, > + }, > + { > + .path = dir_w, > + .access = LANDLOCK_ACCESS_FS_WRITE_FILE, > + }, > + {}, > + }; > + const __u64 handled = LANDLOCK_ACCESS_FS_READ_FILE | > + LANDLOCK_ACCESS_FS_WRITE_FILE | > + LANDLOCK_ACCESS_FS_TRUNCATE; > + const int ruleset_fd = create_ruleset(_metadata, handled, rules); > + > + ASSERT_LE(0, ruleset_fd); > + enforce_ruleset(_metadata, ruleset_fd); > + ASSERT_EQ(0, close(ruleset_fd)); > + > + /* > + * Checks read, write and truncate rights: truncation works. > + * > + * Note: Independent of Landlock, ftruncate(2) on read-only > + * file descriptors never works. > + */ > + EXPECT_EQ(0, test_ftruncate(_metadata, file_rwt, O_WRONLY)); > + EXPECT_EQ(EINVAL, test_ftruncate(_metadata, file_rwt, O_RDONLY)); > + EXPECT_EQ(0, test_truncate(file_rwt)); > + EXPECT_EQ(0, test_open(file_rwt, O_WRONLY | O_TRUNC)); Could you please interleave the test_open() and test_ftruncate() with the same open flags, and start with test_open() to make sure assumptions are correct (cf. previous comment about test_ftruncate)? Like this (everywhere): EXPECT_EQ(0, test_open(file_rwt, O_RDONLY | O_TRUNC)); EXPECT_EQ(EINVAL, test_ftruncate(file_rwt, O_RDONLY)); EXPECT_EQ(0, test_open(file_rwt, O_WRONLY | O_TRUNC)); EXPECT_EQ(0, test_ftruncate(file_rwt, O_WRONLY)); EXPECT_EQ(0, test_truncate(file_rwt)); > + EXPECT_EQ(0, test_open(file_rwt, O_RDONLY | O_TRUNC)); > + > + /* Checks read and write rights: no truncate variant works. */ > + EXPECT_EQ(EACCES, test_ftruncate(_metadata, file_rw, O_WRONLY)); > + EXPECT_EQ(EINVAL, test_ftruncate(_metadata, file_rw, O_RDONLY)); > + EXPECT_EQ(EACCES, test_truncate(file_rw)); > + EXPECT_EQ(EACCES, test_open(file_rw, O_WRONLY | O_TRUNC)); > + EXPECT_EQ(EACCES, test_open(file_rw, O_RDONLY | O_TRUNC)); > + > + /* > + * Checks read and truncate rights: truncation works. > + * > + * Note: Files opened in O_RDONLY can get truncated as part of > + * the same operation. > + */ > + EXPECT_EQ(0, test_open(file_rt, O_RDONLY)); > + EXPECT_EQ(0, test_open(file_rt, O_RDONLY | O_TRUNC)); Why not a test_ftruncate() check here? > + EXPECT_EQ(EACCES, test_open(file_rt, O_WRONLY)); > + EXPECT_EQ(EACCES, test_open(file_rt, O_WRONLY)); There is a missing "| O_TRUNC" > + EXPECT_EQ(0, test_truncate(file_rt)); > + > + /* Checks truncate right: truncate works, but can't open file. */ > + EXPECT_EQ(EACCES, test_open(file_t, O_WRONLY)); > + EXPECT_EQ(EACCES, test_open(file_t, O_RDONLY)); > + EXPECT_EQ(EACCES, test_open(file_t, O_WRONLY | O_TRUNC)); > + EXPECT_EQ(EACCES, test_open(file_t, O_RDONLY | O_TRUNC)); > + EXPECT_EQ(0, test_truncate(file_t)); > + > + /* Checks "no rights" case: No form of truncation works. */ > + EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY)); > + EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY)); > + EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY | O_TRUNC)); > + EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY | O_TRUNC)); > + EXPECT_EQ(EACCES, test_truncate(file_none)); > + > + /* Checks truncate right on directory: truncate works on contained files */ > + EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_WRONLY)); > + EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_RDONLY)); > + EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_WRONLY | O_TRUNC)); > + EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_RDONLY | O_TRUNC)); > + EXPECT_EQ(0, test_truncate(file_in_dir_t)); > + > + /* > + * Checks creat in dir_w: This requires the truncate right > + * when overwriting an existing file, but does not require it > + * when the file is new. > + */ > + EXPECT_EQ(EACCES, test_creat(_metadata, file_in_dir_w, 0600)); > + > + ASSERT_EQ(0, unlink(file_in_dir_w)); > + EXPECT_EQ(0, test_creat(_metadata, file_in_dir_w, 0600)); > +} > + > +/* > + * Exercises file truncation when it's not restricted, > + * as it was the case before LANDLOCK_ACCESS_FS_TRUNCATE existed. > + */ > +TEST_F_FORK(layout1, truncate_unhandled) Can you move layout1.truncate_unhandled tests before layout1.truncate? > +{ > + const char *const file_r = file1_s1d1; > + const char *const file_w = file2_s1d1; > + const char *const file_none = file1_s1d2; > + const struct rule rules[] = { > + { > + .path = file_r, > + .access = LANDLOCK_ACCESS_FS_READ_FILE, > + }, > + { > + .path = file_w, > + .access = LANDLOCK_ACCESS_FS_WRITE_FILE, > + }, > + // Implicitly: No rights for file_none. > + {}, > + }; > + const __u64 handled = LANDLOCK_ACCESS_FS_READ_FILE | > + LANDLOCK_ACCESS_FS_WRITE_FILE; > + const int ruleset_fd = create_ruleset(_metadata, handled, rules); > + > + ASSERT_LE(0, ruleset_fd); > + enforce_ruleset(_metadata, ruleset_fd); > + ASSERT_EQ(0, close(ruleset_fd)); > + > + /* Checks read right: truncation should work through truncate and open. */ > + EXPECT_EQ(0, test_truncate(file_r)); We should be consistent to also check with test_ftruncate() (and order the EXPECT checks appropriately). > + EXPECT_EQ(0, test_open(file_r, O_RDONLY | O_TRUNC)); > + EXPECT_EQ(EACCES, test_open(file_r, O_WRONLY | O_TRUNC)); > + > + /* Checks write right: truncation should work through truncate, ftruncate and open. */ Please align to 80 columns. > + EXPECT_EQ(0, test_truncate(file_w)); > + EXPECT_EQ(0, test_ftruncate(_metadata, file_w, O_WRONLY)); > + EXPECT_EQ(EACCES, test_open(file_w, O_RDONLY | O_TRUNC)); > + EXPECT_EQ(0, test_open(file_w, O_WRONLY | O_TRUNC)); > + > + /* Checks "no rights" case: truncate works but all open attempts fail. */ > + EXPECT_EQ(0, test_truncate(file_none)); > + EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY | O_TRUNC)); > + EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY | O_TRUNC)); > + EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY)); Can you test with test_creat() here? > +} > + > /* clang-format off */ > FIXTURE(layout1_bind) {}; > /* clang-format on */
On Tue, Aug 16, 2022 at 07:08:20PM +0200, Mickaël Salaün wrote: > On 14/08/2022 21:26, Günther Noack wrote: > > +/* > > + * Opens the file and invokes ftruncate(2). > > + * > > + * Returns the errno of ftruncate if ftruncate() fails. > > + * Returns EBADFD if open() or close() fail (should not happen). > > + * Returns 0 if ftruncate(), open() and close() were successful. > > + */ > > +static int test_ftruncate(struct __test_metadata *const _metadata, > > _metadata is no longer needed. Same for test_creat(). Thanks, well spotted! > > > > + const char *const path, int flags) > > +{ > > + int res, err, fd; > > + > > + fd = open(path, flags | O_CLOEXEC); > > + if (fd < 0) > > + return EBADFD; > > Instead of EBADFD, which is a valid open(2) error, you can use ENOSYS and > add a comment explaining that we are not interested by this open(2) error > code but only the ftruncate(2) one because we are sure opening such path is > allowed or because open(2) is already tested before calls to > test_ftruncate(). Changed to ENOSYS and added a comment in the code and as function documentation. The explanation follows the reasoning that callers must guarantee that open() and close() will work, in order to test ftruncate() correctly. If open() or close() fail, we return ENOSYS. Technically EBADFD does not get returned by open(2) according to the man page, but I changed it to ENOSYS anyway (EBADF and EBADFD are easy to mix up). > > + > > + res = ftruncate(fd, 10); > > + err = errno; > > + > > + if (close(fd) != 0) > > + return EBADFD; > > Why not returning errno? See test_open_rel() comments. OK, changed to return errno for consistency, with the same comment. > > > > + > > + if (res < 0) > > + return err; > > + return 0; > > +} > > + > > +/* Invokes truncate(2) and returns the errno or 0. */ > > +static int test_truncate(const char *const path) > > +{ > > + if (truncate(path, 10) < 0) > > + return errno; > > + return 0; > > +} > > + > > +/* > > + * Invokes creat(2) and returns its errno or 0. > > + * Closes the opened file descriptor on success. > > + * Returns EBADFD if close() returns an error (should not happen). > > + */ > > +static int test_creat(struct __test_metadata *const _metadata, > > + const char *const path, mode_t mode) > > +{ > > + int fd = creat(path, mode); > > + > > + if (fd < 0) > > + return errno; > > + > > + if (close(fd) < 0) > > + return EBADFD; > > Why not returning errno? Done. (Same) > > + // Implicitly: No access rights for file_none. > > Please use /* */ comments everywhere. Done. (in both places) > > + /* > > + * Checks read, write and truncate rights: truncation works. > > + * > > + * Note: Independent of Landlock, ftruncate(2) on read-only > > + * file descriptors never works. > > + */ > > + EXPECT_EQ(0, test_ftruncate(_metadata, file_rwt, O_WRONLY)); > > + EXPECT_EQ(EINVAL, test_ftruncate(_metadata, file_rwt, O_RDONLY)); > > + EXPECT_EQ(0, test_truncate(file_rwt)); > > + EXPECT_EQ(0, test_open(file_rwt, O_WRONLY | O_TRUNC)); > > Could you please interleave the test_open() and test_ftruncate() with the > same open flags, and start with test_open() to make sure assumptions are > correct (cf. previous comment about test_ftruncate)? Like this (everywhere): > > EXPECT_EQ(0, test_open(file_rwt, O_RDONLY | O_TRUNC)); > EXPECT_EQ(EINVAL, test_ftruncate(file_rwt, O_RDONLY)); > EXPECT_EQ(0, test_open(file_rwt, O_WRONLY | O_TRUNC)); > EXPECT_EQ(0, test_ftruncate(file_rwt, O_WRONLY)); > EXPECT_EQ(0, test_truncate(file_rwt)); OK, I ordered it like that. When calling EXPECT_EQ(foo, test_ftruncate(...)); and "foo" is not ENOSYS (which it should never be!), then that alone is enough to guarantee that the open() in test_ftruncate worked, so in a sense this is already tested implicitly. The only thing to make sure is to never *expect* ENOSYS from test_ftruncate(), but I documented in test_ftruncate() now that it is better to use test_open() for testing the result of open(). > > > > + EXPECT_EQ(0, test_open(file_rwt, O_RDONLY | O_TRUNC)); > > + > > + /* Checks read and write rights: no truncate variant works. */ > > + EXPECT_EQ(EACCES, test_ftruncate(_metadata, file_rw, O_WRONLY)); > > + EXPECT_EQ(EINVAL, test_ftruncate(_metadata, file_rw, O_RDONLY)); > > + EXPECT_EQ(EACCES, test_truncate(file_rw)); > > + EXPECT_EQ(EACCES, test_open(file_rw, O_WRONLY | O_TRUNC)); > > + EXPECT_EQ(EACCES, test_open(file_rw, O_RDONLY | O_TRUNC)); > > + > > + /* > > + * Checks read and truncate rights: truncation works. > > + * > > + * Note: Files opened in O_RDONLY can get truncated as part of > > + * the same operation. > > + */ > > + EXPECT_EQ(0, test_open(file_rt, O_RDONLY)); > > + EXPECT_EQ(0, test_open(file_rt, O_RDONLY | O_TRUNC)); > > Why not a test_ftruncate() check here? The twist here is, althrough truncate() works, ftruncate() cannot truncate the file in this case: (a) when trying to open the file for writing, Landlock policy keeps you from doing that (tested with test_open). (b) when opening the file read-only, that works, but read-only fds can never be ftruncated (explained in the man page). It's slightly surprising when just glancing over the test, so I added the check for extra clarity: EXPECT_EQ(EINVAL, test_ftruncate(file_rt, O_RDONLY)); The check for test_open with O_WRONLY was already there. > > + EXPECT_EQ(EACCES, test_open(file_rt, O_WRONLY)); > > + EXPECT_EQ(EACCES, test_open(file_rt, O_WRONLY)); > > There is a missing "| O_TRUNC" Well spotted! Fixed. > > + EXPECT_EQ(0, test_truncate(file_rt)); > > + > > + /* Checks truncate right: truncate works, but can't open file. */ > > + EXPECT_EQ(EACCES, test_open(file_t, O_WRONLY)); > > + EXPECT_EQ(EACCES, test_open(file_t, O_RDONLY)); > > + EXPECT_EQ(EACCES, test_open(file_t, O_WRONLY | O_TRUNC)); > > + EXPECT_EQ(EACCES, test_open(file_t, O_RDONLY | O_TRUNC)); > > + EXPECT_EQ(0, test_truncate(file_t)); > > + > > + /* Checks "no rights" case: No form of truncation works. */ > > + EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY)); > > + EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY)); > > + EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY | O_TRUNC)); > > + EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY | O_TRUNC)); > > + EXPECT_EQ(EACCES, test_truncate(file_none)); > > + > > + /* Checks truncate right on directory: truncate works on contained files */ > > + EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_WRONLY)); > > + EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_RDONLY)); > > + EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_WRONLY | O_TRUNC)); > > + EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_RDONLY | O_TRUNC)); > > + EXPECT_EQ(0, test_truncate(file_in_dir_t)); > > + > > + /* > > + * Checks creat in dir_w: This requires the truncate right > > + * when overwriting an existing file, but does not require it > > + * when the file is new. > > + */ > > + EXPECT_EQ(EACCES, test_creat(_metadata, file_in_dir_w, 0600)); > > + > > + ASSERT_EQ(0, unlink(file_in_dir_w)); > > + EXPECT_EQ(0, test_creat(_metadata, file_in_dir_w, 0600)); > > +} > > + > > +/* > > + * Exercises file truncation when it's not restricted, > > + * as it was the case before LANDLOCK_ACCESS_FS_TRUNCATE existed. > > + */ > > +TEST_F_FORK(layout1, truncate_unhandled) > > Can you move layout1.truncate_unhandled tests before layout1.truncate? Done. > > > > +{ > > + const char *const file_r = file1_s1d1; > > + const char *const file_w = file2_s1d1; > > + const char *const file_none = file1_s1d2; > > + const struct rule rules[] = { > > + { > > + .path = file_r, > > + .access = LANDLOCK_ACCESS_FS_READ_FILE, > > + }, > > + { > > + .path = file_w, > > + .access = LANDLOCK_ACCESS_FS_WRITE_FILE, > > + }, > > + // Implicitly: No rights for file_none. > > + {}, > > + }; > > + const __u64 handled = LANDLOCK_ACCESS_FS_READ_FILE | > > + LANDLOCK_ACCESS_FS_WRITE_FILE; > > + const int ruleset_fd = create_ruleset(_metadata, handled, rules); > > + > > + ASSERT_LE(0, ruleset_fd); > > + enforce_ruleset(_metadata, ruleset_fd); > > + ASSERT_EQ(0, close(ruleset_fd)); > > + > > + /* Checks read right: truncation should work through truncate and open. */ > > + EXPECT_EQ(0, test_truncate(file_r)); > > We should be consistent to also check with test_ftruncate() (and order the > EXPECT checks appropriately). Added. > > > > + EXPECT_EQ(0, test_open(file_r, O_RDONLY | O_TRUNC)); > > + EXPECT_EQ(EACCES, test_open(file_r, O_WRONLY | O_TRUNC)); > > + > > + /* Checks write right: truncation should work through truncate, ftruncate and open. */ > > Please align to 80 columns. Done. > > > > + EXPECT_EQ(0, test_truncate(file_w)); > > + EXPECT_EQ(0, test_ftruncate(_metadata, file_w, O_WRONLY)); > > + EXPECT_EQ(EACCES, test_open(file_w, O_RDONLY | O_TRUNC)); > > + EXPECT_EQ(0, test_open(file_w, O_WRONLY | O_TRUNC)); > > + > > + /* Checks "no rights" case: truncate works but all open attempts fail. */ > > + EXPECT_EQ(0, test_truncate(file_none)); > > + EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY | O_TRUNC)); > > + EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY | O_TRUNC)); > > + EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY)); > > Can you test with test_creat() here? Added. Thanks for the careful review! —Günther --
On Wed, Aug 17, 2022 at 08:00:17PM +0200, Günther Noack wrote: > On Tue, Aug 16, 2022 at 07:08:20PM +0200, Mickaël Salaün wrote: > > On 14/08/2022 21:26, Günther Noack wrote: > > > +/* > > > + * Opens the file and invokes ftruncate(2). > > > + * > > > + * Returns the errno of ftruncate if ftruncate() fails. > > > + * Returns EBADFD if open() or close() fail (should not happen). > > > + * Returns 0 if ftruncate(), open() and close() were successful. > > > + */ > > > +static int test_ftruncate(struct __test_metadata *const _metadata, > > > > _metadata is no longer needed. Same for test_creat(). > > Thanks, well spotted! > > > > > > > > + const char *const path, int flags) > > > +{ > > > + int res, err, fd; > > > + > > > + fd = open(path, flags | O_CLOEXEC); > > > + if (fd < 0) > > > + return EBADFD; > > > > Instead of EBADFD, which is a valid open(2) error, you can use ENOSYS and > > add a comment explaining that we are not interested by this open(2) error > > code but only the ftruncate(2) one because we are sure opening such path is > > allowed or because open(2) is already tested before calls to > > test_ftruncate(). > > Changed to ENOSYS and added a comment in the code and as function documentation. > > The explanation follows the reasoning that callers must guarantee that > open() and close() will work, in order to test ftruncate() correctly. > If open() or close() fail, we return ENOSYS. > > Technically EBADFD does not get returned by open(2) according to the > man page, but I changed it to ENOSYS anyway (EBADF and EBADFD are easy > to mix up). The more I think about it, the more I feel that test_ftruncate() in its current form was a mistake: * In reality, we just care about the ftruncate() result, not about open(). * The tests became slightly confusing and asymmetric because some places could call test_ftruncate() while others would call test_open() * Trying open(..., O_RDONLY) + ftruncate() is also confusing in tests, because that never works anyway (ftruncate() only works on writable fds) So: I'm contemplating to use a different approach instead: * Open a writable FD for each file *before enforcing Landlock*. * *Then* enforce Landlock (now some of these files can't be opened any more) * Then try ftruncate() with the previously opened file descriptor. As a result, * we have some new repetitive but simple code for opening file descriptors * we remove the long version of test_ftruncate() with all its error case complication and replace it with a trivial one that takes an already-opened file descriptor. This way, each block in the test now checks the following things: * check truncate(file) * check ftruncate(file_fd) <--- passing the FD! * check open(file, O_RDONLY|O_TRUNC) * check open(file, O_WRONLY|O_TRUNC) It's now easy to see in the tests that the result from truncate() and ftruncate() is always the same. The complication of worrying whether open() works before ftruncate() is also gone (so no more special open() checks needed before checking ftruncate()). I removed the testing of ftruncate() on read-only file descriptors, because that is forbidden in the ftruncate() manpage anyway and always returns EINVAL independent of Landlock. I feel that this helps clarify the tests, even if it undoes some of your comments about expecting open() to work before ftruncate(). Does that approach look reasonable to you? I might just send you a patch version with that variant I think - this might be clearer in code than in my textual description here. :) —Günther > > > > + > > > + res = ftruncate(fd, 10); > > > + err = errno; > > > + > > > + if (close(fd) != 0) > > > + return EBADFD; > > > > Why not returning errno? See test_open_rel() comments. > > OK, changed to return errno for consistency, with the same comment. > > > > > > > > + > > > + if (res < 0) > > > + return err; > > > + return 0; > > > +} > > > + > > > +/* Invokes truncate(2) and returns the errno or 0. */ > > > +static int test_truncate(const char *const path) > > > +{ > > > + if (truncate(path, 10) < 0) > > > + return errno; > > > + return 0; > > > +} > > > + > > > +/* > > > + * Invokes creat(2) and returns its errno or 0. > > > + * Closes the opened file descriptor on success. > > > + * Returns EBADFD if close() returns an error (should not happen). > > > + */ > > > +static int test_creat(struct __test_metadata *const _metadata, > > > + const char *const path, mode_t mode) > > > +{ > > > + int fd = creat(path, mode); > > > + > > > + if (fd < 0) > > > + return errno; > > > + > > > + if (close(fd) < 0) > > > + return EBADFD; > > > > Why not returning errno? > > Done. (Same) > > > > + // Implicitly: No access rights for file_none. > > > > Please use /* */ comments everywhere. > > Done. (in both places) > > > > + /* > > > + * Checks read, write and truncate rights: truncation works. > > > + * > > > + * Note: Independent of Landlock, ftruncate(2) on read-only > > > + * file descriptors never works. > > > + */ > > > + EXPECT_EQ(0, test_ftruncate(_metadata, file_rwt, O_WRONLY)); > > > + EXPECT_EQ(EINVAL, test_ftruncate(_metadata, file_rwt, O_RDONLY)); > > > + EXPECT_EQ(0, test_truncate(file_rwt)); > > > + EXPECT_EQ(0, test_open(file_rwt, O_WRONLY | O_TRUNC)); > > > > Could you please interleave the test_open() and test_ftruncate() with the > > same open flags, and start with test_open() to make sure assumptions are > > correct (cf. previous comment about test_ftruncate)? Like this (everywhere): > > > > EXPECT_EQ(0, test_open(file_rwt, O_RDONLY | O_TRUNC)); > > EXPECT_EQ(EINVAL, test_ftruncate(file_rwt, O_RDONLY)); > > EXPECT_EQ(0, test_open(file_rwt, O_WRONLY | O_TRUNC)); > > EXPECT_EQ(0, test_ftruncate(file_rwt, O_WRONLY)); > > EXPECT_EQ(0, test_truncate(file_rwt)); > > OK, I ordered it like that. > > When calling > > EXPECT_EQ(foo, test_ftruncate(...)); > > and "foo" is not ENOSYS (which it should never be!), then that alone > is enough to guarantee that the open() in test_ftruncate worked, so in > a sense this is already tested implicitly. > > The only thing to make sure is to never *expect* ENOSYS from > test_ftruncate(), but I documented in test_ftruncate() now that it is > better to use test_open() for testing the result of open(). > > > > > > > > + EXPECT_EQ(0, test_open(file_rwt, O_RDONLY | O_TRUNC)); > > > + > > > + /* Checks read and write rights: no truncate variant works. */ > > > + EXPECT_EQ(EACCES, test_ftruncate(_metadata, file_rw, O_WRONLY)); > > > + EXPECT_EQ(EINVAL, test_ftruncate(_metadata, file_rw, O_RDONLY)); > > > + EXPECT_EQ(EACCES, test_truncate(file_rw)); > > > + EXPECT_EQ(EACCES, test_open(file_rw, O_WRONLY | O_TRUNC)); > > > + EXPECT_EQ(EACCES, test_open(file_rw, O_RDONLY | O_TRUNC)); > > > + > > > + /* > > > + * Checks read and truncate rights: truncation works. > > > + * > > > + * Note: Files opened in O_RDONLY can get truncated as part of > > > + * the same operation. > > > + */ > > > + EXPECT_EQ(0, test_open(file_rt, O_RDONLY)); > > > + EXPECT_EQ(0, test_open(file_rt, O_RDONLY | O_TRUNC)); > > > > Why not a test_ftruncate() check here? > > The twist here is, althrough truncate() works, ftruncate() cannot > truncate the file in this case: > > (a) when trying to open the file for writing, Landlock policy keeps > you from doing that (tested with test_open). > > (b) when opening the file read-only, that works, but read-only fds can > never be ftruncated (explained in the man page). > > It's slightly surprising when just glancing over the test, so I added > the check for extra clarity: > > EXPECT_EQ(EINVAL, test_ftruncate(file_rt, O_RDONLY)); > > The check for test_open with O_WRONLY was already there. > > > > + EXPECT_EQ(EACCES, test_open(file_rt, O_WRONLY)); > > > + EXPECT_EQ(EACCES, test_open(file_rt, O_WRONLY)); > > > > There is a missing "| O_TRUNC" > > Well spotted! Fixed. > > > > + EXPECT_EQ(0, test_truncate(file_rt)); > > > + > > > + /* Checks truncate right: truncate works, but can't open file. */ > > > + EXPECT_EQ(EACCES, test_open(file_t, O_WRONLY)); > > > + EXPECT_EQ(EACCES, test_open(file_t, O_RDONLY)); > > > + EXPECT_EQ(EACCES, test_open(file_t, O_WRONLY | O_TRUNC)); > > > + EXPECT_EQ(EACCES, test_open(file_t, O_RDONLY | O_TRUNC)); > > > + EXPECT_EQ(0, test_truncate(file_t)); > > > + > > > + /* Checks "no rights" case: No form of truncation works. */ > > > + EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY)); > > > + EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY)); > > > + EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY | O_TRUNC)); > > > + EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY | O_TRUNC)); > > > + EXPECT_EQ(EACCES, test_truncate(file_none)); > > > + > > > + /* Checks truncate right on directory: truncate works on contained files */ > > > + EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_WRONLY)); > > > + EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_RDONLY)); > > > + EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_WRONLY | O_TRUNC)); > > > + EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_RDONLY | O_TRUNC)); > > > + EXPECT_EQ(0, test_truncate(file_in_dir_t)); > > > + > > > + /* > > > + * Checks creat in dir_w: This requires the truncate right > > > + * when overwriting an existing file, but does not require it > > > + * when the file is new. > > > + */ > > > + EXPECT_EQ(EACCES, test_creat(_metadata, file_in_dir_w, 0600)); > > > + > > > + ASSERT_EQ(0, unlink(file_in_dir_w)); > > > + EXPECT_EQ(0, test_creat(_metadata, file_in_dir_w, 0600)); > > > +} > > > + > > > +/* > > > + * Exercises file truncation when it's not restricted, > > > + * as it was the case before LANDLOCK_ACCESS_FS_TRUNCATE existed. > > > + */ > > > +TEST_F_FORK(layout1, truncate_unhandled) > > > > Can you move layout1.truncate_unhandled tests before layout1.truncate? > > Done. > > > > > > > > +{ > > > + const char *const file_r = file1_s1d1; > > > + const char *const file_w = file2_s1d1; > > > + const char *const file_none = file1_s1d2; > > > + const struct rule rules[] = { > > > + { > > > + .path = file_r, > > > + .access = LANDLOCK_ACCESS_FS_READ_FILE, > > > + }, > > > + { > > > + .path = file_w, > > > + .access = LANDLOCK_ACCESS_FS_WRITE_FILE, > > > + }, > > > + // Implicitly: No rights for file_none. > > > + {}, > > > + }; > > > + const __u64 handled = LANDLOCK_ACCESS_FS_READ_FILE | > > > + LANDLOCK_ACCESS_FS_WRITE_FILE; > > > + const int ruleset_fd = create_ruleset(_metadata, handled, rules); > > > + > > > + ASSERT_LE(0, ruleset_fd); > > > + enforce_ruleset(_metadata, ruleset_fd); > > > + ASSERT_EQ(0, close(ruleset_fd)); > > > + > > > + /* Checks read right: truncation should work through truncate and open. */ > > > + EXPECT_EQ(0, test_truncate(file_r)); > > > > We should be consistent to also check with test_ftruncate() (and order the > > EXPECT checks appropriately). > > Added. > > > > > > > > + EXPECT_EQ(0, test_open(file_r, O_RDONLY | O_TRUNC)); > > > + EXPECT_EQ(EACCES, test_open(file_r, O_WRONLY | O_TRUNC)); > > > + > > > + /* Checks write right: truncation should work through truncate, ftruncate and open. */ > > > > Please align to 80 columns. > > Done. > > > > > > > > + EXPECT_EQ(0, test_truncate(file_w)); > > > + EXPECT_EQ(0, test_ftruncate(_metadata, file_w, O_WRONLY)); > > > + EXPECT_EQ(EACCES, test_open(file_w, O_RDONLY | O_TRUNC)); > > > + EXPECT_EQ(0, test_open(file_w, O_WRONLY | O_TRUNC)); > > > + > > > + /* Checks "no rights" case: truncate works but all open attempts fail. */ > > > + EXPECT_EQ(0, test_truncate(file_none)); > > > + EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY | O_TRUNC)); > > > + EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY | O_TRUNC)); > > > + EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY)); > > > > Can you test with test_creat() here? > > Added. > > Thanks for the careful review! > —Günther > > -- --
On 17/08/2022 21:35, Günther Noack wrote: > On Wed, Aug 17, 2022 at 08:00:17PM +0200, Günther Noack wrote: >> On Tue, Aug 16, 2022 at 07:08:20PM +0200, Mickaël Salaün wrote: >>> On 14/08/2022 21:26, Günther Noack wrote: >>>> +/* >>>> + * Opens the file and invokes ftruncate(2). >>>> + * >>>> + * Returns the errno of ftruncate if ftruncate() fails. >>>> + * Returns EBADFD if open() or close() fail (should not happen). >>>> + * Returns 0 if ftruncate(), open() and close() were successful. >>>> + */ >>>> +static int test_ftruncate(struct __test_metadata *const _metadata, >>> >>> _metadata is no longer needed. Same for test_creat(). >> >> Thanks, well spotted! >> >>> >>> >>>> + const char *const path, int flags) >>>> +{ >>>> + int res, err, fd; >>>> + >>>> + fd = open(path, flags | O_CLOEXEC); >>>> + if (fd < 0) >>>> + return EBADFD; >>> >>> Instead of EBADFD, which is a valid open(2) error, you can use ENOSYS and >>> add a comment explaining that we are not interested by this open(2) error >>> code but only the ftruncate(2) one because we are sure opening such path is >>> allowed or because open(2) is already tested before calls to >>> test_ftruncate(). >> >> Changed to ENOSYS and added a comment in the code and as function documentation. >> >> The explanation follows the reasoning that callers must guarantee that >> open() and close() will work, in order to test ftruncate() correctly. >> If open() or close() fail, we return ENOSYS. >> >> Technically EBADFD does not get returned by open(2) according to the >> man page, but I changed it to ENOSYS anyway (EBADF and EBADFD are easy >> to mix up). > > The more I think about it, the more I feel that test_ftruncate() in its current > form was a mistake: > > * In reality, we just care about the ftruncate() result, not about open(). > * The tests became slightly confusing and asymmetric because some > places could call test_ftruncate() while others would call test_open() > * Trying open(..., O_RDONLY) + ftruncate() is also confusing in tests, > because that never works anyway (ftruncate() only works on writable fds) > > So: I'm contemplating to use a different approach instead: > > * Open a writable FD for each file *before enforcing Landlock*. > * *Then* enforce Landlock (now some of these files can't be opened any more) > * Then try ftruncate() with the previously opened file descriptor. > > As a result, > > * we have some new repetitive but simple code for opening file descriptors > * we remove the long version of test_ftruncate() with all its error case > complication and replace it with a trivial one that takes an already-opened > file descriptor. > > This way, each block in the test now checks the following things: > > * check truncate(file) > * check ftruncate(file_fd) <--- passing the FD! > * check open(file, O_RDONLY|O_TRUNC) > * check open(file, O_WRONLY|O_TRUNC) > > It's now easy to see in the tests that the result from truncate() and > ftruncate() is always the same. The complication of worrying whether open() > works before ftruncate() is also gone (so no more special open() checks needed > before checking ftruncate()). I removed the testing of ftruncate() on read-only > file descriptors, because that is forbidden in the ftruncate() manpage anyway > and always returns EINVAL independent of Landlock. > > I feel that this helps clarify the tests, even if it undoes some of your > comments about expecting open() to work before ftruncate(). > > Does that approach look reasonable to you? > > I might just send you a patch version with that variant I think - this might be > clearer in code than in my textual description here. :) The FD from the pre-landlocked state is the right approach, thanks!
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c index cb77eaa01c91..7a2ce6cd1a5a 100644 --- a/tools/testing/selftests/landlock/fs_test.c +++ b/tools/testing/selftests/landlock/fs_test.c @@ -58,6 +58,7 @@ static const char file1_s2d3[] = TMP_DIR "/s2d1/s2d2/s2d3/f1"; static const char file2_s2d3[] = TMP_DIR "/s2d1/s2d2/s2d3/f2"; static const char dir_s3d1[] = TMP_DIR "/s3d1"; +static const char file1_s3d1[] = TMP_DIR "/s3d1/f1"; /* dir_s3d2 is a mount point. */ static const char dir_s3d2[] = TMP_DIR "/s3d1/s3d2"; static const char dir_s3d3[] = TMP_DIR "/s3d1/s3d2/s3d3"; @@ -83,6 +84,7 @@ static const char dir_s3d3[] = TMP_DIR "/s3d1/s3d2/s3d3"; * │ ├── f1 * │ └── f2 * └── s3d1 + * ├── f1 * └── s3d2 * └── s3d3 */ @@ -208,6 +210,7 @@ static void create_layout1(struct __test_metadata *const _metadata) create_file(_metadata, file1_s2d3); create_file(_metadata, file2_s2d3); + create_file(_metadata, file1_s3d1); create_directory(_metadata, dir_s3d2); set_cap(_metadata, CAP_SYS_ADMIN); ASSERT_EQ(0, mount("tmp", dir_s3d2, "tmpfs", 0, "size=4m,mode=700")); @@ -230,6 +233,7 @@ static void remove_layout1(struct __test_metadata *const _metadata) EXPECT_EQ(0, remove_path(file1_s2d2)); EXPECT_EQ(0, remove_path(file1_s2d1)); + EXPECT_EQ(0, remove_path(file1_s3d1)); EXPECT_EQ(0, remove_path(dir_s3d3)); set_cap(_metadata, CAP_SYS_ADMIN); umount(dir_s3d2); @@ -3023,6 +3027,221 @@ TEST_F_FORK(layout1, proc_pipe) ASSERT_EQ(0, close(pipe_fds[1])); } +/* + * Opens the file and invokes ftruncate(2). + * + * Returns the errno of ftruncate if ftruncate() fails. + * Returns EBADFD if open() or close() fail (should not happen). + * Returns 0 if ftruncate(), open() and close() were successful. + */ +static int test_ftruncate(struct __test_metadata *const _metadata, + const char *const path, int flags) +{ + int res, err, fd; + + fd = open(path, flags | O_CLOEXEC); + if (fd < 0) + return EBADFD; + + res = ftruncate(fd, 10); + err = errno; + + if (close(fd) != 0) + return EBADFD; + + if (res < 0) + return err; + return 0; +} + +/* Invokes truncate(2) and returns the errno or 0. */ +static int test_truncate(const char *const path) +{ + if (truncate(path, 10) < 0) + return errno; + return 0; +} + +/* + * Invokes creat(2) and returns its errno or 0. + * Closes the opened file descriptor on success. + * Returns EBADFD if close() returns an error (should not happen). + */ +static int test_creat(struct __test_metadata *const _metadata, + const char *const path, mode_t mode) +{ + int fd = creat(path, mode); + + if (fd < 0) + return errno; + + if (close(fd) < 0) + return EBADFD; + return 0; +} + +TEST_F_FORK(layout1, truncate) +{ + const char *const file_rwt = file1_s1d1; + const char *const file_rw = file2_s1d1; + const char *const file_rt = file1_s1d2; + const char *const file_t = file2_s1d2; + const char *const file_none = file1_s1d3; + const char *const dir_t = dir_s2d1; + const char *const file_in_dir_t = file1_s2d1; + const char *const dir_w = dir_s3d1; + const char *const file_in_dir_w = file1_s3d1; + const struct rule rules[] = { + { + .path = file_rwt, + .access = LANDLOCK_ACCESS_FS_READ_FILE | + LANDLOCK_ACCESS_FS_WRITE_FILE | + LANDLOCK_ACCESS_FS_TRUNCATE, + }, + { + .path = file_rw, + .access = LANDLOCK_ACCESS_FS_READ_FILE | + LANDLOCK_ACCESS_FS_WRITE_FILE, + }, + { + .path = file_rt, + .access = LANDLOCK_ACCESS_FS_READ_FILE | + LANDLOCK_ACCESS_FS_TRUNCATE, + }, + { + .path = file_t, + .access = LANDLOCK_ACCESS_FS_TRUNCATE, + }, + // Implicitly: No access rights for file_none. + { + .path = dir_t, + .access = LANDLOCK_ACCESS_FS_TRUNCATE, + }, + { + .path = dir_w, + .access = LANDLOCK_ACCESS_FS_WRITE_FILE, + }, + {}, + }; + const __u64 handled = LANDLOCK_ACCESS_FS_READ_FILE | + LANDLOCK_ACCESS_FS_WRITE_FILE | + LANDLOCK_ACCESS_FS_TRUNCATE; + const int ruleset_fd = create_ruleset(_metadata, handled, rules); + + ASSERT_LE(0, ruleset_fd); + enforce_ruleset(_metadata, ruleset_fd); + ASSERT_EQ(0, close(ruleset_fd)); + + /* + * Checks read, write and truncate rights: truncation works. + * + * Note: Independent of Landlock, ftruncate(2) on read-only + * file descriptors never works. + */ + EXPECT_EQ(0, test_ftruncate(_metadata, file_rwt, O_WRONLY)); + EXPECT_EQ(EINVAL, test_ftruncate(_metadata, file_rwt, O_RDONLY)); + EXPECT_EQ(0, test_truncate(file_rwt)); + EXPECT_EQ(0, test_open(file_rwt, O_WRONLY | O_TRUNC)); + EXPECT_EQ(0, test_open(file_rwt, O_RDONLY | O_TRUNC)); + + /* Checks read and write rights: no truncate variant works. */ + EXPECT_EQ(EACCES, test_ftruncate(_metadata, file_rw, O_WRONLY)); + EXPECT_EQ(EINVAL, test_ftruncate(_metadata, file_rw, O_RDONLY)); + EXPECT_EQ(EACCES, test_truncate(file_rw)); + EXPECT_EQ(EACCES, test_open(file_rw, O_WRONLY | O_TRUNC)); + EXPECT_EQ(EACCES, test_open(file_rw, O_RDONLY | O_TRUNC)); + + /* + * Checks read and truncate rights: truncation works. + * + * Note: Files opened in O_RDONLY can get truncated as part of + * the same operation. + */ + EXPECT_EQ(0, test_open(file_rt, O_RDONLY)); + EXPECT_EQ(0, test_open(file_rt, O_RDONLY | O_TRUNC)); + EXPECT_EQ(EACCES, test_open(file_rt, O_WRONLY)); + EXPECT_EQ(EACCES, test_open(file_rt, O_WRONLY)); + EXPECT_EQ(0, test_truncate(file_rt)); + + /* Checks truncate right: truncate works, but can't open file. */ + EXPECT_EQ(EACCES, test_open(file_t, O_WRONLY)); + EXPECT_EQ(EACCES, test_open(file_t, O_RDONLY)); + EXPECT_EQ(EACCES, test_open(file_t, O_WRONLY | O_TRUNC)); + EXPECT_EQ(EACCES, test_open(file_t, O_RDONLY | O_TRUNC)); + EXPECT_EQ(0, test_truncate(file_t)); + + /* Checks "no rights" case: No form of truncation works. */ + EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY)); + EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY)); + EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY | O_TRUNC)); + EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY | O_TRUNC)); + EXPECT_EQ(EACCES, test_truncate(file_none)); + + /* Checks truncate right on directory: truncate works on contained files */ + EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_WRONLY)); + EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_RDONLY)); + EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_WRONLY | O_TRUNC)); + EXPECT_EQ(EACCES, test_open(file_in_dir_t, O_RDONLY | O_TRUNC)); + EXPECT_EQ(0, test_truncate(file_in_dir_t)); + + /* + * Checks creat in dir_w: This requires the truncate right + * when overwriting an existing file, but does not require it + * when the file is new. + */ + EXPECT_EQ(EACCES, test_creat(_metadata, file_in_dir_w, 0600)); + + ASSERT_EQ(0, unlink(file_in_dir_w)); + EXPECT_EQ(0, test_creat(_metadata, file_in_dir_w, 0600)); +} + +/* + * Exercises file truncation when it's not restricted, + * as it was the case before LANDLOCK_ACCESS_FS_TRUNCATE existed. + */ +TEST_F_FORK(layout1, truncate_unhandled) +{ + const char *const file_r = file1_s1d1; + const char *const file_w = file2_s1d1; + const char *const file_none = file1_s1d2; + const struct rule rules[] = { + { + .path = file_r, + .access = LANDLOCK_ACCESS_FS_READ_FILE, + }, + { + .path = file_w, + .access = LANDLOCK_ACCESS_FS_WRITE_FILE, + }, + // Implicitly: No rights for file_none. + {}, + }; + const __u64 handled = LANDLOCK_ACCESS_FS_READ_FILE | + LANDLOCK_ACCESS_FS_WRITE_FILE; + const int ruleset_fd = create_ruleset(_metadata, handled, rules); + + ASSERT_LE(0, ruleset_fd); + enforce_ruleset(_metadata, ruleset_fd); + ASSERT_EQ(0, close(ruleset_fd)); + + /* Checks read right: truncation should work through truncate and open. */ + EXPECT_EQ(0, test_truncate(file_r)); + EXPECT_EQ(0, test_open(file_r, O_RDONLY | O_TRUNC)); + EXPECT_EQ(EACCES, test_open(file_r, O_WRONLY | O_TRUNC)); + + /* Checks write right: truncation should work through truncate, ftruncate and open. */ + EXPECT_EQ(0, test_truncate(file_w)); + EXPECT_EQ(0, test_ftruncate(_metadata, file_w, O_WRONLY)); + EXPECT_EQ(EACCES, test_open(file_w, O_RDONLY | O_TRUNC)); + EXPECT_EQ(0, test_open(file_w, O_WRONLY | O_TRUNC)); + + /* Checks "no rights" case: truncate works but all open attempts fail. */ + EXPECT_EQ(0, test_truncate(file_none)); + EXPECT_EQ(EACCES, test_open(file_none, O_RDONLY | O_TRUNC)); + EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY | O_TRUNC)); + EXPECT_EQ(EACCES, test_open(file_none, O_WRONLY)); +} + /* clang-format off */ FIXTURE(layout1_bind) {}; /* clang-format on */
These tests exercise the following truncation operations: * truncate() (truncate by path) * ftruncate() (truncate by file descriptor) * open with the O_TRUNC flag * special case: creat(), which is open with O_CREAT|O_WRONLY|O_TRUNC. in the following scenarios: * Files with read, write and truncate rights. * Files with read and truncate rights. * Files with the truncate right. * Files without the truncate right. In particular, the following scenarios are enforced with the test: * The truncate right is required to use ftruncate, even when the thread already has the right to write to the file. * open() with O_TRUNC requires the truncate right, if it truncates a file. open() already checks security_path_truncate() in this case, and it required no additional check in the Landlock LSM's file_open hook. * creat() requires the truncate right when called with an existing filename. * creat() does *not* require the truncate right when it's creating a new file. Signed-off-by: Günther Noack <gnoack3000@gmail.com> --- tools/testing/selftests/landlock/fs_test.c | 219 +++++++++++++++++++++ 1 file changed, 219 insertions(+)