name_is_dot* cleanup
Subsystems (7)
Amir Goldstein
maintainercover 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
Amir Goldstein
maintainerPATCH 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.
--- fs/overlayfs/readdir.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/overlayfs/readdir.c +1 -1
@@ -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
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()usesstrcmp()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
Amir Goldstein
maintainerPATCH 2/3 fs: add helpers name_is_dot{,dot,_dotdot}
In reply to Amir GoldsteinRename 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
Amir Goldstein
maintainerPATCH 3/3 ovl: use name_is_dot* helpers in readdir code
In reply to Amir GoldsteinUse 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
@@ -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 *palist_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
Eric Biggers
maintainerRe: [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
Christian Brauner
maintainerRe: [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.
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 codeQing Wang (1):
ovl: Fix uninit-value inovl_fill_real
- Eric
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