exec: Remove recursion from search_binary_handler

Recursion in kernel code is generally a bad idea as it can overflow the kernel stack. Recursion in exec also hides that the code is looping and that the loop changes bprm->file. Instead of recursing in search_binary_handler have the methods that would recurse set bprm->interpreter and return 0. Modify exec_binprm to loop when bprm->interpreter is set. Consolidate all of the reassignments of bprm->file in that loop to make it clear what is going on. The structure of the new loop in exec_binprm is that all errors return immediately, while successful completion (ret == 0 && !bprm->interpreter) just breaks out of the loop and runs what exec_bprm has always run upon successful completion. Fail if the an interpreter is being call after execfd has been set. The code has never properly handled an interpreter being called with execfd being set and with reassignments of bprm->file and the assignment of bprm->executable in generic code it has finally become possible to test and fail when if this problematic condition happens. With the reassignments of bprm->file and the assignment of bprm->executable moved into the generic code add a test to see if bprm->executable is being reassigned. In search_binary_handler remove the test for !bprm->file. With all reassignments of bprm->file moved to exec_binprm bprm->file can never be NULL in search_binary_handler.

Eric W. Biederman committed 6 years ago

Commit bc2bf331 parent b8a61c96 files · +43 −55Browse files

Unified · Split

6 files

arch/alpha/kernel/binfmt_loader.c +2 −6
fs/binfmt_em86.c +2 −7
fs/binfmt_misc.c +3 −15
fs/binfmt_script.c +2 −7
fs/exec.c +33 −18
include/linux/binfmts.h +1 −2

arch/alpha/kernel/binfmt_loader.c+2−6

@@ -19,10 +19,6 @@ static int load_binary(struct linux_binprm *bprm) · 18 unmodified lines
19	19	if (bprm->loader)
20	20	return -ENOEXEC;
21	21
22		− allow_write_access(bprm->file);
23		− fput(bprm->file);
24		− bprm->file = NULL;
25		−
26	22	loader = bprm->vma->vm_end - sizeof(void *);
27	23
28	24	file = open_exec("/sbin/loader");
@@ -33,9 +29,9 @@ static int load_binary(struct linux_binprm *bprm) · 4 unmodified lines
33	29	/* Remember if the application is TASO. */
34	30	bprm->taso = eh->ah.entry < 0x100000000UL;
35	31
36		− bprm->file = file;
	32	+ bprm->interpreter = file;
37	33	bprm->loader = loader;
38		− return search_binary_handler(bprm);
	34	+ return 0;
39	35	}
40	36
41	37	static struct linux_binfmt loader_format = {

fs/binfmt_em86.c+2−7

@@ -48,10 +48,6 @@ static int load_em86(struct linux_binprm *bprm) · 47 unmodified lines
48	48	if (bprm->interp_flags & BINPRM_FLAGS_PATH_INACCESSIBLE)
49	49	return -ENOENT;
50	50
51		− allow_write_access(bprm->file);
52		− fput(bprm->file);
53		− bprm->file = NULL;
54		−
55	51	/* Unlike in the script case, we don't have to do any hairy
56	52	* parsing to find our interpreter... it's hardcoded!
57	53	*/
@@ -89,9 +85,8 @@ static int load_em86(struct linux_binprm *bprm) · 31 unmodified lines
89	85	if (IS_ERR(file))
90	86	return PTR_ERR(file);
91	87
92		− bprm->file = file;
93		−
94		− return search_binary_handler(bprm);
	88	+ bprm->interpreter = file;
	89	+ return 0;
95	90	}
96	91
97	92	static struct linux_binfmt em86_format = {

fs/binfmt_misc.c+3−15

@@ -159,18 +159,9 @@ static int load_misc_binary(struct linux_binprm *bprm) · 158 unmodified lines
159	159	goto ret;
160	160	}
161	161
162		− if (fmt->flags & MISC_FMT_OPEN_BINARY) {
163		− /* Pass the open binary to the interpreter */
	162	+ if (fmt->flags & MISC_FMT_OPEN_BINARY)
164	163	bprm->have_execfd = 1;
165		− bprm->executable = bprm->file;
166	164
167		− allow_write_access(bprm->file);
168		− bprm->file = NULL;
169		− } else {
170		− allow_write_access(bprm->file);
171		− fput(bprm->file);
172		− bprm->file = NULL;
173		− }
174	165	/* make argv[1] be the path to the binary */
175	166	retval = copy_strings_kernel(1, &bprm->interp, bprm);
176	167	if (retval < 0)
@@ -199,14 +190,11 @@ static int load_misc_binary(struct linux_binprm *bprm) · 22 unmodified lines
199	190	if (IS_ERR(interp_file))
200	191	goto ret;
201	192
202		− bprm->file = interp_file;
	193	+ bprm->interpreter = interp_file;
