Skip to contentSkip to conversation

name_is_dot* cleanup

PATCH Applied Amir Goldstein 4 participants 9 messages Jan 28 – Jan 29

Subsystems (7)
lore
Share & tools
lore

Amir Goldstein

author maintainer
#0 lore raw

cover name_is_dot* cleanup

Miklos,

Following the syzbot ovl bug report and a fix by Qing Wang, I decided to follow up with a small vfs cleanup of some open coded version of checking "." and ".." name in readdir.

The fix patch is applied at the start of this cleanup series to allow for easy backporting, but it is not an urgent fix so I don't think there is a need to fast track it.

Christian,

I am assuming that you would want to take the vfs cleanup via your tree, so might as well take the ovl adjacent patches with it.

If you want me to drive this entire series via ovl tree, please ack the vfs cleanup patch.

Thanks,
Amir.

[1] https://lore.kernel.org/linux-unionfs/[email protected]/

Amir Goldstein (2):
fs: add helpers name_is_dot{,dot,_dotdot}
ovl: use name_is_dot* helpers in readdir code

Qing Wang (1):
ovl: Fix uninit-value in ovl_fill_real

 fs/crypto/fname.c      |  2 +-
 fs/ecryptfs/crypto.c   |  2 +-
 fs/exportfs/expfs.c    |  3 ++-
 fs/f2fs/dir.c          |  2 +-
 fs/f2fs/hash.c         |  2 +-
 fs/namei.c             |  2 +-
 fs/overlayfs/readdir.c | 39 +++++++++++++++------------------------
 fs/smb/server/vfs.c    |  2 +-
 include/linux/fs.h     | 14 ++++++++++++--
 9 files changed, 35 insertions(+), 33 deletions(-)
Signature · 1 line
2.52.0
author maintainer
#1 lore raw

PATCH 1/3 ovl: Fix uninit-value in ovl_fill_real

From: Qing Wang <[email protected]>

Syzbot reported a KMSAN uninit-value issue in ovl_fill_real.

This iusse's call chain is:
__do_sys_getdents64()

    -> iterate_dir()
        ...
            -> ext4_readdir()
                -> fscrypt_fname_alloc_buffer() // alloc
                -> fscrypt_fname_disk_to_usr // write without tail '\0'
                -> dir_emit()
                    -> ovl_fill_real() // read by strcmp()

The string is used to store the decrypted directory entry name for an encrypted inode. As shown in the call chain, fscrypt_fname_disk_to_usr() write it without null-terminate. However, ovl_fill_real() uses strcmp() to compare the name against "..", which assumes a null-terminated string and may trigger a KMSAN uninit-value warning when the buffer tail contains uninit data.

Reported-by [email protected]
Fixes 4edb83bb1041 ("ovl: constant d_ino for non-merge dirs")
---
 fs/overlayfs/readdir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/overlayfs/readdir.c +1 -1
