UBIFS: prepare to fix a horrid bug
Subsystems (3)
PATCH 1/2 UBIFS: prepare to fix a horrid bug
From: Artem Bityutskiy <[email protected]>
Al Viro pointed me to the fact that '->readdir()' and '->llseek()' have no mutual exclusion, which means the 'ubifs_dir_llseek()' can be run while we are in the middle of 'ubifs_readdir()'.
First of all, this means that 'file->private_data' can be freed while 'ubifs_readdir()' uses it. But this particular patch does not fix the problem. This patch is only a preparation, and the fix will follow next.
In this patch we make 'ubifs_readdir()' stop using 'file->f_pos' directly, because 'file->f_pos' can be changed by '->llseek()' at any point. This may lead 'ubifs_readdir()' to returning inconsistent data: directory entry names may correspond to incorrect file positions.
So here we introduce a local variable 'pos', read 'file->f_pose' once at very the beginning, and then stick to 'pos'. The result of this is that when 'ubifs_dir_llseek()' changes 'file->f_pos' while we are in the middle of 'ubifs_readdir()', the latter "wins".
--- fs/ubifs/dir.c | 24 ++++++++++++------------ 1 files changed, 12 insertions(+), 12 deletions(-)
fs/ubifs/dir.c +12 -12
@@ -349,15 +349,16 @@ static unsigned int vfs_dent_type(uint8_t type)static int ubifs_readdir (struct file *file,void *dirent,filldir_t filldir) {int err, over =0 ; +loff_t pos = file->f_pos;struct qstr nm;union ubifs_key key;struct ubifs_dent_node *dent;struct inode *dir =file_inode (file);struct ubifs_info *c = dir->i_sb->s_fs_info; -dbg_gen ("dir ino %lu, f_pos %#llx" , dir->i_ino, file->f_pos); +dbg_gen ("dir ino %lu, f_pos %#llx" , dir->i_ino, pos); -if (file->f_pos >UBIFS_S_KEY_HASH_MASK || file->f_pos ==2 ) +if (pos >UBIFS_S_KEY_HASH_MASK || pos ==2 )/* * The directory was seek'ed to a senseless position or there * are no more entries.
@@ -365,15 +366,15 @@ static int ubifs_readdir(struct file *file, void *dirent, filldir_t filldir)return 0 ;/* File positions 0 and 1 correspond to "." and ".." */ -if (file->f_pos ==0 ) { +if (pos ==0 ) {ubifs_assert (!file->private_data); over =filldir (dirent,"." ,1 ,0 , dir->i_ino,DT_DIR );if (over)return 0 ; - file->f_pos =1 ; + file->f_pos = pos =1 ; } -if (file->f_pos ==1 ) { +if (pos ==1 ) {ubifs_assert (!file->private_data); over =filldir (dirent,".." ,2 ,1 ,parent_ino (file->f_path.dentry),DT_DIR );
@@ -389,7 +390,7 @@ static int ubifs_readdir(struct file *file, void *dirent, filldir_t filldir) goto out; } - file->f_pos =key_hash_flash (c, &dent->key); + file->f_pos = pos =key_hash_flash (c, &dent->key); file->private_data = dent; }
@@ -397,17 +398,16 @@ static int ubifs_readdir(struct file *file, void *dirent, filldir_t filldir)if (!dent) {/* * The directory was seek'ed to and is now readdir'ed. -* Find the entry corresponding to @file->f_pos or the -* closest one. +* Find the entry corresponding to @pos or the closest one. */ -dent_key_init_hash (c, &key, dir->i_ino, file->f_pos); +dent_key_init_hash (c, &key, dir->i_ino, pos); nm.name =NULL ; dent =ubifs_tnc_next_ent (c, &key, &nm);if (IS_ERR (dent)) { err =PTR_ERR (dent); goto out; } - file->f_pos =key_hash_flash (c, &dent->key); + file->f_pos = pos =key_hash_flash (c, &dent->key); file->private_data = dent; }
@@ -419,7 +419,7 @@ static int ubifs_readdir(struct file *file, void *dirent, filldir_t filldir) ubifs_inode(dir)->creat_sqnum); nm.len =le16_to_cpu (dent->nlen); - over =filldir (dirent, dent->name, nm.len, file->f_pos, + over =filldir (dirent, dent->name, nm.len, pos,le64_to_cpu (dent->inum),vfs_dent_type (dent->type));if (over)
@@ -435,7 +435,7 @@ static int ubifs_readdir(struct file *file, void *dirent, filldir_t filldir) }kfree (file->private_data); - file->f_pos =key_hash_flash (c, &dent->key); + file->f_pos = pos =key_hash_flash (c, &dent->key); file->private_data = dent;cond_resched (); }
Signature · 1 line
1.7.7.6
PATCH 2/2 UBIFS: fix a horrid bug
From: Artem Bityutskiy <[email protected]>
Al Viro pointed me to the fact that '->readdir()' and '->llseek()' have no mutual exclusion, which means the 'ubifs_dir_llseek()' can be run while we are in the middle of 'ubifs_readdir()'.
This means that 'file->private_data' can be freed while 'ubifs_readdir()' uses it, and this is a very bad bug: not only 'ubifs_readdir()' can return garbage, but this may corrupt memory and lead to all kinds of problems like crashes an security holes.
This patch fixes the problem by using the 'file->f_version' field, which '->llseek()' always unconditionally sets to zero. We set it to 1 in 'ubifs_readdir()' and whenever we detect that it became 0, we know there was a seek and it is time to clear the state saved in 'file->private_data'.
I tested this patch by writing a user-space program which runds readdir and seek in parallell. I could easily crash the kernel without these patches, but could not crash it with these patches.
--- fs/ubifs/dir.c | 30 +++++++++++++++++++++++++++--- 1 files changed, 27 insertions(+), 3 deletions(-)
fs/ubifs/dir.c +27 -3
@@ -365,6 +365,24 @@ static int ubifs_readdir(struct file *file, void *dirent, filldir_t filldir)*/ return 0 ; +if (file->f_version ==0 ) { +/* +* The file was seek'ed, which means that @file->private_data +* is now invalid. This may also be just the first +* 'ubifs_readdir()' invocation, in which case +* @file->private_data is NULL, and the below code is +* basically a no-op. +*/ +kfree (file->private_data); + file->private_data =NULL ; + } + +/* +* 'generic_file_llseek()' unconditionally sets @file->f_version to +* zero, and we use this for detecting whether the file was seek'ed. +*/ + file->f_version =1 ; +/* File positions 0 and 1 correspond to "." and ".." */ if (pos ==0 ) {ubifs_assert (!file->private_data);
@@ -438,6 +456,14 @@ static int ubifs_readdir(struct file *file, void *dirent, filldir_t filldir) file->f_pos = pos =key_hash_flash (c, &dent->key); file->private_data = dent;cond_resched (); + +if (file->f_version ==0 ) +/* +* The file was seek'ed meanwhile, lets return and start +* reading direntries from the new position on the next +* invocation. +*/ +return 0 ; } out:
@@ -448,15 +474,13 @@ out:kfree (file->private_data); file->private_data =NULL ; +/* 2 is a special value indicating that there are no more direntries */ file->f_pos =2 ;return 0 ; } -/* If a directory is seeked, we have to free saved readdir() state */ static loff_t ubifs_dir_llseek (struct file *file,loff_t offset,int whence) { -kfree (file->private_data); - file->private_data =NULL ;return generic_file_llseek (file, offset, whence); }
Signature · 1 line
1.7.7.6
"linux-mtd" <[email protected]> wrote on 2013/06/28 13:15:14:
Quoted 4 lines
From: Artem Bityutskiy <[email protected]>
Al Viro pointed me to the fact that '->readdir()' and '->llseek()' have
no
Quoted 1 line
mutual exclusion, which means the '
ubifs_dir_llseek()' can be run while
we are
Quoted 4 lines
in the middle of '
ubifs_readdir()'.First of all, this means that '
file->private_data' can be freed while 'ubifs_readdir()' uses it. But this particular patch does not fix the
problem.
Quoted 3 lines
This patch is only a preparation, and the fix will follow next.
In this patch we make '
ubifs_readdir()' stop using 'file->f_pos'
directly,
Quoted 1 line
because '
file->f_pos' can be changed by '->llseek()' at any point. This
may
Quoted 1 line
lead '
ubifs_readdir()' to returning inconsistent data: directory entry
names
Quoted 3 lines
may correspond to incorrect file positions.
So here we introduce a local variable 'pos', read '
file->f_pose' once at
very
Quoted 3 lines
the beginning, and then stick to 'pos'. The result of this is that when '
ubifs_dir_llseek()' changes 'file->f_pos' while we are in the middle of 'ubifs_readdir()', the latter "wins".
Ouch, I hope JFFS2 doesn't have the same bug?
Jcoe
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
Al Viro
maintainerQuoted 7 lines · Joakim Tjernlund
Quoted 1 line
So here we introduce a local variable 'pos', read '
file->f_pose' once atvery
Quoted 3 lines
the beginning, and then stick to 'pos'. The result of this is that when '
ubifs_dir_llseek()' changes 'file->f_pos' while we are in the middle of 'ubifs_readdir()', the latter "wins".Ouch, I hope JFFS2 doesn't have the same bug?
FWIW, this class of bugs (f_pos races, *not* kfree-under-us) is dealt with by switch to saner API - see commits in linux-next marked [readdir] <something>