203	194	if (fmt->flags & MISC_FMT_CREDENTIALS)
204	195	bprm->preserve_creds = 1;
205	196
206		− retval = search_binary_handler(bprm);
207		− if (retval < 0)
208		− goto ret;
209		−
	197	+ retval = 0;
210	198	ret:
211	199	dput(fmt->dentry);
212	200	return retval;

fs/binfmt_script.c+2−7

@@ -93,11 +93,6 @@ static int load_script(struct linux_binprm *bprm) · 92 unmodified lines
93	93	if (bprm->interp_flags & BINPRM_FLAGS_PATH_INACCESSIBLE)
94	94	return -ENOENT;
95	95
96		− /* Release since we are not mapping a binary into memory. */
97		− allow_write_access(bprm->file);
98		− fput(bprm->file);
99		− bprm->file = NULL;
100		−
101	96	/*
102	97	* OK, we've parsed out the interpreter name and
103	98	* (optional) argument.
@@ -138,8 +133,8 @@ static int load_script(struct linux_binprm *bprm) · 34 unmodified lines
138	133	if (IS_ERR(file))
139	134	return PTR_ERR(file);
140	135
141		− bprm->file = file;
142		− return search_binary_handler(bprm);
	136	+ bprm->interpreter = file;
	137	+ return 0;
143	138	}
144	139
145	140	static struct linux_binfmt script_format = {

fs/exec.c+33−18

@@ -1710,16 +1710,12 @@ EXPORT_SYMBOL(remove_arg_zero); · 1709 unmodified lines
1710	1710	/*
1711	1711	* cycle the list of binary formats handler, until one recognizes the image
1712	1712	*/
1713		−int search_binary_handler(struct linux_binprm *bprm)
	1713	+static int search_binary_handler(struct linux_binprm *bprm)