index 160960bb0ad0b..724ec9d93fc82 100644
@@ -755,7 +755,7 @@ static bool ovl_fill_real(struct dir_context *ctx, const char *name,
 	struct dir_context *orig_ctx = rdt->orig_ctx;
 	bool res;
 
-	if (rdt->parent_ino && strcmp(name, "..") == 0) {
+	if (rdt->parent_ino && namelen == 2 && !strncmp(name, "..", 2)) {
 		ino = rdt->parent_ino;
 	} else if (rdt->cache) {
 		struct ovl_cache_entry *p;
Signature · 1 line
2.52.0
maintainer Acked-by on v1 1/3
#2 lore raw

Re: [PATCH 1/3] ovl: Fix uninit-value in ovl_fill_real

Quoted 22 lines · Amir Goldstein

From: Qing Wang <[email protected]>

Syzbot reported a KMSAN uninit-value issue in ovl_fill_real.

This iusse's call chain is:
__do_sys_getdents64()

    -> iterate_dir()
        ...
            -> ext4_readdir()
                -> fscrypt_fname_alloc_buffer() // alloc
                -> fscrypt_fname_disk_to_usr // write without tail '\0'
                -> dir_emit()
                    -> ovl_fill_real() // read by strcmp()

The string is used to store the decrypted directory entry name for an encrypted inode. As shown in the call chain, fscrypt_fname_disk_to_usr() write it without null-terminate. However, ovl_fill_real() uses strcmp() to compare the name against "..", which assumes a null-terminated string and may trigger a KMSAN uninit-value warning when the buffer tail contains uninit data.

Quoted 3 lines
Reported-by [email protected]
Fixes 4edb83bb1041 ("ovl: constant d_ino for non-merge dirs")

Thanks,
Miklos

author maintainer
#3 lore raw

PATCH 2/3 fs: add helpers name_is_dot{,dot,_dotdot}

In reply to Amir Goldstein

Rename the helper is_dot_dotdot() into the name_ namespace and add complementary helpers to check for dot and dotdot names individually.

---
 fs/crypto/fname.c      |  2 +-
 fs/ecryptfs/crypto.c   |  2 +-
 fs/exportfs/expfs.c    |  3 ++-
 fs/f2fs/dir.c          |  2 +-
 fs/f2fs/hash.c         |  2 +-
 fs/namei.c             |  2 +-
 fs/overlayfs/readdir.c |  3 ++-
 fs/smb/server/vfs.c    |  2 +-
 include/linux/fs.h     | 14 ++++++++++++--
 9 files changed, 22 insertions(+), 10 deletions(-)
fs/crypto/fname.c +1 -1
index a9a4432d12ba1..629eb0d72e860 100644
@@ -76,7 +76,7 @@ struct fscrypt_nokey_name {
 
 static inline bool fscrypt_is_dot_dotdot(const struct qstr *str)
 {
-	return is_dot_dotdot(str->name, str->len);
+	return name_is_dot_dotdot(str->name, str->len);
 }
 
 /**
fs/ecryptfs/crypto.c +1 -1
index 260f8a4938b01..3c89f06c74532 100644
@@ -1904,7 +1904,7 @@ int ecryptfs_decode_and_decrypt_filename(char **plaintext_name,
 
 	if ((mount_crypt_stat->flags & ECRYPTFS_GLOBAL_ENCRYPT_FILENAMES) &&
 	    !(mount_crypt_stat->flags & ECRYPTFS_ENCRYPTED_VIEW_ENABLED)) {
-		if (is_dot_dotdot(name, name_size)) {
+		if (name_is_dot_dotdot(name, name_size)) {
 			rc = ecryptfs_copy_filename(plaintext_name,
 						    plaintext_name_size,
 						    name, name_size);
fs/exportfs/expfs.c +2 -1
index d3e55de4a2a2a..6c9be60a3e48d 100644
@@ -253,7 +253,8 @@ static bool filldir_one(struct dir_context *ctx, const char *name, int len,
 		container_of(ctx, struct getdents_callback, ctx);
 
 	buf->sequence++;
-	if (buf->ino == ino && len <= NAME_MAX && !is_dot_dotdot(name, len)) {
+	if (buf->ino == ino && len <= NAME_MAX &&
+	    !name_is_dot_dotdot(name, len)) {
 		memcpy(buf->name, name, len);
 		buf->name[len] = '\0';
 		buf->found = 1;
fs/f2fs/dir.c +1 -1
index 48f4f98afb013..29412e6e078d0 100644
@@ -67,7 +67,7 @@ int f2fs_init_casefolded_name(const struct inode *dir,
 	int len;
 
 	if (IS_CASEFOLDED(dir) &&
-	    !is_dot_dotdot(fname->usr_fname->name, fname->usr_fname->len)) {
+	    !name_is_dot_dotdot(fname->usr_fname->name, fname->usr_fname->len)) {
 		buf = f2fs_kmem_cache_alloc(f2fs_cf_name_slab,
 					    GFP_NOFS, false, F2FS_SB(sb));
 		if (!buf)
fs/f2fs/hash.c +1 -1
index 049ce50cec9b0..14082fe5e6b29 100644
@@ -100,7 +100,7 @@ void f2fs_hash_filename(const struct inode *dir, struct f2fs_filename *fname)
 
 	WARN_ON_ONCE(!name);
 
-	if (is_dot_dotdot(name, len)) {
+	if (name_is_dot_dotdot(name, len)) {
 		fname->hash = 0;
 		return;
 	}
fs/namei.c +1 -1
index b3fe9d4a7037e..63bbcea75bbd3 100644
@@ -3050,7 +3050,7 @@ int lookup_noperm_common(struct qstr *qname, struct dentry *base)
 	if (!len)
 		return -EACCES;
 
-	if (is_dot_dotdot(name, len))
+	if (name_is_dot_dotdot(name, len))
 		return -EACCES;
 
 	while (len--) {
fs/overlayfs/readdir.c +2 -1
index 724ec9d93fc82..9f6b36f3d4cf8 100644
@@ -76,7 +76,8 @@ static int ovl_casefold(struct ovl_readdir_data *rdd, const char *str, int len,
 	char *cf_name;
 	int cf_len;
 
-	if (!IS_ENABLED(CONFIG_UNICODE) || !rdd->map || is_dot_dotdot(str, len))
+	if (!IS_ENABLED(CONFIG_UNICODE) || !rdd->map ||
+	    name_is_dot_dotdot(str, len))
 		return 0;
 
 	cf_name = kmalloc(NAME_MAX, GFP_KERNEL);
fs/smb/server/vfs.c +1 -1
index b8e648b8300f6..b38eb8e0b3eb7 100644
@@ -1052,7 +1052,7 @@ static bool __dir_empty(struct dir_context *ctx, const char *name, int namlen,
 	struct ksmbd_readdir_data *buf;
 
 	buf = container_of(ctx, struct ksmbd_readdir_data, ctx);
-	if (!is_dot_dotdot(name, namlen))
+	if (!name_is_dot_dotdot(name, namlen))
 		buf->dirent_count++;
 
 	return !buf->dirent_count;
include/linux/fs.h +12 -2
index b1310648bb585..43f9baef4dd36 100644
@@ -2846,12 +2846,22 @@ u64 vfsmount_to_propagation_flags(struct vfsmount *mnt);
 
 extern char *file_path(struct file *, char *, int);
 
+static inline bool name_is_dot(const char *name, size_t len)
+{
+	return unlikely(len == 1 && name[0] == '.');
+}
+
+static inline bool name_is_dotdot(const char *name, size_t len)
+{
+	return unlikely(len == 2 && name[0] == '.' && name[1] == '.');
+}
+
 /**
- * is_dot_dotdot - returns true only if @name is "." or ".."
+ * name_is_dot_dotdot - returns true only if @name is "." or ".."
  * @name: file name to check
  * @len: length of file name, in bytes
  */
-static inline bool is_dot_dotdot(const char *name, size_t len)
+static inline bool name_is_dot_dotdot(const char *name, size_t len)
 {
 	return len && unlikely(name[0] == '.') &&
 		(len == 1 || (len == 2 && name[1] == '.'));
Signature · 1 line
2.52.0
author maintainer
#4 lore raw

PATCH 3/3 ovl: use name_is_dot* helpers in readdir code

In reply to Amir Goldstein

Use the helpers in place of all the different open coded variants. This makes the code more readable and robust.

---
 fs/overlayfs/readdir.c | 36 +++++++++++++-----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)
fs/overlayfs/readdir.c +13 -23
index 9f6b36f3d4cf8..5727948d6566a 100644
@@ -154,7 +154,7 @@ static bool ovl_calc_d_ino(struct ovl_readdir_data *rdd,
 		return true;
 
 	/* Always recalc d_ino for parent */
-	if (strcmp(p->name, "..") == 0)
+	if (name_is_dotdot(p->name, p->len))
 		return true;
 
 	/* If this is lower, then native d_ino will do */
@@ -165,7 +165,7 @@ static bool ovl_calc_d_ino(struct ovl_readdir_data *rdd,
 	 * Recalc d_ino for '.' and for all entries if dir is impure (contains
 	 * copied up entries)
 	 */
-	if ((p->name[0] == '.' && p->len == 1) ||
+	if (name_is_dot(p->name, p->len) ||
 	    ovl_test_flag(OVL_IMPURE, d_inode(rdd->dentry)))
 		return true;
 
@@ -561,12 +561,12 @@ static int ovl_cache_update(const struct path *path, struct ovl_cache_entry *p,
 	if (!ovl_same_dev(ofs) && !p->check_xwhiteout)
 		goto out;
 
-	if (p->name[0] == '.') {
+	if (name_is_dot_dotdot(p->name, p->len)) {
 		if (p->len == 1) {
 			this = dget(dir);
 			goto get;
 		}
-		if (p->len == 2 && p->name[1] == '.') {
+		if (p->len == 2) {
 			/* we shall not be moved */
 			this = dget(dir->d_parent);
 			goto get;
@@ -666,8 +666,7 @@ static int ovl_dir_read_impure(const struct path *path,  struct list_head *list,
 		return err;
 
 	list_for_each_entry_safe(p, n, list, l_node) {
-		if (strcmp(p->name, ".") != 0 &&
-		    strcmp(p->name, "..") != 0) {
+		if (!name_is_dot_dotdot(p->name, p->len)) {
 			err = ovl_cache_update(path, p, true);
 			if (err)
 				return err;
@@ -756,7 +755,7 @@ static bool ovl_fill_real(struct dir_context *ctx, const char *name,
 	struct dir_context *orig_ctx = rdt->orig_ctx;
 	bool res;
 
-	if (rdt->parent_ino && namelen == 2 && !strncmp(name, "..", 2)) {
+	if (rdt->parent_ino && name_is_dotdot(name, namelen)) {
 		ino = rdt->parent_ino;
 	} else if (rdt->cache) {
 		struct ovl_cache_entry *p;
@@ -1097,11 +1096,8 @@ int ovl_check_empty_dir(struct dentry *dentry, struct list_head *list)
 			goto del_entry;
 		}
 
-		if (p->name[0] == '.') {
-			if (p->len == 1)
-				goto del_entry;
-			if (p->len == 2 && p->name[1] == '.')
-				goto del_entry;
+		if (name_is_dot_dotdot(p->name, p->len)) {
+			goto del_entry;
 		}
 		err = -ENOTEMPTY;
 		break;
@@ -1146,7 +1142,7 @@ static bool ovl_check_d_type(struct dir_context *ctx, const char *name,
 		container_of(ctx, struct ovl_readdir_data, ctx);
 
 	/* Even if d_type is not supported, DT_DIR is returned for . and .. */
-	if (!strncmp(name, ".", namelen) || !strncmp(name, "..", namelen))
+	if (name_is_dot_dotdot(name, namelen))
 		return true;
 
 	if (d_type != DT_UNKNOWN)
@@ -1209,11 +1205,8 @@ static int ovl_workdir_cleanup_recurse(struct ovl_fs *ofs, const struct path *pa
 	list_for_each_entry(p, &list, l_node) {
 		struct dentry *dentry;
 
-		if (p->name[0] == '.') {
-			if (p->len == 1)
-				continue;
-			if (p->len == 2 && p->name[1] == '.')
-				continue;
+		if (name_is_dot_dotdot(p->name, p->len)) {
+			continue;
 		} else if (incompat) {
 			pr_err("overlay with incompat feature '%s' cannot be mounted\n",
 				p->name);
@@ -1278,11 +1271,8 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
 		goto out;
 
 	list_for_each_entry(p, &list, l_node) {
-		if (p->name[0] == '.') {
-			if (p->len == 1)
-				continue;
-			if (p->len == 2 && p->name[1] == '.')
-				continue;
+		if (name_is_dot_dotdot(p->name, p->len)) {
+			continue;
 		}
 		index = ovl_lookup_upper_unlocked(ofs, p->name, indexdir, p->len);
 		if (IS_ERR(index)) {
Signature · 1 line
2.52.0
maintainer
#5 lore raw

Re: [PATCH 3/3] ovl: use name_is_dot* helpers in readdir code

Quoted 3 lines · Amir Goldstein
fs/overlayfs/readdir.c +2 -0
+		if (name_is_dot_dotdot(p->name, p->len)) {
+			goto del_entry;
 		}

[...]

fs/overlayfs/readdir.c int ovl_indexdir_cleanup(struct ovl_fs *ofs) · line 1276

Quoted 3 lines
fs/overlayfs/readdir.c +2 -0
+		if (name_is_dot_dotdot(p->name, p->len)) {
+			continue;
 		}

Should drop the braces in the above two places, as noted by checkpatch.

- Eric

#6 lore raw

Re: [PATCH 3/3] ovl: use name_is_dot* helpers in readdir code

Quoted 10 lines · Eric Biggers
Quoted 3 lines · Amir Goldstein
fs/overlayfs/readdir.c +2 -0
+		if (name_is_dot_dotdot(p->name, p->len)) {
+			goto del_entry;
 		}

[...]

fs/overlayfs/readdir.c int ovl_indexdir_cleanup(struct ovl_fs *ofs) · line 1276

Quoted 3 lines
fs/overlayfs/readdir.c +2 -0
+		if (name_is_dot_dotdot(p->name, p->len)) {
+			continue;
 		}

Should drop the braces in the above two places, as noted by checkpatch.

I'll do that when I apply.

maintainer Reviewed-by on the v1 series
#7 lore raw
In reply to Amir Goldstein
Quoted 22 lines · Amir Goldstein

Miklos,

Following the syzbot ovl bug report and a fix by Qing Wang, I decided to follow up with a small vfs cleanup of some open coded version of checking "." and ".." name in readdir.

The fix patch is applied at the start of this cleanup series to allow for easy backporting, but it is not an urgent fix so I don't think there is a need to fast track it.

Christian,

I am assuming that you would want to take the vfs cleanup via your tree, so might as well take the ovl adjacent patches with it.

If you want me to drive this entire series via ovl tree, please ack the vfs cleanup patch.

Thanks,
Amir.

Quoted 8 lines

[1] https://lore.kernel.org/linux-unionfs/[email protected]/

Amir Goldstein (2):
fs: add helpers name_is_dot{,dot,_dotdot}
ovl: use name_is_dot* helpers in readdir code

Qing Wang (1):
ovl: Fix uninit-value in ovl_fill_real

- Eric

maintainer Applied
#8 lore raw
In reply to Amir Goldstein
Quoted 11 lines · Amir Goldstein

Miklos,

Following the syzbot ovl bug report and a fix by Qing Wang, I decided to follow up with a small vfs cleanup of some open coded version of checking "." and ".." name in readdir.

The fix patch is applied at the start of this cleanup series to allow for easy backporting, but it is not an urgent fix so I don't think there is a need to fast track it.

[...]

Applied to the vfs-7.0.misc branch of the vfs/vfs.git tree. Patches in the vfs-7.0.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-7.0.misc

[1/3] ovl: Fix uninit-value in ovl_fill_real

      https://git.kernel.org/vfs/vfs/c/1992330d90dd
[2/3] fs: add helpers name_is_dot{,dot,_dotdot}
      https://git.kernel.org/vfs/vfs/c/55fb177d3a03
[3/3] ovl: use name_is_dot* helpers in readdir code
      https://git.kernel.org/vfs/vfs/c/9cf8ddb12a72