Hello Kees,
+-- On Wed, 24 Oct 2012, Kees Cook wrote --+
| What should the code here _actually_ be doing? The _script and _misc
| handlers expect to rewrite the bprm contents and recurse, but the module
| loader want to try again. It's not clear to me what the binfmt module
| handler is even there for; I don't see any binfmt-XXXX aliases in the tree.
| If nothing uses it, should we just rip it out? That would solve it too.
I've been following this issue and updated versions of HDs patch. Below is a
small patch to search_binary_handler() routine, which attempts to make the
request_module call before calling load_script routine.
Besides fixing the stack disclosure issue it also helps to *simplify* the
search_binary_handler routine by removing the -for (try=0;try<2;try++)- loop.
I'd really appreciate any comments/suggestions you may have.
===
diff --git a/fs/exec.c b/fs/exec.c
index 8b9011b..e368f41 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1369,65 +1369,55 @@ int search_binary_handler(struct linux_binprm
*bprm,struct pt_regs *regs)
old_vpid = task_pid_nr_ns(current, task_active_pid_ns(current->parent));
rcu_read_unlock();
- retval = -ENOENT;
- for (try=0; try<2; try++) {
- read_lock(&binfmt_lock);
- list_for_each_entry(fmt, &formats, lh) {
- int (*fn)(struct linux_binprm *, struct pt_regs *) =
fmt->load_binary;
- if (!fn)
- continue;
- if (!try_module_get(fmt->module))
- continue;
- read_unlock(&binfmt_lock);
- retval = fn(bprm, regs);
- /*
- * Restore the depth counter to its starting value
- * in this call, so we don't have to rely on every
- * load_binary function to restore it on return.
- */
- bprm->recursion_depth = depth;
- if (retval >= 0) {
- if (depth == 0) {
- trace_sched_process_exec(current,
old_pid, bprm);
- ptrace_event(PTRACE_EVENT_EXEC,
old_vpid);
- }
- put_binfmt(fmt);
- allow_write_access(bprm->file);
- if (bprm->file)
- fput(bprm->file);
- bprm->file = NULL;
- current->did_exec = 1;
- proc_exec_connector(current);
- return retval;
- }
- read_lock(&binfmt_lock);
- put_binfmt(fmt);
- if (retval != -ENOEXEC || bprm->mm == NULL)
- break;
- if (!bprm->file) {
- read_unlock(&binfmt_lock);
- return retval;
- }
- }
- read_unlock(&binfmt_lock);
#ifdef CONFIG_MODULES
- if (retval != -ENOEXEC || bprm->mm == NULL) {
- break;
- } else {
-#define printable(c) (((c)=='\t') || ((c)=='\n') || (0x20<=(c) && (c)<=0x7e))
- if (printable(bprm->buf[0]) &&
- printable(bprm->buf[1]) &&
- printable(bprm->buf[2]) &&
- printable(bprm->buf[3]))
- break; /* -ENOEXEC */
- if (try)
- break; /* -ENOEXEC */
- request_module("binfmt-%04x", *(unsigned short
*)(&bprm->buf[2]));
- }
-#else
- break;
+#define printable(c) (0x20<=(c) && (c)<=0x7e)
+
+ if (printable(bprm->buf[0]) && printable(bprm->buf[1])
+ && printable(bprm->buf[2]) && printable(bprm->buf[3]))
+ request_module("binfmt-%04x", *(unsigned short *)(&bprm->buf[2]));
#endif
- }
+
+ retval = -ENOENT;
+ read_lock(&binfmt_lock);
+ list_for_each_entry(fmt, &formats, lh) {
+ int (*fn)(struct linux_binprm *, struct pt_regs *) = fmt->load_binary;
+ if (!fn)
+ continue;
+ if (!try_module_get(fmt->module))
+ continue;
+ read_unlock(&binfmt_lock);
+ retval = fn(bprm, regs);
+ /*
+ * Restore the depth counter to its starting value
+ * in this call, so we don't have to rely on every
+ * load_binary function to restore it on return.
+ */
+ bprm->recursion_depth = depth;
+ if (retval >= 0) {
+ if (depth == 0) {
+ trace_sched_process_exec(current, old_pid, bprm);
+ ptrace_event(PTRACE_EVENT_EXEC, old_vpid);
+ }
+ put_binfmt(fmt);
+ allow_write_access(bprm->file);
+ if (bprm->file)
+ fput(bprm->file);
+ bprm->file = NULL;
+ current->did_exec = 1;
+ proc_exec_connector(current);
+ return retval;
+ }
+ read_lock(&binfmt_lock);
+ put_binfmt(fmt);
+ if (retval != -ENOEXEC || bprm->mm == NULL)
+ break;
+ if (!bprm->file) {
+ read_unlock(&binfmt_lock);
+ return retval;
+ }
+ }
+ read_unlock(&binfmt_lock);
+
return retval;
}
===
Thank you.
--
Prasad J Pandit / Red Hat Security Response Team
DB7A 84C5 D3F9 7CD1 B5EB C939 D048 7860 3655 602B
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
