fix setuid sometimes wouldn't

check_unsafe_exec() also notes whether the fs_struct is being shared by more threads than will get killed by the exec, and if so sets LSM_UNSAFE_SHARE to make bprm_set_creds() careful about euid. But /proc/<pid>/cwd and /proc/<pid>/root lookups make transient use of get_fs_struct(), which also raises that sharing count. This might occasionally cause a setuid program not to change euid, in the same way as happened with files->count (check_unsafe_exec also looks at sighand->count, but /proc doesn't raise that one). We'd prefer exec not to unshare fs_struct: so fix this in procfs, replacing get_fs_struct() by get_fs_path(), which does path_get while still holding task_lock, instead of raising fs->count. Signed-off-by: Hugh Dickins <hugh@veritas.com> Cc: stable@kernel.org ___ fs/proc/base.c | 50 +++++++++++++++-------------------------------- 1 file changed, 16 insertions(+), 34 deletions(-) Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

fix setuid sometimes wouldn't
check_unsafe_exec() also notes whether the fs_struct is being shared by more threads than will get killed by the exec, and if so sets LSM_UNSAFE_SHARE to make bprm_set_creds() careful about euid. But /proc/<pid>/cwd and /proc/<pid>/root lookups make transient use of get_fs_struct(), which also raises that sharing count. This might occasionally cause a setuid program not to change euid, in the same way as happened with files->count (check_unsafe_exec also looks at sighand->count, but /proc doesn't raise that one). We'd prefer exec not to unshare fs_struct: so fix this in procfs, replacing get_fs_struct() by get_fs_path(), which does path_get while still holding task_lock, instead of raising fs->count. Signed-off-by: Hugh Dickins <hugh@veritas.com> Cc: stable@kernel.org ___ fs/proc/base.c | 50 +++++++++++++++-------------------------------- 1 file changed, 16 insertions(+), 34 deletions(-) Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Hugh Dickins · Linus Torvalds
1 parent e426b64c41
Showing 1 changed file with 16 additions and 34 deletions Side-by-side Diff
fs/proc/base.c
@@ -146,15 +146,22 @@
 	return count;
 }
  
-static struct fs_struct *get_fs_struct(struct task_struct *task)
+static int get_fs_path(struct task_struct *task, struct path *path, bool root)
 {
 	struct fs_struct *fs;
+	int result = -ENOENT;
+
 	task_lock(task);
 	fs = task->fs;
-	if(fs)
-		atomic_inc(&fs->count);
+	if (fs) {
+		read_lock(&fs->lock);
+		*path = root ? fs->root : fs->pwd;
+		path_get(path);
+		read_unlock(&fs->lock);
+		result = 0;
+	}
 	task_unlock(task);
-	return fs;
+	return result;
 }
  
 static int get_nr_threads(struct task_struct *tsk)
  
  
  
  
  
@@ -172,42 +179,24 @@
 static int proc_cwd_link(struct inode *inode, struct path *path)
 {
 	struct task_struct *task = get_proc_task(inode);
-	struct fs_struct *fs = NULL;
 	int result = -ENOENT;
  
 	if (task) {
-		fs = get_fs_struct(task);
+		result = get_fs_path(task, path, 0);
 		put_task_struct(task);
 	}
-	if (fs) {
-		read_lock(&fs->lock);
-		*path = fs->pwd;
-		path_get(&fs->pwd);
-		read_unlock(&fs->lock);
-		result = 0;
-		put_fs_struct(fs);
-	}
 	return result;
 }
  
 static int proc_root_link(struct inode *inode, struct path *path)
 {
 	struct task_struct *task = get_proc_task(inode);
-	struct fs_struct *fs = NULL;
 	int result = -ENOENT;
  
 	if (task) {
-		fs = get_fs_struct(task);
+		result = get_fs_path(task, path, 1);
 		put_task_struct(task);
 	}
-	if (fs) {
-		read_lock(&fs->lock);
-		*path = fs->root;
-		path_get(&fs->root);
-		read_unlock(&fs->lock);
-		result = 0;
-		put_fs_struct(fs);
-	}
 	return result;
 }
  
@@ -596,7 +585,6 @@
 	struct task_struct *task = get_proc_task(inode);
 	struct nsproxy *nsp;
 	struct mnt_namespace *ns = NULL;
-	struct fs_struct *fs = NULL;
 	struct path root;
 	struct proc_mounts *p;
 	int ret = -EINVAL;
  
  
@@ -610,21 +598,15 @@
 				get_mnt_ns(ns);
 		}
 		rcu_read_unlock();
