| Message ID | 156022837711.3227213.11787906519006016743.stgit@magnolia |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | vfs: make immutable files actually immutable | expand |
On Mon, Jun 10, 2019 at 09:46:17PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > The chattr manpage has this to say about immutable files: > > "A file with the 'i' attribute cannot be modified: it cannot be deleted > or renamed, no link can be created to this file, most of the file's > metadata can not be modified, and the file can not be opened in write > mode." > > Once the flag is set, it is enforced for quite a few file operations, > such as fallocate, fpunch, fzero, rm, touch, open, etc. However, we > don't check for immutability when doing a write(), a PROT_WRITE mmap(), > a truncate(), or a write to a previously established mmap. > > If a program has an open write fd to a file that the administrator > subsequently marks immutable, the program still can change the file > contents. Weird! > > The ability to write to an immutable file does not follow the manpage > promise that immutable files cannot be modified. Worse yet it's > inconsistent with the behavior of other syscalls which don't allow > modifications of immutable files. > > Therefore, add the necessary checks to make the write, mmap, and > truncate behavior consistent with what the manpage says and consistent > with other syscalls on filesystems which support IMMUTABLE. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> I note that this patch doesn't allow writes to swap files. So Amir's generic/554 test will still fail for those file systems that don't use copy_file_range. I'm indifferent as to whether you add a new patch, or include that change in this patch, but perhaps we should fix this while we're making changes in these code paths? - Ted
On Thu, Jun 20, 2019 at 05:52:12PM -0400, Theodore Ts'o wrote: > On Mon, Jun 10, 2019 at 09:46:17PM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > The chattr manpage has this to say about immutable files: > > > > "A file with the 'i' attribute cannot be modified: it cannot be deleted > > or renamed, no link can be created to this file, most of the file's > > metadata can not be modified, and the file can not be opened in write > > mode." > > > > Once the flag is set, it is enforced for quite a few file operations, > > such as fallocate, fpunch, fzero, rm, touch, open, etc. However, we > > don't check for immutability when doing a write(), a PROT_WRITE mmap(), > > a truncate(), or a write to a previously established mmap. > > > > If a program has an open write fd to a file that the administrator > > subsequently marks immutable, the program still can change the file > > contents. Weird! > > > > The ability to write to an immutable file does not follow the manpage > > promise that immutable files cannot be modified. Worse yet it's > > inconsistent with the behavior of other syscalls which don't allow > > modifications of immutable files. > > > > Therefore, add the necessary checks to make the write, mmap, and > > truncate behavior consistent with what the manpage says and consistent > > with other syscalls on filesystems which support IMMUTABLE. > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > I note that this patch doesn't allow writes to swap files. So Amir's > generic/554 test will still fail for those file systems that don't use > copy_file_range. I didn't add any IS_SWAPFILE checks here, so I'm not sure to what you're referring? > I'm indifferent as to whether you add a new patch, or include that > change in this patch, but perhaps we should fix this while we're > making changes in these code paths? The swapfile patches should be in a separate patch, which I was planning to work on but hadn't really gotten around to it. --D > - Ted
On Thu, Jun 20, 2019 at 03:13:06PM -0700, Darrick J. Wong wrote: > > I note that this patch doesn't allow writes to swap files. So Amir's > > generic/554 test will still fail for those file systems that don't use > > copy_file_range. > > I didn't add any IS_SWAPFILE checks here, so I'm not sure to what you're > referring? Sorry, my bad; I mistyped. What I should have said is this patch doesn't *prohibit* writes to swap files.... (And so Amir's generic/554 test, even modified so it allow reads from swapfiles, but not writes, when using copy_file_range, is still failing for ext4. I was looking to see if I could remove it from my exclude list, but not yet. :-) > > I'm indifferent as to whether you add a new patch, or include that > > change in this patch, but perhaps we should fix this while we're > > making changes in these code paths? > > The swapfile patches should be in a separate patch, which I was planning > to work on but hadn't really gotten around to it. Ok, great, thanks!! - Ted
diff --git a/fs/attr.c b/fs/attr.c index d22e8187477f..1fcfdcc5b367 100644 --- a/fs/attr.c +++ b/fs/attr.c @@ -233,19 +233,18 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de WARN_ON_ONCE(!inode_is_locked(inode)); - if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID | ATTR_TIMES_SET)) { - if (IS_IMMUTABLE(inode) || IS_APPEND(inode)) - return -EPERM; - } + if (IS_IMMUTABLE(inode)) + return -EPERM; + + if ((ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID | ATTR_TIMES_SET)) && + IS_APPEND(inode)) + return -EPERM; /* * If utimes(2) and friends are called with times == NULL (or both * times are UTIME_NOW), then we need to check for write permission */ if (ia_valid & ATTR_TOUCH) { - if (IS_IMMUTABLE(inode)) - return -EPERM; - if (!inode_owner_or_capable(inode)) { error = inode_permission(inode, MAY_WRITE); if (error) diff --git a/mm/filemap.c b/mm/filemap.c index df2006ba0cfa..65433c7ab1d8 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -2940,6 +2940,9 @@ inline ssize_t generic_write_checks(struct kiocb *iocb, struct iov_iter *from) loff_t count; int ret; + if (IS_IMMUTABLE(inode)) + return -EPERM; + if (!iov_iter_count(from)) return 0; diff --git a/mm/memory.c b/mm/memory.c index ddf20bd0c317..4311cfdade90 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -2235,6 +2235,9 @@ static vm_fault_t do_page_mkwrite(struct vm_fault *vmf) vmf->flags = FAULT_FLAG_WRITE|FAULT_FLAG_MKWRITE; + if (vmf->vma->vm_file && IS_IMMUTABLE(file_inode(vmf->vma->vm_file))) + return VM_FAULT_SIGBUS; + ret = vmf->vma->vm_ops->page_mkwrite(vmf); /* Restore original flags so that caller is not surprised */ vmf->flags = old_flags; diff --git a/mm/mmap.c b/mm/mmap.c index 7e8c3e8ae75f..ac1e32205237 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -1483,8 +1483,12 @@ unsigned long do_mmap(struct file *file, unsigned long addr, case MAP_SHARED_VALIDATE: if (flags & ~flags_mask) return -EOPNOTSUPP; - if ((prot&PROT_WRITE) && !(file->f_mode&FMODE_WRITE)) - return -EACCES; + if (prot & PROT_WRITE) { + if (!(file->f_mode & FMODE_WRITE)) + return -EACCES; + if (IS_IMMUTABLE(file_inode(file))) + return -EPERM; + } /* * Make sure we don't allow writing to an append-only