ext4: fix fallocate to use file_modified to update permissions consistently
PATCH 1/1 ext4: fix fallocate to use file_modified to update permissions consistently
From: Darrick J. Wong <[email protected]>
Since the initial introduction of (posix) fallocate back at the turn of the century, it has been possible to use this syscall to change the user-visible contents of files. This can happen by extending the file size during a preallocation, or through any of the newer modes (punch, zero, collapse, insert range). Because the call can be used to change file contents, we should treat it like we do any other modification to a file -- update the mtime, and drop set[ug]id privileges/capabilities.
The VFS function file_modified() does all this for us if pass it a locked inode, so let's make fallocate drop permissions correctly.
--- fs/ext4/ext4.h | 2 +- fs/ext4/extents.c | 32 +++++++++++++++++++++++++------- fs/ext4/inode.c | 7 ++++++- 3 files changed, 32 insertions(+), 9 deletions(-)
fs/ext4/ext4.h +1 -1
@@ -3030,7 +3030,7 @@ extern int ext4_inode_attach_jinode(struct inode *inode);extern int ext4_can_truncate (struct inode *inode);extern int ext4_truncate (struct inode *);extern int ext4_break_layouts (struct inode *); -extern int ext4_punch_hole (struct inode *inode,loff_t offset,loff_t length); +extern int ext4_punch_hole (struct file *file,loff_t offset,loff_t length);extern void ext4_set_inode_flags (struct inode *,bool init);extern int ext4_alloc_da_blocks (struct inode *inode);extern void ext4_set_aops (struct inode *inode);
fs/ext4/extents.c +25 -7
@@ -4501,9 +4501,9 @@ static int ext4_alloc_file_blocks(struct file *file, ext4_lblk_t offset,return ret >0 ? ret2 : ret; } -static int ext4_collapse_range (struct inode *inode,loff_t offset,loff_t len); +static int ext4_collapse_range (struct file *file,loff_t offset,loff_t len); -static int ext4_insert_range (struct inode *inode,loff_t offset,loff_t len); +static int ext4_insert_range (struct file *file,loff_t offset,loff_t len);static long ext4_zero_range (struct file *file,loff_t offset,loff_t len,int mode)
@@ -4575,6 +4575,10 @@ static long ext4_zero_range(struct file *file, loff_t offset,/* Wait all existing dio workers, newcomers will block on i_rwsem */ inode_dio_wait (inode); + ret =file_modified (file); +if (ret) + goto out_mutex; +/* Preallocate the range including the unaligned edges */ if (partial_begin || partial_end) { ret = ext4_alloc_file_blocks(file,
@@ -4691,7 +4695,7 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)return -EOPNOTSUPP ;if (mode &FALLOC_FL_PUNCH_HOLE ) { - ret =ext4_punch_hole (inode, offset, len); + ret =ext4_punch_hole (file, offset, len); goto exit; }
@@ -4700,12 +4704,12 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len) goto exit;if (mode &FALLOC_FL_COLLAPSE_RANGE ) { - ret =ext4_collapse_range (inode, offset, len); + ret =ext4_collapse_range (file, offset, len); goto exit; }if (mode &FALLOC_FL_INSERT_RANGE ) { - ret =ext4_insert_range (inode, offset, len); + ret =ext4_insert_range (file, offset, len); goto exit; }
@@ -4741,6 +4745,10 @@ long ext4_fallocate(struct file *file, int mode, loff_t offset, loff_t len)/* Wait all existing dio workers, newcomers will block on i_rwsem */ inode_dio_wait (inode); + ret =file_modified (file); +if (ret) + goto out; + ret =ext4_alloc_file_blocks (file, lblk, max_blocks, new_size, flags);if (ret) goto out;
@@ -5242,8 +5250,9 @@ ext4_ext_shift_extents(struct inode *inode, handle_t *handle,* This implements the fallocate's collapse range functionality for ext4 * Returns: 0 and non-zero on error. */ -static int ext4_collapse_range (struct inode *inode,loff_t offset,loff_t len) +static int ext4_collapse_range (struct file *file,loff_t offset,loff_t len) { +struct inode *inode =file_inode (file);struct super_block *sb = inode->i_sb;struct address_space *mapping = inode->i_mapping;ext4_lblk_t punch_start, punch_stop;
@@ -5295,6 +5304,10 @@ static int ext4_collapse_range(struct inode *inode, loff_t offset, loff_t len)/* Wait for existing dio to complete */ inode_dio_wait (inode); + ret =file_modified (file); +if (ret) + goto out_mutex; +/* * Prevent page faults from reinstantiating pages we have released from * page cache.
@@ -5388,8 +5401,9 @@ static int ext4_collapse_range(struct inode *inode, loff_t offset, loff_t len)* by len bytes. * Returns 0 on success, error otherwise. */ -static int ext4_insert_range (struct inode *inode,loff_t offset,loff_t len) +static int ext4_insert_range (struct file *file,loff_t offset,loff_t len) { +struct inode *inode =file_inode (file);struct super_block *sb = inode->i_sb;struct address_space *mapping = inode->i_mapping;handle_t *handle;
@@ -5446,6 +5460,10 @@ static int ext4_insert_range(struct inode *inode, loff_t offset, loff_t len)/* Wait for existing dio to complete */ inode_dio_wait (inode); + ret =file_modified (file); +if (ret) + goto out_mutex; +/* * Prevent page faults from reinstantiating pages we have released from * page cache.
fs/ext4/inode.c +6 -1
@@ -3919,8 +3919,9 @@ int ext4_break_layouts(struct inode *inode)* Returns: 0 on success or negative on failure */ -int ext4_punch_hole (struct inode *inode,loff_t offset,loff_t length) +int ext4_punch_hole (struct file *file,loff_t offset,loff_t length) { +struct inode *inode =file_inode (file);struct super_block *sb = inode->i_sb;ext4_lblk_t first_block, stop_block;struct address_space *mapping = inode->i_mapping;
@@ -3982,6 +3983,10 @@ int ext4_punch_hole(struct inode *inode, loff_t offset, loff_t length)/* Wait all existing dio workers, newcomers will block on i_rwsem */ inode_dio_wait (inode); + ret =file_modified (file); +if (ret) + goto out_mutex; +/* * Prevent page faults from reinstantiating pages we have released from * page cache.
Quoted 13 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
Since the initial introduction of (posix) fallocate back at the turn of the century, it has been possible to use this syscall to change the user-visible contents of files. This can happen by extending the file size during a preallocation, or through any of the newer modes (punch, zero, collapse, insert range). Because the call can be used to change file contents, we should treat it like we do any other modification to a file -- update the mtime, and drop set[ug]id privileges/capabilities.
The VFS function
file_modified()does all this for us if pass it a locked inode, so let's make fallocate drop permissions correctly.
Quoted 6 lines
Signed-off-by Darrick J. Wong <[email protected]>--- fs/ext4/ext4.h | 2 +- fs/ext4/extents.c | 32 +++++++++++++++++++++++++------- fs/ext4/inode.c | 7 ++++++- 3 files changed, 32 insertions(+), 9 deletions(-)
Is there a test in xfstests that tests for this?
- Eric
Quoted 14 lines · Eric Biggers
Quoted 13 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
Since the initial introduction of (posix) fallocate back at the turn of the century, it has been possible to use this syscall to change the user-visible contents of files. This can happen by extending the file size during a preallocation, or through any of the newer modes (punch, zero, collapse, insert range). Because the call can be used to change file contents, we should treat it like we do any other modification to a file -- update the mtime, and drop set[ug]id privileges/capabilities.
The VFS function
file_modified()does all this for us if pass it a locked inode, so let's make fallocate drop permissions correctly.
Quoted 8 lines
Quoted 6 lines
Signed-off-by Darrick J. Wong <[email protected]>--- fs/ext4/ext4.h | 2 +- fs/ext4/extents.c | 32 +++++++++++++++++++++++++------- fs/ext4/inode.c | 7 ++++++- 3 files changed, 32 insertions(+), 9 deletions(-)Is there a test in xfstests that tests for this?
Not currently. I /do/ actually have one drafted (which I'll email out as a reply) but I was gonna tackle fixing btrfs before I actually submit it to Eryu for upstream fstests.
--D
Quoted 1 line
- Eric
RFC PATCH 1/1 fstests: ensure we drop suid after fallocate
In reply to Darrick J. WongFrom: Darrick J. Wong <[email protected]>
fallocate changes file contents, so make sure that we drop privileges and file capabilities after each fallocate operation.
--- tests/generic/834 | 129 +++++++++++++++++++++++++++++++++++++++++++++++++ tests/generic/834.out | 33 +++++++++++++ tests/generic/835 | 129 +++++++++++++++++++++++++++++++++++++++++++++++++ tests/generic/835.out | 33 +++++++++++++ tests/generic/836 | 129 +++++++++++++++++++++++++++++++++++++++++++++++++ tests/generic/836.out | 33 +++++++++++++ tests/generic/837 | 129 +++++++++++++++++++++++++++++++++++++++++++++++++ tests/generic/837.out | 33 +++++++++++++ tests/generic/838 | 129 +++++++++++++++++++++++++++++++++++++++++++++++++ tests/generic/838.out | 33 +++++++++++++ tests/generic/839 | 79 ++++++++++++++++++++++++++++++ tests/generic/839.out | 13 +++++ 12 files changed, 902 insertions(+) create mode 100755 tests/generic/834 create mode 100644 tests/generic/834.out create mode 100755 tests/generic/835 create mode 100644 tests/generic/835.out create mode 100755 tests/generic/836 create mode 100644 tests/generic/836.out create mode 100755 tests/generic/837 create mode 100644 tests/generic/837.out create mode 100755 tests/generic/838 create mode 100644 tests/generic/838.out create mode 100755 tests/generic/839 create mode 100755 tests/generic/839.out
tests/generic/834 new file +129 -0
@@ -0,0 +1,129 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2022 Oracle. All Rights Reserved. +# +# FS QA Test No. 834 +# +# Functional test for dropping suid and sgid bits as part of a fallocate. +# +. ./common/preamble +_begin_fstest auto clone quick + +# Override the default cleanup function. +_cleanup() +{ + cd / + rm -r -f $tmp.* $junk_dir +} + +# Import common functions. +. ./common/filter +. ./common/reflink + +# real QA test starts here + +# Modify as appropriate. +_supported_fs generic +_require_user +_require_test +verb=falloc +_require_xfs_io_command $verb + +_require_congruent_file_oplen $TEST_DIR 65536 + +junk_dir=$TEST_DIR/$seq +junk_file=$junk_dir/a +mkdir -p $junk_dir/ +chmod a+rw $junk_dir/ + +setup_testfile() { + rm -f $junk_file + _pwrite_byte 0x58 0 192k $junk_file >> $seqres.full + sync +} + +commit_and_check() { + local user="$1" + local command="$2" + local start="$3" + local end="$4" + + stat -c '%a %A %n' $junk_file | _filter_test_dir + + local cmd="$XFS_IO_PROG -c '$command $start $end' $junk_file" + if [ -n "$user" ]; then + su - "$user" -c "$cmd" >> $seqres.full + else + $SHELL -c "$cmd" >> $seqres.full + fi + + stat -c '%a %A %n' $junk_file | _filter_test_dir + + # Blank line in output + echo +} + +nr=0 +# Commit to a non-exec file by an unprivileged user clears suid but +# leaves sgid. +nr=$((nr + 1)) +echo "Test $nr - qa_user, non-exec file $verb" +setup_testfile +chmod a+rws $junk_file +commit_and_check "$qa_user" "$verb" 64k 64k + +# Commit to a group-exec file by an unprivileged user clears suid and +# sgid. +nr=$((nr + 1)) +echo "Test $nr - qa_user, group-exec file $verb" +setup_testfile +chmod g+x,a+rws $junk_file +commit_and_check "$qa_user" "$verb" 64k 64k + +# Commit to a user-exec file by an unprivileged user clears suid but +# not sgid. +nr=$((nr + 1)) +echo "Test $nr - qa_user, user-exec file $verb" +setup_testfile +chmod u+x,a+rws,g-x $junk_file +commit_and_check "$qa_user" "$verb" 64k 64k + +# Commit to a all-exec file by an unprivileged user clears suid and +# sgid. +nr=$((nr + 1)) +echo "Test $nr - qa_user, all-exec file $verb" +setup_testfile +chmod a+rwxs $junk_file +commit_and_check "$qa_user" "$verb" 64k 64k + +# Commit to a non-exec file by root clears suid but leaves sgid. +nr=$((nr + 1)) +echo "Test $nr - root, non-exec file $verb" +setup_testfile +chmod a+rws $junk_file +commit_and_check "" "$verb" 64k 64k + +# Commit to a group-exec file by root clears suid and sgid. +nr=$((nr + 1)) +echo "Test $nr - root, group-exec file $verb" +setup_testfile +chmod g+x,a+rws $junk_file +commit_and_check "" "$verb" 64k 64k + +# Commit to a user-exec file by root clears suid but not sgid. +nr=$((nr + 1)) +echo "Test $nr - root, user-exec file $verb" +setup_testfile +chmod u+x,a+rws,g-x $junk_file +commit_and_check "" "$verb" 64k 64k + +# Commit to a all-exec file by root clears suid and sgid. +nr=$((nr + 1)) +echo "Test $nr - root, all-exec file $verb" +setup_testfile +chmod a+rwxs $junk_file +commit_and_check "" "$verb" 64k 64k + +# success, all done +status=0 +exit
tests/generic/834.out new file +33 -0
@@ -0,0 +1,33 @@ +QA output created by 834 +Test 1 - qa_user, non-exec file falloc +6666 -rwSrwSrw- TEST_DIR/834/a +666 -rw-rw-rw- TEST_DIR/834/a + +Test 2 - qa_user, group-exec file falloc +6676 -rwSrwsrw- TEST_DIR/834/a +676 -rw-rwxrw- TEST_DIR/834/a + +Test 3 - qa_user, user-exec file falloc +6766 -rwsrwSrw- TEST_DIR/834/a +766 -rwxrw-rw- TEST_DIR/834/a + +Test 4 - qa_user, all-exec file falloc +6777 -rwsrwsrwx TEST_DIR/834/a +777 -rwxrwxrwx TEST_DIR/834/a + +Test 5 - root, non-exec file falloc +6666 -rwSrwSrw- TEST_DIR/834/a +6666 -rwSrwSrw- TEST_DIR/834/a + +Test 6 - root, group-exec file falloc +6676 -rwSrwsrw- TEST_DIR/834/a +6676 -rwSrwsrw- TEST_DIR/834/a + +Test 7 - root, user-exec file falloc +6766 -rwsrwSrw- TEST_DIR/834/a +6766 -rwsrwSrw- TEST_DIR/834/a + +Test 8 - root, all-exec file falloc +6777 -rwsrwsrwx TEST_DIR/834/a +6777 -rwsrwsrwx TEST_DIR/834/a +
tests/generic/835 new file +129 -0
129 lines not on this page · raw message
tests/generic/835.out new file +33 -0
@@ -0,0 +1,33 @@ +QA output created by 835 +Test 1 - qa_user, non-exec file fpunch +6666 -rwSrwSrw- TEST_DIR/835/a +666 -rw-rw-rw- TEST_DIR/835/a + +Test 2 - qa_user, group-exec file fpunch +6676 -rwSrwsrw- TEST_DIR/835/a +676 -rw-rwxrw- TEST_DIR/835/a + +Test 3 - qa_user, user-exec file fpunch +6766 -rwsrwSrw- TEST_DIR/835/a +766 -rwxrw-rw- TEST_DIR/835/a + +Test 4 - qa_user, all-exec file fpunch +6777 -rwsrwsrwx TEST_DIR/835/a +777 -rwxrwxrwx TEST_DIR/835/a + +Test 5 - root, non-exec file fpunch +6666 -rwSrwSrw- TEST_DIR/835/a +6666 -rwSrwSrw- TEST_DIR/835/a + +Test 6 - root, group-exec file fpunch +6676 -rwSrwsrw- TEST_DIR/835/a +6676 -rwSrwsrw- TEST_DIR/835/a + +Test 7 - root, user-exec file fpunch +6766 -rwsrwSrw- TEST_DIR/835/a +6766 -rwsrwSrw- TEST_DIR/835/a + +Test 8 - root, all-exec file fpunch +6777 -rwsrwsrwx TEST_DIR/835/a +6777 -rwsrwsrwx TEST_DIR/835/a +
tests/generic/836 new file +129 -0
129 lines not on this page · raw message
tests/generic/836.out new file +33 -0
@@ -0,0 +1,33 @@ +QA output created by 836 +Test 1 - qa_user, non-exec file fzero +6666 -rwSrwSrw- TEST_DIR/836/a +666 -rw-rw-rw- TEST_DIR/836/a + +Test 2 - qa_user, group-exec file fzero +6676 -rwSrwsrw- TEST_DIR/836/a +676 -rw-rwxrw- TEST_DIR/836/a + +Test 3 - qa_user, user-exec file fzero +6766 -rwsrwSrw- TEST_DIR/836/a +766 -rwxrw-rw- TEST_DIR/836/a + +Test 4 - qa_user, all-exec file fzero +6777 -rwsrwsrwx TEST_DIR/836/a +777 -rwxrwxrwx TEST_DIR/836/a + +Test 5 - root, non-exec file fzero +6666 -rwSrwSrw- TEST_DIR/836/a +6666 -rwSrwSrw- TEST_DIR/836/a + +Test 6 - root, group-exec file fzero +6676 -rwSrwsrw- TEST_DIR/836/a +6676 -rwSrwsrw- TEST_DIR/836/a + +Test 7 - root, user-exec file fzero +6766 -rwsrwSrw- TEST_DIR/836/a +6766 -rwsrwSrw- TEST_DIR/836/a + +Test 8 - root, all-exec file fzero +6777 -rwsrwsrwx TEST_DIR/836/a +6777 -rwsrwsrwx TEST_DIR/836/a +
tests/generic/837 new file +129 -0
129 lines not on this page · raw message
tests/generic/837.out new file +33 -0
@@ -0,0 +1,33 @@ +QA output created by 837 +Test 1 - qa_user, non-exec file finsert +6666 -rwSrwSrw- TEST_DIR/837/a +666 -rw-rw-rw- TEST_DIR/837/a + +Test 2 - qa_user, group-exec file finsert +6676 -rwSrwsrw- TEST_DIR/837/a +676 -rw-rwxrw- TEST_DIR/837/a + +Test 3 - qa_user, user-exec file finsert +6766 -rwsrwSrw- TEST_DIR/837/a +766 -rwxrw-rw- TEST_DIR/837/a + +Test 4 - qa_user, all-exec file finsert +6777 -rwsrwsrwx TEST_DIR/837/a +777 -rwxrwxrwx TEST_DIR/837/a + +Test 5 - root, non-exec file finsert +6666 -rwSrwSrw- TEST_DIR/837/a +6666 -rwSrwSrw- TEST_DIR/837/a + +Test 6 - root, group-exec file finsert +6676 -rwSrwsrw- TEST_DIR/837/a +6676 -rwSrwsrw- TEST_DIR/837/a + +Test 7 - root, user-exec file finsert +6766 -rwsrwSrw- TEST_DIR/837/a +6766 -rwsrwSrw- TEST_DIR/837/a + +Test 8 - root, all-exec file finsert +6777 -rwsrwsrwx TEST_DIR/837/a +6777 -rwsrwsrwx TEST_DIR/837/a +
tests/generic/838 new file +129 -0
129 lines not on this page · raw message
tests/generic/838.out new file +33 -0
@@ -0,0 +1,33 @@ +QA output created by 838 +Test 1 - qa_user, non-exec file fcollapse +6666 -rwSrwSrw- TEST_DIR/838/a +666 -rw-rw-rw- TEST_DIR/838/a + +Test 2 - qa_user, group-exec file fcollapse +6676 -rwSrwsrw- TEST_DIR/838/a +676 -rw-rwxrw- TEST_DIR/838/a + +Test 3 - qa_user, user-exec file fcollapse +6766 -rwsrwSrw- TEST_DIR/838/a +766 -rwxrw-rw- TEST_DIR/838/a + +Test 4 - qa_user, all-exec file fcollapse +6777 -rwsrwsrwx TEST_DIR/838/a +777 -rwxrwxrwx TEST_DIR/838/a + +Test 5 - root, non-exec file fcollapse +6666 -rwSrwSrw- TEST_DIR/838/a +6666 -rwSrwSrw- TEST_DIR/838/a + +Test 6 - root, group-exec file fcollapse +6676 -rwSrwsrw- TEST_DIR/838/a +6676 -rwSrwsrw- TEST_DIR/838/a + +Test 7 - root, user-exec file fcollapse +6766 -rwsrwSrw- TEST_DIR/838/a +6766 -rwsrwSrw- TEST_DIR/838/a + +Test 8 - root, all-exec file fcollapse +6777 -rwsrwsrwx TEST_DIR/838/a +6777 -rwsrwsrwx TEST_DIR/838/a +
tests/generic/839 new file +79 -0
79 lines not on this page · raw message
tests/generic/839.out new file +13 -0
@@ -0,0 +1,13 @@ +QA output created by 839 +Test 1 - qa_user +777 -rwxrwxrwx TEST_DIR/839/a +TEST_DIR/839/a cap_setgid,cap_setuid=ep +777 -rwxrwxrwx TEST_DIR/839/a +TEST_DIR/839/a + +Test 2 - root +777 -rwxrwxrwx TEST_DIR/839/a +TEST_DIR/839/a cap_setgid,cap_setuid=ep +777 -rwxrwxrwx TEST_DIR/839/a +TEST_DIR/839/a +
Quoted 10 lines · Darrick J. Wong
From: Darrick J. Wong <[email protected]>
Since the initial introduction of (posix) fallocate back at the turn of the century, it has been possible to use this syscall to change the user-visible contents of files. This can happen by extending the file size during a preallocation, or through any of the newer modes (punch, zero, collapse, insert range). Because the call can be used to change file contents, we should treat it like we do any other modification to a file -- update the mtime, and drop set[ug]id privileges/capabilities.
Quoted 4 lines
The VFS function
file_modified()does all this for us if pass it a locked inode, so let's make fallocate drop permissions correctly.Signed-off-by Darrick J. Wong <[email protected]>
Applied, thanks!
(BTW, when you reply to a patch with a different patch, such as in this case, "fstests: ensure we drop suid after fallocate", it hopelessly confuses b4, which is why I'm sending this notification manually. Try running "b4 am 20220308185043.GA117678@magnolia" and compare that to https://lore.kernel.org/r/20220308185043.GA117678@magnolia
and you'll see what I mean.)
- Ted