From 18319cb478de23340fdcb6385b0cc074a5416da7 Mon Sep 17 00:00:00 2001 From: Jan Harkes Date: Mon, 8 Nov 2021 18:34:30 -0800 Subject: coda: avoid NULL pointer dereference from a bad inode Patch series "Coda updates for -next". The following patch series contains some fixes for the Coda kernel module I've had sitting around and were tested extensively in a development version of the Coda kernel module that lives outside of the main kernel. This patch (of 9): Avoid accessing coda_inode_info from a dentry with a bad inode. Link: https://lkml.kernel.org/r/20210908140308.18491-1-jaharkes@cs.cmu.edu Link: https://lkml.kernel.org/r/20210908140308.18491-2-jaharkes@cs.cmu.edu Signed-off-by: Jan Harkes Cc: Alex Shi Cc: Jing Yangyang Cc: Xin Tan Cc: Xiyu Yang Cc: Zeal Robot Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- fs/coda/dir.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) (limited to 'fs/coda') diff --git a/fs/coda/dir.c b/fs/coda/dir.c index d69989c1bac3..3fd085009f26 100644 --- a/fs/coda/dir.c +++ b/fs/coda/dir.c @@ -499,15 +499,20 @@ out: */ static int coda_dentry_delete(const struct dentry * dentry) { - int flags; + struct inode *inode; + struct coda_inode_info *cii; if (d_really_is_negative(dentry)) return 0; - flags = (ITOC(d_inode(dentry))->c_flags) & C_PURGE; - if (is_bad_inode(d_inode(dentry)) || flags) { + inode = d_inode(dentry); + if (!inode || is_bad_inode(inode)) return 1; - } + + cii = ITOC(inode); + if (cii->c_flags & C_PURGE) + return 1; + return 0; } -- cgit v1.2.3-58-ga151 From 3d8e72d97411370aab662e85e2c3a7b26555179c Mon Sep 17 00:00:00 2001 From: Jan Harkes Date: Mon, 8 Nov 2021 18:34:33 -0800 Subject: coda: check for async upcall request using local state Originally flagged by Smatch because the code implicitly assumed outSize is not NULL for non-async upcalls because of a flag that was (not) set in req->uc_flags. However req->uc_flags field is in shared state and although the current code will not allow it to be changed before the async request check the code is more robust when it tests against the local outSize variable. Link: https://lkml.kernel.org/r/20210908140308.18491-3-jaharkes@cs.cmu.edu Signed-off-by: Jan Harkes Cc: Alex Shi Cc: Jing Yangyang Cc: Xin Tan Cc: Xiyu Yang Cc: Zeal Robot Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- fs/coda/upcall.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'fs/coda') diff --git a/fs/coda/upcall.c b/fs/coda/upcall.c index eb3b1898da46..59f6cfd06f96 100644 --- a/fs/coda/upcall.c +++ b/fs/coda/upcall.c @@ -744,7 +744,8 @@ static int coda_upcall(struct venus_comm *vcp, list_add_tail(&req->uc_chain, &vcp->vc_pending); wake_up_interruptible(&vcp->vc_waitq); - if (req->uc_flags & CODA_REQ_ASYNC) { + /* We can return early on asynchronous requests */ + if (outSize == NULL) { mutex_unlock(&vcp->vc_mutex); return 0; } -- cgit v1.2.3-58-ga151 From b1deb685b07945bb374fa75857033bb658a1253b Mon Sep 17 00:00:00 2001 From: Alex Shi Date: Mon, 8 Nov 2021 18:34:36 -0800 Subject: coda: remove err which no one care No one care 'err' in func coda_release, so better remove it. Link: https://lkml.kernel.org/r/20210908140308.18491-4-jaharkes@cs.cmu.edu Signed-off-by: Alex Shi Signed-off-by: Jan Harkes Cc: Jing Yangyang Cc: Xin Tan Cc: Xiyu Yang Cc: Zeal Robot Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- fs/coda/file.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'fs/coda') diff --git a/fs/coda/file.c b/fs/coda/file.c index ef5ca22bfb3e..52deab784667 100644 --- a/fs/coda/file.c +++ b/fs/coda/file.c @@ -238,11 +238,10 @@ int coda_release(struct inode *coda_inode, struct file *coda_file) struct coda_file_info *cfi; struct coda_inode_info *cii; struct inode *host_inode; - int err; cfi = coda_ftoc(coda_file); - err = venus_close(coda_inode->i_sb, coda_i2f(coda_inode), + venus_close(coda_inode->i_sb, coda_i2f(coda_inode), coda_flags, coda_file->f_cred->fsuid); host_inode = file_inode(cfi->cfi_container); -- cgit v1.2.3-58-ga151 From 76097eb7a48a2ddcf4755773bd501c7aa14cbb7d Mon Sep 17 00:00:00 2001 From: Jan Harkes Date: Mon, 8 Nov 2021 18:34:39 -0800 Subject: coda: avoid flagging NULL inodes Somehow we hit a negative dentry in coda_rename even after checking with d_really_is_positive. Maybe something raced and turned the new_dentry negative while we were fixing up directory link counts. Link: https://lkml.kernel.org/r/20210908140308.18491-5-jaharkes@cs.cmu.edu Signed-off-by: Jan Harkes Cc: Alex Shi Cc: Jing Yangyang Cc: Xin Tan Cc: Xiyu Yang Cc: Zeal Robot Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- fs/coda/coda_linux.h | 3 +++ 1 file changed, 3 insertions(+) (limited to 'fs/coda') diff --git a/fs/coda/coda_linux.h b/fs/coda/coda_linux.h index e7b27754ce78..3c2947bba5e5 100644 --- a/fs/coda/coda_linux.h +++ b/fs/coda/coda_linux.h @@ -83,6 +83,9 @@ static __inline__ void coda_flag_inode(struct inode *inode, int flag) { struct coda_inode_info *cii = ITOC(inode); + if (!inode) + return; + spin_lock(&cii->c_lock); cii->c_flags |= flag; spin_unlock(&cii->c_lock); -- cgit v1.2.3-58-ga151 From b2e36228367a8a097c7d15670fad47d77098f56d Mon Sep 17 00:00:00 2001 From: Jan Harkes Date: Mon, 8 Nov 2021 18:34:42 -0800 Subject: coda: avoid hidden code duplication in rename We were actually fixing up the directory mtime in both branches after the negative dentry test, it was just that one branch was only flagging the directory inodes to refresh their attributes while the other branch used the optional optimization to set mtime to the current time and not go back to the Coda client. Link: https://lkml.kernel.org/r/20210908140308.18491-6-jaharkes@cs.cmu.edu Signed-off-by: Jan Harkes Cc: Alex Shi Cc: Jing Yangyang Cc: Xin Tan Cc: Xiyu Yang Cc: Zeal Robot Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- fs/coda/dir.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) (limited to 'fs/coda') diff --git a/fs/coda/dir.c b/fs/coda/dir.c index 3fd085009f26..328d7a684b63 100644 --- a/fs/coda/dir.c +++ b/fs/coda/dir.c @@ -317,13 +317,10 @@ static int coda_rename(struct user_namespace *mnt_userns, struct inode *old_dir, coda_dir_drop_nlink(old_dir); coda_dir_inc_nlink(new_dir); } - coda_dir_update_mtime(old_dir); - coda_dir_update_mtime(new_dir); coda_flag_inode(d_inode(new_dentry), C_VATTR); - } else { - coda_flag_inode(old_dir, C_VATTR); - coda_flag_inode(new_dir, C_VATTR); } + coda_dir_update_mtime(old_dir); + coda_dir_update_mtime(new_dir); } return error; } -- cgit v1.2.3-58-ga151 From 5a646fb3a3e2de8bbab19d41826b3e54b09969bc Mon Sep 17 00:00:00 2001 From: Jan Harkes Date: Mon, 8 Nov 2021 18:34:45 -0800 Subject: coda: avoid doing bad things on inode type changes during revalidation When Coda discovers an inconsistent object, it turns it into a symlink. However we can't just follow this change in the kernel on an existing file or directory inode that may still have references. This patch removes the inconsistent inode from the inode hash and allocates a new inode for the symlink object. Link: https://lkml.kernel.org/r/20210908140308.18491-7-jaharkes@cs.cmu.edu Signed-off-by: Jan Harkes Cc: Alex Shi Cc: Jing Yangyang Cc: Xin Tan Cc: Xiyu Yang Cc: Zeal Robot Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- fs/coda/cnode.c | 13 +++++++++---- fs/coda/coda_linux.c | 39 +++++++++++++++++++-------------------- fs/coda/coda_linux.h | 3 ++- 3 files changed, 30 insertions(+), 25 deletions(-) (limited to 'fs/coda') diff --git a/fs/coda/cnode.c b/fs/coda/cnode.c index 06855f6c7902..62a3d2565c26 100644 --- a/fs/coda/cnode.c +++ b/fs/coda/cnode.c @@ -63,9 +63,10 @@ struct inode * coda_iget(struct super_block * sb, struct CodaFid * fid, struct inode *inode; struct coda_inode_info *cii; unsigned long hash = coda_f2i(fid); + umode_t inode_type = coda_inode_type(attr); +retry: inode = iget5_locked(sb, hash, coda_test_inode, coda_set_inode, fid); - if (!inode) return ERR_PTR(-ENOMEM); @@ -75,11 +76,15 @@ struct inode * coda_iget(struct super_block * sb, struct CodaFid * fid, inode->i_ino = hash; /* inode is locked and unique, no need to grab cii->c_lock */ cii->c_mapcount = 0; + coda_fill_inode(inode, attr); unlock_new_inode(inode); + } else if ((inode->i_mode & S_IFMT) != inode_type) { + /* Inode has changed type, mark bad and grab a new one */ + remove_inode_hash(inode); + coda_flag_inode(inode, C_PURGE); + iput(inode); + goto retry; } - - /* always replace the attributes, type might have changed */ - coda_fill_inode(inode, attr); return inode; } diff --git a/fs/coda/coda_linux.c b/fs/coda/coda_linux.c index 2e1a5a192074..903ca8fa4b9b 100644 --- a/fs/coda/coda_linux.c +++ b/fs/coda/coda_linux.c @@ -87,28 +87,27 @@ static struct coda_timespec timespec64_to_coda(struct timespec64 ts64) } /* utility functions below */ +umode_t coda_inode_type(struct coda_vattr *attr) +{ + switch (attr->va_type) { + case C_VREG: + return S_IFREG; + case C_VDIR: + return S_IFDIR; + case C_VLNK: + return S_IFLNK; + case C_VNON: + default: + return 0; + } +} + void coda_vattr_to_iattr(struct inode *inode, struct coda_vattr *attr) { - int inode_type; - /* inode's i_flags, i_ino are set by iget - XXX: is this all we need ?? - */ - switch (attr->va_type) { - case C_VNON: - inode_type = 0; - break; - case C_VREG: - inode_type = S_IFREG; - break; - case C_VDIR: - inode_type = S_IFDIR; - break; - case C_VLNK: - inode_type = S_IFLNK; - break; - default: - inode_type = 0; - } + /* inode's i_flags, i_ino are set by iget + * XXX: is this all we need ?? + */ + umode_t inode_type = coda_inode_type(attr); inode->i_mode |= inode_type; if (attr->va_mode != (u_short) -1) diff --git a/fs/coda/coda_linux.h b/fs/coda/coda_linux.h index 3c2947bba5e5..9be281bbcc06 100644 --- a/fs/coda/coda_linux.h +++ b/fs/coda/coda_linux.h @@ -53,10 +53,11 @@ int coda_getattr(struct user_namespace *, const struct path *, struct kstat *, u32, unsigned int); int coda_setattr(struct user_namespace *, struct dentry *, struct iattr *); -/* this file: heloers */ +/* this file: helpers */ char *coda_f2s(struct CodaFid *f); int coda_iscontrol(const char *name, size_t length); +umode_t coda_inode_type(struct coda_vattr *attr); void coda_vattr_to_iattr(struct inode *, struct coda_vattr *); void coda_iattr_to_vattr(struct iattr *, struct coda_vattr *); unsigned short coda_flags_to_cflags(unsigned short); -- cgit v1.2.3-58-ga151 From 1077c2857791076a7e81b0ba91571f136cee08e4 Mon Sep 17 00:00:00 2001 From: Xiyu Yang Date: Mon, 8 Nov 2021 18:34:48 -0800 Subject: coda: convert from atomic_t to refcount_t on coda_vm_ops->refcnt refcount_t type and corresponding API can protect refcounters from accidental underflow and overflow and further use-after-free situations. Link: https://lkml.kernel.org/r/20210908140308.18491-8-jaharkes@cs.cmu.edu Signed-off-by: Xiyu Yang Signed-off-by: Xin Tan Signed-off-by: Jan Harkes Cc: Alex Shi Cc: Jing Yangyang Cc: Zeal Robot Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- fs/coda/file.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) (limited to 'fs/coda') diff --git a/fs/coda/file.c b/fs/coda/file.c index 52deab784667..29dd87be2fb8 100644 --- a/fs/coda/file.c +++ b/fs/coda/file.c @@ -8,6 +8,7 @@ * to the Coda project. Contact Peter Braam . */ +#include #include #include #include @@ -28,7 +29,7 @@ #include "coda_int.h" struct coda_vm_ops { - atomic_t refcnt; + refcount_t refcnt; struct file *coda_file; const struct vm_operations_struct *host_vm_ops; struct vm_operations_struct vm_ops; @@ -98,7 +99,7 @@ coda_vm_open(struct vm_area_struct *vma) struct coda_vm_ops *cvm_ops = container_of(vma->vm_ops, struct coda_vm_ops, vm_ops); - atomic_inc(&cvm_ops->refcnt); + refcount_inc(&cvm_ops->refcnt); if (cvm_ops->host_vm_ops && cvm_ops->host_vm_ops->open) cvm_ops->host_vm_ops->open(vma); @@ -113,7 +114,7 @@ coda_vm_close(struct vm_area_struct *vma) if (cvm_ops->host_vm_ops && cvm_ops->host_vm_ops->close) cvm_ops->host_vm_ops->close(vma); - if (atomic_dec_and_test(&cvm_ops->refcnt)) { + if (refcount_dec_and_test(&cvm_ops->refcnt)) { vma->vm_ops = cvm_ops->host_vm_ops; fput(cvm_ops->coda_file); kfree(cvm_ops); @@ -189,7 +190,7 @@ coda_file_mmap(struct file *coda_file, struct vm_area_struct *vma) cvm_ops->vm_ops.open = coda_vm_open; cvm_ops->vm_ops.close = coda_vm_close; cvm_ops->coda_file = coda_file; - atomic_set(&cvm_ops->refcnt, 1); + refcount_set(&cvm_ops->refcnt, 1); vma->vm_ops = &cvm_ops->vm_ops; } -- cgit v1.2.3-58-ga151 From 118b7ee169d2e469a080bf1d2fa68cd31452bdad Mon Sep 17 00:00:00 2001 From: Jing Yangyang Date: Mon, 8 Nov 2021 18:34:52 -0800 Subject: coda: use vmemdup_user to replace the open code vmemdup_user is better than duplicating its implementation, So just replace the open code. fs/coda/psdev.c:125:10-18:WARNING:opportunity for vmemdup_user The issue is detected with the help of Coccinelle. Link: https://lkml.kernel.org/r/20210908140308.18491-9-jaharkes@cs.cmu.edu Reported-by: Zeal Robot Signed-off-by: Jing Yangyang Signed-off-by: Jan Harkes Cc: Alex Shi Cc: Xin Tan Cc: Xiyu Yang Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- fs/coda/psdev.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) (limited to 'fs/coda') diff --git a/fs/coda/psdev.c b/fs/coda/psdev.c index 240669f51eac..7e23cb22d394 100644 --- a/fs/coda/psdev.c +++ b/fs/coda/psdev.c @@ -122,14 +122,10 @@ static ssize_t coda_psdev_write(struct file *file, const char __user *buf, hdr.opcode, hdr.unique); nbytes = size; } - dcbuf = kvmalloc(nbytes, GFP_KERNEL); - if (!dcbuf) { - retval = -ENOMEM; - goto out; - } - if (copy_from_user(dcbuf, buf, nbytes)) { - kvfree(dcbuf); - retval = -EFAULT; + + dcbuf = vmemdup_user(buf, nbytes); + if (IS_ERR(dcbuf)) { + retval = PTR_ERR(dcbuf); goto out; } -- cgit v1.2.3-58-ga151 From 98d5b61ef5fae7681df27065ad95ee6e30c42243 Mon Sep 17 00:00:00 2001 From: Jan Harkes Date: Mon, 8 Nov 2021 18:34:55 -0800 Subject: coda: bump module version to 7.2 Helps with tracking which patches have been propagated upstream and if users are running the latest known version. Link: https://lkml.kernel.org/r/20210908140308.18491-10-jaharkes@cs.cmu.edu Signed-off-by: Jan Harkes Cc: Alex Shi Cc: Jing Yangyang Cc: Xin Tan Cc: Xiyu Yang Cc: Zeal Robot Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- fs/coda/psdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'fs/coda') diff --git a/fs/coda/psdev.c b/fs/coda/psdev.c index 7e23cb22d394..b39580ad4ce5 100644 --- a/fs/coda/psdev.c +++ b/fs/coda/psdev.c @@ -384,7 +384,7 @@ MODULE_AUTHOR("Jan Harkes, Peter J. Braam"); MODULE_DESCRIPTION("Coda Distributed File System VFS interface"); MODULE_ALIAS_CHARDEV_MAJOR(CODA_PSDEV_MAJOR); MODULE_LICENSE("GPL"); -MODULE_VERSION("7.0"); +MODULE_VERSION("7.2"); static int __init init_coda(void) { -- cgit v1.2.3-58-ga151