Re: [PATCH] ecryptfs: Fix inodes not being evicted until unmount
On Sat, Jan 30, 2021 at 11:06:40AM -0600, Tyler Hicks wrote: > Hey Jeffrey and Dan - thanks for the patch! Unfortunately, I think this > would allow the eCryptfs inode's nlink count to get out of sync with the > lower inode's nlink count in the case of direct manipulation to the > lower filesystem. Hmm. What if I instead synchronize it before calling vfs_unlink(), then call drop_nlink() if vfs_unlink() succeeds? > Is the condition that you're trying to fix a result of going through the > this code path? > > ecryptfs_unlink() -> ecryptfs_do_unlink() -> vfs_unlink() -> nfs_unlink() -> > nfs_sillyrename() -> nfs_async_unlink() Yes, that is the code path that causes it. V/R, Jeffrey Mitchell
[PATCH] ecryptfs: fix kernel panic with null dev_name
When mounting eCryptfs, a null "dev_name" argument to ecryptfs_mount()
causes a kernel panic if the parsed options are valid. The easiest way to
reproduce this is to call mount() from userspace with an existing
eCryptfs mount's options and a "source" argument of 0.
Error out if "dev_name" is null in ecryptfs_mount()
Signed-off-by: Jeffrey Mitchell
---
fs/ecryptfs/main.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
index e63259fdef28..b2f6a1937d23 100644
--- a/fs/ecryptfs/main.c
+++ b/fs/ecryptfs/main.c
@@ -492,6 +492,12 @@ static struct dentry *ecryptfs_mount(struct
file_system_type *fs_type, int flags
goto out;
}
+ if (!dev_name) {
+ rc = -EINVAL;
+ err = "Device name cannot be null";
+ goto out;
+ }
+
rc = ecryptfs_parse_options(sbi, raw_data, &check_ruid);
if (rc) {
err = "Error parsing options";
--
2.25.1
[PATCH] ecryptfs: add rename flag support
Currently, ecryptfs_rename() returns EINVAL if any flags are passed in.
However, if the lower filesystem has support for those flags, it doesn't
require any additional complexity to take advantage of it in eCryptfs.
Add flag support to ecryptfs_rename() by passing them through to the lower
filesystem
Signed-off-by: Jeffrey Mitchell
---
fs/ecryptfs/inode.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index 18e9285fbb4c..647afc2fd754 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -601,9 +601,6 @@ ecryptfs_rename(struct user_namespace *mnt_userns, struct
inode *old_dir,
struct inode *target_inode;
struct renamedata rd = {};
- if (flags)
- return -EINVAL;
-
lower_old_dir_dentry = ecryptfs_dentry_to_lower(old_dentry->d_parent);
lower_new_dir_dentry = ecryptfs_dentry_to_lower(new_dentry->d_parent);
@@ -636,6 +633,7 @@ ecryptfs_rename(struct user_namespace *mnt_userns, struct
inode *old_dir,
rd.new_mnt_userns = &init_user_ns;
rd.new_dir = d_inode(lower_new_dir_dentry);
rd.new_dentry = lower_new_dentry;
+ rd.flags= flags;
rc = vfs_rename(&rd);
if (rc)
goto out_lock;
--
2.25.1
[PATCH] nfs: Fix getxattr kernel panic and memory overflow
Move the buffer size check to decode_attr_security_label() before memcpy()
Only call memcpy() if the buffer is large enough
Signed-off-by: Jeffrey Mitchell
---
fs/nfs/nfs4proc.c | 2 --
fs/nfs/nfs4xdr.c | 5 -
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 2e2dac29a9e9..45e0585e0667 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -5845,8 +5845,6 @@ static int _nfs4_get_security_label(struct inode *inode,
void *buf,
return ret;
if (!(fattr.valid & NFS_ATTR_FATTR_V4_SECURITY_LABEL))
return -ENOENT;
- if (buflen < label.len)
- return -ERANGE;
return 0;
}
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 47817ef0aadb..50162e0a959c 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -4166,7 +4166,10 @@ static int decode_attr_security_label(struct xdr_stream
*xdr, uint32_t *bitmap,
return -EIO;
if (len < NFS4_MAXLABELLEN) {
if (label) {
- memcpy(label->label, p, len);
+ if (label->len && label->len < len)
+ return -ERANGE;
+ if (label->len)
+ memcpy(label->label, p, len);
label->len = len;
label->pi = pi;
label->lfs = lfs;
--
2.25.1
[PATCH] nfs: Fix getxattr kernel panic and memory overflow
_nfs4_get_security_label() and decode_attr_security_label() run severe risks with memory management and do not fully implement their functionality. In the case that the buffer and length are both NULL, which according to the getxattr man page should simply return the length of the attribute, decode_attr_security_label() will kernel panic trying to write to the null pointer. If the buffer length is nonzero but below the size of the attribute, decode_attr_security_label() will write the data anyway, overflowing the buffer, and it isn't until later in _nfs4_get_security_label() that -ERANGE gets returned. Here is some of the kernel panic output that I reproduced: BUG: kernel NULL pointer dereference, address: [...] RIP: 0010:__memcpy+0x12/0x20 [...] Call Trace: decode_getfattr_attrs+0xb1f/0xdc0 ? set_next_entity+0x8e/0x180 decode_getfattr_generic.constprop.0+0x10f/0x210 ? rpc_decode_header+0x570/0x570 nfs4_xdr_dec_getattr+0x94/0xa0 [...] _nfs4_get_security_label+0x134/0x180 ? _cond_resched+0x10/0x20 ? __kmalloc+0x1f6/0x200 nfs4_xattr_get_nfs4_label+0x89/0x120 __vfs_getxattr+0x4e/0x70 ecryptfs_getxattr_lower+0x4a/0x70 ecryptfs_xattr_get+0x23/0x24 __vfs_getxattr+0x4e/0x70 sb_finish_set_opts+0x12c/0x240 selinux_set_mnt_opts+0x288/0x6a0 security_sb_set_mnt_opts+0x40/0x60 vfs_get_tree+0x57/0xb0 do_mount+0x742/0x9b0 __x64_sys_mount+0x89/0xc0 do_syscall_64+0x3e/0x70 - Jeffrey Jeffrey Mitchell (1): nfs: Fix getxattr kernel panic and memory overflow fs/nfs/nfs4proc.c | 2 -- fs/nfs/nfs4xdr.c | 5 - 2 files changed, 4 insertions(+), 3 deletions(-) -- 2.25.1
[PATCH] ecryptfs: Fix inodes not being evicted until unmount
On asynchronous base filesystems like NFS, eCryptFS leaves inodes for deleted files in the cache until unmounting. Change call in ecryptfs_do_unlink() from set_nlink() to drop_nlink() in order to reliably evict inodes from the cache even on top of NFS. Signed-off-by: Dan Robertson Signed-off-by: Jeffrey Mitchell --- fs/ecryptfs/inode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c index e23752d..f7594b6 100644 --- a/fs/ecryptfs/inode.c +++ b/fs/ecryptfs/inode.c @@ -147,7 +147,7 @@ static int ecryptfs_do_unlink(struct inode *dir, struct dentry *dentry, goto out_unlock; } fsstack_copy_attr_times(dir, lower_dir_inode); - set_nlink(inode, ecryptfs_inode_to_lower(inode)->i_nlink); + drop_nlink(inode); inode->i_ctime = dir->i_ctime; out_unlock: dput(lower_dentry); -- 2.7.4
Re: [PATCH] ecryptfs: Fix inodes not being evicted until unmount
Steps to reproduce issue: Mount nfs Inside the nfs mount, create back and front directories for ecryptfs Use ecryptfs to mount one directory onto the other Create a few files inside the ecryptfs mount Set ftrace to monitor ecryptfs_do_unlink() and ecryptfs_evict_inode() Delete files Unmount ecryptfs ftrace output before patch: # tracer: function_graph # # CPU DURATION FUNCTION CALLS # | | | | | | | 1) | /* Tracing enabled */ 1) | /* Writing to files */ 1) | /* Unlinking first file */ 0) * 27804.34 us | ecryptfs_do_unlink(); 1) | /* rm'ing second file */ -- 0) unlink-1384 =>rm-1385 -- 0) * 32828.63 us | ecryptfs_do_unlink(); 1) | /* Unmounting eCryptFS */ -- 1) trace-a-1365 => umount-1387 -- 1) 2.408 us| ecryptfs_evict_inode(); 1) 8.914 us| ecryptfs_evict_inode(); 1) 3.344 us| ecryptfs_evict_inode(); ftrace output after patch: # tracer: function_graph # # CPU DURATION FUNCTION CALLS # | | | | | | | 0) | /* Tracing enabled */ 0) | /* Writing to files */ 0) | /* Unlinking first file */ 1) * 24728.81 us | ecryptfs_do_unlink(); 1) + 20.923 us | ecryptfs_evict_inode(); 0) | /* rm'ing second file */ -- 1) unlink-1333 =>rm-1334 -- 1) * 41093.71 us | ecryptfs_do_unlink(); 1) + 11.384 us | ecryptfs_evict_inode(); 0) | /* Unmounting eCryptFS */ -- 0) trace-a-1314 => umount-1336 -- 0) 2.986 us| ecryptfs_evict_inode(); On Fri, Dec 18, 2020 at 01:07:30PM -0600, Jeffrey Mitchell wrote: > On asynchronous base filesystems like NFS, eCryptFS leaves inodes for > deleted files in the cache until unmounting. Change call in > ecryptfs_do_unlink() from set_nlink() to drop_nlink() in order to reliably > evict inodes from the cache even on top of NFS. > > Signed-off-by: Dan Robertson > Signed-off-by: Jeffrey Mitchell > --- > fs/ecryptfs/inode.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c > index e23752d..f7594b6 100644 > --- a/fs/ecryptfs/inode.c > +++ b/fs/ecryptfs/inode.c > @@ -147,7 +147,7 @@ static int ecryptfs_do_unlink(struct inode *dir, struct > dentry *dentry, > goto out_unlock; > } > fsstack_copy_attr_times(dir, lower_dir_inode); > - set_nlink(inode, ecryptfs_inode_to_lower(inode)->i_nlink); > + drop_nlink(inode); > inode->i_ctime = dir->i_ctime; > out_unlock: > dput(lower_dentry); > -- > 2.7.4 >
[PATCH] xfs: set inode size after creating symlink
When XFS creates a new symlink, it writes its size to disk but not to the VFS inode. This causes i_size_read() to return 0 for that symlink until it is re-read from disk, for example when the system is rebooted. I found this inconsistency while protecting directories with eCryptFS. The command "stat path/to/symlink/in/ecryptfs" will report "Size: 0" if the symlink was created after the last reboot on an XFS root. Call i_size_write() in xfs_symlink() Signed-off-by: Jeffrey Mitchell --- fs/xfs/xfs_symlink.c | 1 + 1 file changed, 1 insertion(+) diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c index 1f43fd7f3209..c835827ae389 100644 --- a/fs/xfs/xfs_symlink.c +++ b/fs/xfs/xfs_symlink.c @@ -300,6 +300,7 @@ xfs_symlink( } ASSERT(pathlen == 0); } + i_size_write(VFS_I(ip), ip->i_d.di_size); /* * Create the directory entry for the symlink. -- 2.25.1
Re: [PATCH] xfs: set inode size after creating symlink
On Thu, Jan 21, 2021 at 10:41:37AM -0800, Darrick J. Wong wrote: > Do directories have the same problem? Yes, I just checked in a VM. While ecryptfs does call vfs_getattr(), it only uses the "blocks" value to supplement the data from an identical generic_fillattr() call to what ecryptfs_getattr_link() uses. The reported size still comes from i_size_read(). V/R, Jeffrey Mitchell
[PATCH] nfs: Fix security label length not being reset
In nfs_readdir_page_filler(), reset security label buffer length before
every reuse
Signed-off-by: Jeffrey Mitchell
---
fs/nfs/dir.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index a12f42e7d8c7..5ff6af4478a5 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -579,6 +579,9 @@ int nfs_readdir_page_filler(nfs_readdir_descriptor_t *desc,
struct nfs_entry *en
xdr_set_scratch_buffer(&stream, page_address(scratch), PAGE_SIZE);
do {
+ if (entry->label)
+ entry->label->len = NFS4_MAXLABELLEN;
+
status = xdr_decode(desc, entry, &stream);
if (status != 0) {
if (status == -EAGAIN)
--
2.25.1
[PATCH] nfs: Fix security label length not being reset
nfs_readdir_page_filler() iterates over entries in a directory, reusing the same security label buffer, but does not reset the buffer's length. This causes decode_attr_security_label() to return -ERANGE if an entry's security label is longer than the previous one's. This error, in nfs4_decode_dirent(), only gets passed up as -EAGAIN, which causes another failed attempt to copy into the buffer. The second error is ignored and the remaining entries do not show up in ls, specifically the getdents64() syscall. Reproduce by creating multiple files in NFS and giving one of the later files a longer security label. ls will not see that file nor any that are added afterwards, though they will exist on the backend. - Jeffrey Jeffrey Mitchell (1): nfs: Fix security label length not being reset fs/nfs/dir.c | 3 +++ 1 file changed, 3 insertions(+) -- 2.25.1
[PATCH v2] nfs: Fix security label length not being reset
nfs_readdir_page_filler() iterates over entries in a directory, reusing
the same security label buffer, but does not reset the buffer's length.
This causes decode_attr_security_label() to return -ERANGE if an entry's
security label is longer than the previous one's. This error, in
nfs4_decode_dirent(), only gets passed up as -EAGAIN, which causes another
failed attempt to copy into the buffer. The second error is ignored and
the remaining entries do not show up in ls, specifically the getdents64()
syscall.
Reproduce by creating multiple files in NFS and giving one of the later
files a longer security label. ls will not see that file nor any that are
added afterwards, though they will exist on the backend.
In nfs_readdir_page_filler(), reset security label buffer length before
every reuse
Signed-off-by: Jeffrey Mitchell
---
v2: Added explanation from cover letter as requested by J. Bruce Fields
fs/nfs/dir.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index e732580fe47b..cb52db9a0cfb 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -579,6 +579,9 @@ int nfs_readdir_page_filler(nfs_readdir_descriptor_t *desc,
struct nfs_entry *en
xdr_set_scratch_buffer(&stream, page_address(scratch), PAGE_SIZE);
do {
+ if (entry->label)
+ entry->label->len = NFS4_MAXLABELLEN;
+
status = xdr_decode(desc, entry, &stream);
if (status != 0) {
if (status == -EAGAIN)
--
2.25.1
[PATCH] ecryptfs: Fix inodes not being evicted until unmount
On asynchronous base filesystems like NFS, eCryptFS leaves inodes for deleted files in the cache until unmounting. Change call in ecryptfs_do_unlink() from set_nlink() to drop_nlink() in order to reliably evict inodes from the cache even on top of NFS. Signed-off-by: Dan Robertson Signed-off-by: Jeffrey Mitchell --- fs/ecryptfs/inode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c index e23752d..f7594b6 100644 --- a/fs/ecryptfs/inode.c +++ b/fs/ecryptfs/inode.c @@ -147,7 +147,7 @@ static int ecryptfs_do_unlink(struct inode *dir, struct dentry *dentry, goto out_unlock; } fsstack_copy_attr_times(dir, lower_dir_inode); - set_nlink(inode, ecryptfs_inode_to_lower(inode)->i_nlink); + drop_nlink(inode); inode->i_ctime = dir->i_ctime; out_unlock: dput(lower_dentry); -- 2.7.4
Re: [PATCH] ecryptfs: Fix inodes not being evicted until unmount
Steps to reproduce issue: Mount nfs Inside the nfs mount, create back and front directories for ecryptfs Use ecryptfs to mount one directory onto the other Create a few files inside the ecryptfs mount Set ftrace to monitor ecryptfs_do_unlink() and ecryptfs_evict_inode() Delete files Unmount ecryptfs ftrace output before patch: # tracer: function_graph # # CPU DURATION FUNCTION CALLS # | | | | | | | 1) | /* Tracing enabled */ 1) | /* Writing to files */ 1) | /* Unlinking first file */ 0) * 27804.34 us | ecryptfs_do_unlink(); 1) | /* rm'ing second file */ -- 0) unlink-1384 =>rm-1385 -- 0) * 32828.63 us | ecryptfs_do_unlink(); 1) | /* Unmounting eCryptFS */ -- 1) trace-a-1365 => umount-1387 -- 1) 2.408 us| ecryptfs_evict_inode(); 1) 8.914 us| ecryptfs_evict_inode(); 1) 3.344 us| ecryptfs_evict_inode(); ftrace output after patch: # tracer: function_graph # # CPU DURATION FUNCTION CALLS # | | | | | | | 0) | /* Tracing enabled */ 0) | /* Writing to files */ 0) | /* Unlinking first file */ 1) * 24728.81 us | ecryptfs_do_unlink(); 1) + 20.923 us | ecryptfs_evict_inode(); 0) | /* rm'ing second file */ -- 1) unlink-1333 =>rm-1334 -- 1) * 41093.71 us | ecryptfs_do_unlink(); 1) + 11.384 us | ecryptfs_evict_inode(); 0) | /* Unmounting eCryptFS */ -- 0) trace-a-1314 => umount-1336 -- 0) 2.986 us| ecryptfs_evict_inode(); On Fri, May 08, 2020 at 09:31:33AM -0500, Jeffrey Mitchell wrote: > On asynchronous base filesystems like NFS, eCryptFS leaves inodes for > deleted files in the cache until unmounting. Change call in > ecryptfs_do_unlink() from set_nlink() to drop_nlink() in order to reliably > evict inodes from the cache even on top of NFS. > > Signed-off-by: Dan Robertson > Signed-off-by: Jeffrey Mitchell > --- > fs/ecryptfs/inode.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c > index e23752d..f7594b6 100644 > --- a/fs/ecryptfs/inode.c > +++ b/fs/ecryptfs/inode.c > @@ -147,7 +147,7 @@ static int ecryptfs_do_unlink(struct inode *dir, struct > dentry *dentry, > goto out_unlock; > } > fsstack_copy_attr_times(dir, lower_dir_inode); > - set_nlink(inode, ecryptfs_inode_to_lower(inode)->i_nlink); > + drop_nlink(inode); > inode->i_ctime = dir->i_ctime; > out_unlock: > dput(lower_dentry); > -- > 2.7.4 >