1714	1714	{
1715	1715	bool need_retry = IS_ENABLED(CONFIG_MODULES);
1716	1716	struct linux_binfmt *fmt;
1717	1717	int retval;
1718	1718
1719		− /* This allows 4 levels of binfmt rewrites before failing hard. */
1720		− if (bprm->recursion_depth > 5)
1721		− return -ELOOP;
1722		−
1723	1719	retval = prepare_binprm(bprm);
1724	1720	if (retval < 0)
1725	1721	return retval;
@@ -1736,14 +1732,11 @@ int search_binary_handler(struct linux_binprm *bprm) · 10 unmodified lines
1736	1732	continue;
1737	1733	read_unlock(&binfmt_lock);
1738	1734
1739		− bprm->recursion_depth++;
1740	1735	retval = fmt->load_binary(bprm);
1741		− bprm->recursion_depth--;
1742	1736
1743	1737	read_lock(&binfmt_lock);
1744	1738	put_binfmt(fmt);
1745		− if (bprm->point_of_no_return \|\| !bprm->file \|\|
1746		− (retval != -ENOEXEC)) {
	1739	+ if (bprm->point_of_no_return \|\| (retval != -ENOEXEC)) {
1747	1740	read_unlock(&binfmt_lock);
1748	1741	return retval;
1749	1742	}
@@ -1762,12 +1755,11 @@ int search_binary_handler(struct linux_binprm *bprm) · 12 unmodified lines
1762	1755
1763	1756	return retval;
1764	1757	}
1765		−EXPORT_SYMBOL(search_binary_handler);
1766	1758
1767	1759	static int exec_binprm(struct linux_binprm *bprm)
1768	1760	{
1769	1761	pid_t old_pid, old_vpid;
1770		− int ret;
	1762	+ int ret, depth;
1771	1763
1772	1764	/* Need to fetch pid before load_binary changes it */
1773	1765	old_pid = current->pid;
@@ -1775,15 +1767,38 @@ static int exec_binprm(struct linux_binprm *bprm) · 1 unmodified line
1775	1767	old_vpid = task_pid_nr_ns(current, task_active_pid_ns(current->parent));
1776	1768	rcu_read_unlock();
1777	1769
1778		− ret = search_binary_handler(bprm);
1779		− if (ret >= 0) {
1780		− audit_bprm(bprm);
1781		− trace_sched_process_exec(current, old_pid, bprm);
1782		− ptrace_event(PTRACE_EVENT_EXEC, old_vpid);
1783		− proc_exec_connector(current);
	1770	+ /* This allows 4 levels of binfmt rewrites before failing hard. */
	1771	+ for (depth = 0;; depth++) {
	1772	+ struct file *exec;
	1773	+ if (depth > 5)
	1774	+ return -ELOOP;
	1775	+
	1776	+ ret = search_binary_handler(bprm);
	1777	+ if (ret < 0)
	1778	+ return ret;
	1779	+ if (!bprm->interpreter)
	1780	+ break;
	1781	+
	1782	+ exec = bprm->file;
	1783	+ bprm->file = bprm->interpreter;
	1784	+ bprm->interpreter = NULL;
	1785	+
	1786	+ allow_write_access(exec);
	1787	+ if (unlikely(bprm->have_execfd)) {
	1788	+ if (bprm->executable) {
	1789	+ fput(exec);
	1790	+ return -ENOEXEC;
	1791	+ }
	1792	+ bprm->executable = exec;
	1793	+ } else
	1794	+ fput(exec);
1784	1795	}
1785	1796
1786		− return ret;
	1797	+ audit_bprm(bprm);
	1798	+ trace_sched_process_exec(current, old_pid, bprm);
	1799	+ ptrace_event(PTRACE_EVENT_EXEC, old_vpid);
	1800	+ proc_exec_connector(current);
	1801	+ return 0;
1787	1802	}
1788	1803
1789	1804	/*

include/linux/binfmts.h+1−2

@@ -50,8 +50,8 @@ struct linux_binprm { · 49 unmodified lines
50	50	#ifdef __alpha__
51	51	unsigned int taso:1;
52	52	#endif
53		− unsigned int recursion_depth; /* only for search_binary_handler() */
54	53	struct file * executable; /* Executable to pass to the interpreter */
	54	+ struct file * interpreter;
55	55	struct file * file;
56	56	struct cred cred; / new credentials */
57	57	int unsafe; /* how unsafe this exec is (mask of LSM_UNSAFE_) /
@@ -117,7 +117,6 @@ static inline void insert_binfmt(struct linux_binfmt *fmt) · 59 unmodified lines
117	117	extern void unregister_binfmt(struct linux_binfmt *);
118	118
119	119	extern int __must_check remove_arg_zero(struct linux_binprm *);
120		−extern int search_binary_handler(struct linux_binprm *);
121	120	extern int begin_new_exec(struct linux_binprm * bprm);
122	121	extern void setup_new_exec(struct linux_binprm * bprm);
123	122	extern void finalize_exec(struct linux_binprm *bprm);