-		if (ns)
-			fs = get_fs_struct(task);
+		if (ns && get_fs_path(task, &root, 1) == 0)
+			ret = 0;
 		put_task_struct(task);
 	}
  
 	if (!ns)
 		goto err;
-	if (!fs)
+	if (ret)
 		goto err_put_ns;
-
-	read_lock(&fs->lock);
-	root = fs->root;
-	path_get(&root);
-	read_unlock(&fs->lock);
-	put_fs_struct(fs);
  
 	ret = -ENOMEM;
 	p = kmalloc(sizeof(struct proc_mounts), GFP_KERNEL);
...	...	@@ -146,15 +146,22 @@
146	146	return count;
147	147	}
148	148
149		-static struct fs_struct get_fs_struct(struct task_struct task)
	149	+static int get_fs_path(struct task_struct task, struct path path, bool root)
150	150	{
151	151	struct fs_struct *fs;
	152	+ int result = -ENOENT;
	153	+
152	154	task_lock(task);
153	155	fs = task->fs;
154		- if(fs)
155		- atomic_inc(&fs->count);
	156	+ if (fs) {
	157	+ read_lock(&fs->lock);
	158	+ *path = root ? fs->root : fs->pwd;
	159	+ path_get(path);
	160	+ read_unlock(&fs->lock);
	161	+ result = 0;
	162	+ }
156	163	task_unlock(task);
157		- return fs;
	164	+ return result;
158	165	}
159	166
160	167	static int get_nr_threads(struct task_struct *tsk)
161	168
162	169
163	170
164	171
165	172
...	...	@@ -172,42 +179,24 @@
172	179	static int proc_cwd_link(struct inode inode, struct path path)
173	180	{
174	181	struct task_struct *task = get_proc_task(inode);
175		- struct fs_struct *fs = NULL;
176	182	int result = -ENOENT;
177	183
178	184	if (task) {
179		- fs = get_fs_struct(task);
	185	+ result = get_fs_path(task, path, 0);
180	186	put_task_struct(task);
181	187	}
182		- if (fs) {
183		- read_lock(&fs->lock);
184		- *path = fs->pwd;
185		- path_get(&fs->pwd);
186		- read_unlock(&fs->lock);
187		- result = 0;
188		- put_fs_struct(fs);
189		- }
190	188	return result;
191	189	}
192	190
193	191	static int proc_root_link(struct inode inode, struct path path)
194	192	{
195	193	struct task_struct *task = get_proc_task(inode);
196		- struct fs_struct *fs = NULL;
197	194	int result = -ENOENT;
198	195
199	196	if (task) {
200		- fs = get_fs_struct(task);
	197	+ result = get_fs_path(task, path, 1);
201	198	put_task_struct(task);
202	199	}
203		- if (fs) {
204		- read_lock(&fs->lock);
205		- *path = fs->root;
206		- path_get(&fs->root);
207		- read_unlock(&fs->lock);
208		- result = 0;
209		- put_fs_struct(fs);
210		- }
211	200	return result;
212	201	}
213	202
...	...	@@ -596,7 +585,6 @@
596	585	struct task_struct *task = get_proc_task(inode);
597	586	struct nsproxy *nsp;
598	587	struct mnt_namespace *ns = NULL;
599		- struct fs_struct *fs = NULL;
600	588	struct path root;
601	589	struct proc_mounts *p;
602	590	int ret = -EINVAL;
603	591
604	592
...	...	@@ -610,21 +598,15 @@
610	598	get_mnt_ns(ns);
611	599	}
612	600	rcu_read_unlock();
613		- if (ns)
614		- fs = get_fs_struct(task);
	601	+ if (ns && get_fs_path(task, &root, 1) == 0)
	602	+ ret = 0;
615	603	put_task_struct(task);
616	604	}
617	605
618	606	if (!ns)
619	607	goto err;
620		- if (!fs)
	608	+ if (ret)
621	609	goto err_put_ns;
622		-
623		- read_lock(&fs->lock);
624		- root = fs->root;
625		- path_get(&root);
626		- read_unlock(&fs->lock);
627		- put_fs_struct(fs);
628	610
629	611	ret = -ENOMEM;
630	612	p = kmalloc(sizeof(struct proc_mounts), GFP_KERNEL);