If O_EXCL is *not* specified, then linkat() can be used to link the temporary file into the filesystem. If O_EXCL is specified then linkat() should fail (-1).
After commit 863f144f12ad ("vfs: open inside ->tmpfile()") the O_EXCL flag is no longer honored by the vfs layer for tmpfile, which means the file can be linked even if O_EXCL flag is specified, which is a change in behaviour for userspace!
The open flags was previously passed as a parameter, so it was uneffected by the changes to file->f_flags caused by finish_open(). This patch fixes the issue by storing file->f_flags in a local variable so the O_EXCL test logic is restored.
This regression was detected by Android CTS Bionic fcntl() tests running on android-mainline [1].
[1] https://android.googlesource.com/platform/bionic/+/ refs/heads/master/tests/fcntl_test.cpp#352
Fixes: 863f144f12ad ("vfs: open inside ->tmpfile()") To: lkml linux-kernel@vger.kernel.org Cc: Alexander Viro viro@zeniv.linux.org.uk Cc: Miklos Szeredi mszeredi@redhat.com Cc: stable@vger.kernel.org Cc: linux-fsdevel@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: Will McVicker willmcvicker@google.com Cc: Peter Griffin gpeter@google.com Signed-off-by: Peter Griffin peter.griffin@linaro.org --- fs/namei.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/namei.c b/fs/namei.c index 578c2110df02..9155ecb547ce 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -3591,6 +3591,7 @@ static int vfs_tmpfile(struct user_namespace *mnt_userns, struct inode *dir = d_inode(parentpath->dentry); struct inode *inode; int error; + int open_flag = file->f_flags;
/* we want directory to be writable */ error = inode_permission(mnt_userns, dir, MAY_WRITE | MAY_EXEC); @@ -3613,7 +3614,7 @@ static int vfs_tmpfile(struct user_namespace *mnt_userns, if (error) return error; inode = file_inode(file); - if (!(file->f_flags & O_EXCL)) { + if (!(open_flag & O_EXCL)) { spin_lock(&inode->i_lock); inode->i_state |= I_LINKABLE; spin_unlock(&inode->i_lock);
On Thu, 3 Nov 2022 at 18:04, Peter Griffin peter.griffin@linaro.org wrote:
If O_EXCL is *not* specified, then linkat() can be used to link the temporary file into the filesystem. If O_EXCL is specified then linkat() should fail (-1).
After commit 863f144f12ad ("vfs: open inside ->tmpfile()") the O_EXCL flag is no longer honored by the vfs layer for tmpfile, which means the file can be linked even if O_EXCL flag is specified, which is a change in behaviour for userspace!
The open flags was previously passed as a parameter, so it was uneffected by the changes to file->f_flags caused by finish_open(). This patch fixes the issue by storing file->f_flags in a local variable so the O_EXCL test logic is restored.
This regression was detected by Android CTS Bionic fcntl() tests running on android-mainline [1].
[1] https://android.googlesource.com/platform/bionic/+/ refs/heads/master/tests/fcntl_test.cpp#352
Looks good.
Acked-by: Miklos Szeredi mszeredi@redhat.com
Thanks, Miklos
On 11/03/2022, Miklos Szeredi wrote:
On Thu, 3 Nov 2022 at 18:04, Peter Griffin peter.griffin@linaro.org wrote:
If O_EXCL is *not* specified, then linkat() can be used to link the temporary file into the filesystem. If O_EXCL is specified then linkat() should fail (-1).
After commit 863f144f12ad ("vfs: open inside ->tmpfile()") the O_EXCL flag is no longer honored by the vfs layer for tmpfile, which means the file can be linked even if O_EXCL flag is specified, which is a change in behaviour for userspace!
The open flags was previously passed as a parameter, so it was uneffected by the changes to file->f_flags caused by finish_open(). This patch fixes the issue by storing file->f_flags in a local variable so the O_EXCL test logic is restored.
This regression was detected by Android CTS Bionic fcntl() tests running on android-mainline [1].
[1] https://android.googlesource.com/platform/bionic/+/ refs/heads/master/tests/fcntl_test.cpp#352
Looks good.
Acked-by: Miklos Szeredi mszeredi@redhat.com
Thanks, Miklos
Thanks Peter for tracking this down! I tested this on the android-mainline version of 6.1-rc3 on a Pixel 6 device.
Tested-by: Will McVicker willmcvicker@google.com
Regards, Will
Hi Alexander,
On Thu, 3 Nov 2022 at 19:12, Miklos Szeredi miklos@szeredi.hu wrote:
On Thu, 3 Nov 2022 at 18:04, Peter Griffin peter.griffin@linaro.org wrote:
If O_EXCL is *not* specified, then linkat() can be used to link the temporary file into the filesystem. If O_EXCL is specified then linkat() should fail (-1).
After commit 863f144f12ad ("vfs: open inside ->tmpfile()") the O_EXCL flag is no longer honored by the vfs layer for tmpfile, which means the file can be linked even if O_EXCL flag is specified, which is a change in behaviour for userspace!
The open flags was previously passed as a parameter, so it was uneffected by the changes to file->f_flags caused by finish_open(). This patch fixes the issue by storing file->f_flags in a local variable so the O_EXCL test logic is restored.
This regression was detected by Android CTS Bionic fcntl() tests running on android-mainline [1].
[1] https://android.googlesource.com/platform/bionic/+/ refs/heads/master/tests/fcntl_test.cpp#352
Looks good.
Acked-by: Miklos Szeredi mszeredi@redhat.com
As this patch now has an Acked-by the original author of the commit that reworked the tmpfile vfs logic and introduced the regression. Can you pick up this commit and send it onto Linus for inclusion into the next v6.1-rc release?
Note, it fixes a regression for userspace introduced in this merge window so I was hoping to get the fix into the next -rc so that the v6.1 release does not contain this bug.
Many thanks,
Peter
On Mon, Nov 14, 2022 at 02:38:15PM +0000, Peter Griffin wrote:
commit that reworked the tmpfile vfs logic and introduced the regression. Can you pick up this commit and send it onto Linus for inclusion into the next v6.1-rc release?
Note, it fixes a regression for userspace introduced in this merge window so I was hoping to get the fix into the next -rc so that the v6.1 release does not contain this bug.
Applied to #fixes and pushed out; will send a pull request to Linus tomorrow...
linux-stable-mirror@lists.linaro.org