From f2aae745b82c842221f4f233051f9ac641790959 Mon Sep 17 00:00:00 2001 From: Zhihao Cheng Date: Mon, 1 Jun 2020 17:10:36 +0800 Subject: [PATCH 1/6] ubifs: xattr: Fix some potential memory leaks while iterating entries Fix some potential memory leaks in error handling branches while iterating xattr entries. For example, function ubifs_tnc_remove_ino() forgets to free pxent if it exists. Similar problems also exist in ubifs_purge_xattrs(), ubifs_add_orphan() and ubifs_jnl_write_inode(). Signed-off-by: Zhihao Cheng Cc: Fixes: 1e51764a3c2ac05a2 ("UBIFS: add new flash file system") Signed-off-by: Richard Weinberger --- fs/ubifs/journal.c | 2 ++ fs/ubifs/orphan.c | 2 ++ fs/ubifs/tnc.c | 3 +++ fs/ubifs/xattr.c | 2 ++ 4 files changed, 9 insertions(+) diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c index 4a5b06f8d812..ed935aefe3e0 100644 --- a/fs/ubifs/journal.c +++ b/fs/ubifs/journal.c @@ -894,6 +894,7 @@ int ubifs_jnl_write_inode(struct ubifs_info *c, const struct inode *inode) if (err == -ENOENT) break; + kfree(pxent); goto out_release; } @@ -906,6 +907,7 @@ int ubifs_jnl_write_inode(struct ubifs_info *c, const struct inode *inode) ubifs_err(c, "dead directory entry '%s', error %d", xent->name, err); ubifs_ro_mode(c, err); + kfree(pxent); kfree(xent); goto out_release; } diff --git a/fs/ubifs/orphan.c b/fs/ubifs/orphan.c index 2c294085ffed..0fb61956146d 100644 --- a/fs/ubifs/orphan.c +++ b/fs/ubifs/orphan.c @@ -173,6 +173,7 @@ int ubifs_add_orphan(struct ubifs_info *c, ino_t inum) err = PTR_ERR(xent); if (err == -ENOENT) break; + kfree(pxent); return err; } @@ -182,6 +183,7 @@ int ubifs_add_orphan(struct ubifs_info *c, ino_t inum) xattr_orphan = orphan_add(c, xattr_inum, orphan); if (IS_ERR(xattr_orphan)) { + kfree(pxent); kfree(xent); return PTR_ERR(xattr_orphan); } diff --git a/fs/ubifs/tnc.c b/fs/ubifs/tnc.c index f609f6cdde70..b120a00773f8 100644 --- a/fs/ubifs/tnc.c +++ b/fs/ubifs/tnc.c @@ -2885,6 +2885,7 @@ int ubifs_tnc_remove_ino(struct ubifs_info *c, ino_t inum) err = PTR_ERR(xent); if (err == -ENOENT) break; + kfree(pxent); return err; } @@ -2898,6 +2899,7 @@ int ubifs_tnc_remove_ino(struct ubifs_info *c, ino_t inum) fname_len(&nm) = le16_to_cpu(xent->nlen); err = ubifs_tnc_remove_nm(c, &key1, &nm); if (err) { + kfree(pxent); kfree(xent); return err; } @@ -2906,6 +2908,7 @@ int ubifs_tnc_remove_ino(struct ubifs_info *c, ino_t inum) highest_ino_key(c, &key2, xattr_inum); err = ubifs_tnc_remove_range(c, &key1, &key2); if (err) { + kfree(pxent); kfree(xent); return err; } diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c index 9aefbb60074f..a0b9b349efe6 100644 --- a/fs/ubifs/xattr.c +++ b/fs/ubifs/xattr.c @@ -522,6 +522,7 @@ int ubifs_purge_xattrs(struct inode *host) xent->name, err); ubifs_ro_mode(c, err); kfree(pxent); + kfree(xent); return err; } @@ -531,6 +532,7 @@ int ubifs_purge_xattrs(struct inode *host) err = remove_xattr(c, host, xino, &nm); if (err) { kfree(pxent); + kfree(xent); iput(xino); ubifs_err(c, "cannot remove xattr, error %d", err); return err; From 58f6e78a65f1fcbf732f60a7478ccc99873ff3ba Mon Sep 17 00:00:00 2001 From: Zhihao Cheng Date: Mon, 1 Jun 2020 17:10:37 +0800 Subject: [PATCH 2/6] ubifs: dent: Fix some potential memory leaks while iterating entries Fix some potential memory leaks in error handling branches while iterating dent entries. For example, function dbg_check_dir() forgets to free pdent if it exists. Signed-off-by: Zhihao Cheng Cc: Fixes: 1e51764a3c2ac05a2 ("UBIFS: add new flash file system") Signed-off-by: Richard Weinberger --- fs/ubifs/debug.c | 1 + 1 file changed, 1 insertion(+) diff --git a/fs/ubifs/debug.c b/fs/ubifs/debug.c index 31288d8fa2ce..ebff43f8009c 100644 --- a/fs/ubifs/debug.c +++ b/fs/ubifs/debug.c @@ -1123,6 +1123,7 @@ int dbg_check_dir(struct ubifs_info *c, const struct inode *dir) err = PTR_ERR(dent); if (err == -ENOENT) break; + kfree(pdent); return err; } From d005f8c6588efcfbe88099b6edafc6f58c84a9c1 Mon Sep 17 00:00:00 2001 From: Zhihao Cheng Date: Mon, 1 Jun 2020 17:12:31 +0800 Subject: [PATCH 3/6] ubi: check kthread_should_stop() after the setting of task state A detach hung is possible when a race occurs between the detach process and the ubi background thread. The following sequences outline the race: ubi thread: if (list_empty(&ubi->works)... ubi detach: set_bit(KTHREAD_SHOULD_STOP, &kthread->flags) => by kthread_stop() wake_up_process() => ubi thread is still running, so 0 is returned ubi thread: set_current_state(TASK_INTERRUPTIBLE) schedule() => ubi thread will never be scheduled again ubi detach: wait_for_completion() => hung task! To fix that, we need to check kthread_should_stop() after we set the task state, so the ubi thread will either see the stop bit and exit or the task state is reset to runnable such that it isn't scheduled out indefinitely. Signed-off-by: Zhihao Cheng Cc: Fixes: 801c135ce73d5df1ca ("UBI: Unsorted Block Images") Reported-by: syzbot+853639d0cb16c31c7a14@syzkaller.appspotmail.com Signed-off-by: Richard Weinberger --- drivers/mtd/ubi/wl.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c index 42cac572f82d..7847de75a74c 100644 --- a/drivers/mtd/ubi/wl.c +++ b/drivers/mtd/ubi/wl.c @@ -1639,6 +1639,19 @@ int ubi_thread(void *u) !ubi->thread_enabled || ubi_dbg_is_bgt_disabled(ubi)) { set_current_state(TASK_INTERRUPTIBLE); spin_unlock(&ubi->wl_lock); + + /* + * Check kthread_should_stop() after we set the task + * state to guarantee that we either see the stop bit + * and exit or the task state is reset to runnable such + * that it's not scheduled out indefinitely and detects + * the stop bit at kthread_should_stop(). + */ + if (kthread_should_stop()) { + set_current_state(TASK_RUNNING); + break; + } + schedule(); continue; } From dd7db149bcd914db8fdeb19cd5597c0740121bc7 Mon Sep 17 00:00:00 2001 From: Zhihao Cheng Date: Mon, 17 Aug 2020 22:29:09 +0800 Subject: [PATCH 4/6] ubifs: ubifs_jnl_change_xattr: Remove assertion 'nlink > 0' for host inode Changing xattr of a temp file will trigger following assertion failed and make ubifs turn into readonly filesystem: ubifs_assert_failed [ubifs]: UBIFS assert failed: host->i_nlink > 0, in fs/ubifs/journal.c:1801 Reproducer: 1. fd = open(__O_TMPFILE) 2. fsetxattr(fd, key, value2, XATTR_CREATE) 3. fsetxattr(fd, key, value2, XATTR_REPLACE) Fix this by removing assertion 'nlink > 0' for host inode. Reported-by: Chengsong Ke Signed-off-by: Zhihao Cheng Signed-off-by: Richard Weinberger --- fs/ubifs/journal.c | 1 - 1 file changed, 1 deletion(-) diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c index ed935aefe3e0..cb1fa0c37322 100644 --- a/fs/ubifs/journal.c +++ b/fs/ubifs/journal.c @@ -1800,7 +1800,6 @@ int ubifs_jnl_change_xattr(struct ubifs_info *c, const struct inode *inode, u8 hash[UBIFS_HASH_ARR_SZ]; dbg_jnl("ino %lu, ino %lu", host->i_ino, inode->i_ino); - ubifs_assert(c, host->i_nlink > 0); ubifs_assert(c, inode->i_nlink > 0); ubifs_assert(c, mutex_is_locked(&host_ui->ui_mutex)); From 121b8fcbf98892d5f26d8326a00d09e4cdb76369 Mon Sep 17 00:00:00 2001 From: Zhihao Cheng Date: Fri, 28 Aug 2020 11:32:50 +0800 Subject: [PATCH 5/6] ubifs: setflags: Don't show error message when vfs_ioc_setflags_prepare() fails Following process will trigger ubifs_err: 1. useradd -m freg (Under root) 2. cd /home/freg && mkdir mp (Under freg) 3. mount -t ubifs /dev/ubi0_0 /home/freg/mp (Under root) 4. cd /home/freg && echo 123 > mp/a (Under root) 5. cd mp && chown freg a && chgrp freg a && chmod 777 a (Under root) 6. chattr +i a (Under freg) UBIFS error (ubi0:0 pid 1723): ubifs_ioctl [ubifs]: can't modify inode 65 attributes chattr: Operation not permitted while setting flags on a This is not an UBIFS problem, it was caused by task priviliage checking on file operations. Remove error message printing from kernel just like other filesystems (eg. ext4), since we already have enough information from userspace tools. Signed-off-by: Zhihao Cheng Signed-off-by: Richard Weinberger --- fs/ubifs/ioctl.c | 1 - 1 file changed, 1 deletion(-) diff --git a/fs/ubifs/ioctl.c b/fs/ubifs/ioctl.c index 3df9be2c684c..4363d85a3fd4 100644 --- a/fs/ubifs/ioctl.c +++ b/fs/ubifs/ioctl.c @@ -134,7 +134,6 @@ static int setflags(struct inode *inode, int flags) return err; out_unlock: - ubifs_err(c, "can't modify inode %lu attributes", inode->i_ino); mutex_unlock(&ui->ui_mutex); ubifs_release_budget(c, &req); return err; From 78c7d49f55d8631b67c09f9bfbe8155211a9ea06 Mon Sep 17 00:00:00 2001 From: Richard Weinberger Date: Mon, 28 Sep 2020 20:58:59 +0200 Subject: [PATCH 6/6] ubifs: journal: Make sure to not dirty twice for auth nodes When removing the last reference of an inode the size of an auth node is already part of write_len. So we must not call ubifs_add_auth_dirt(). Call it only when needed. Cc: Cc: Sascha Hauer Cc: Kristof Havasi Fixes: 6a98bc4614de ("ubifs: Add authentication nodes to journal") Reported-and-tested-by: Kristof Havasi Reviewed-by: Sascha Hauer Signed-off-by: Richard Weinberger --- fs/ubifs/journal.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c index cb1fa0c37322..091c2ad8f211 100644 --- a/fs/ubifs/journal.c +++ b/fs/ubifs/journal.c @@ -938,8 +938,6 @@ int ubifs_jnl_write_inode(struct ubifs_info *c, const struct inode *inode) inode->i_ino); release_head(c, BASEHD); - ubifs_add_auth_dirt(c, lnum); - if (last_reference) { err = ubifs_tnc_remove_ino(c, inode->i_ino); if (err) @@ -949,6 +947,8 @@ int ubifs_jnl_write_inode(struct ubifs_info *c, const struct inode *inode) } else { union ubifs_key key; + ubifs_add_auth_dirt(c, lnum); + ino_key_init(c, &key, inode->i_ino); err = ubifs_tnc_add(c, &key, lnum, offs, ilen, hash); }