Re: [PATCH] exec: do not leave bprm->interp on stack
From: P J P
Date: Fri Oct 26 2012 - 13:38:39 EST
+-- On Thu, 25 Oct 2012, Al Viro wrote --+
| * every bleeding script will have bogus execution of modprobe done
| at execve time (and you'd better pray that /sbin/modprobe isn't a shell
| script wrapper around the actual binary, or you *will* get loop prevention
| kick in)
| * none of the existing binfmt-<...> aliases is going to be hit
| now; IOW, all usecases got broken. Granted, realistically it just means
| broken modular aout support, but then it's the only reason to have that
| request_module() there in the first place.
Please have a look at the updated patch below.
It fixes the issue of excessive calls to request_module. find_module() routine
is used before request_module(), to see if the module is already loaded or
not. Module alias could dodge this though, I guess.
In this, request_module is called only when at least 1 of the first 4 bytes is
NOT printable, as is the present upstream case. It avoids calling
request_module for regular ELFs because printable() macro now includes the
last - DEL(0x7f) - character as well.
#define printable(c) (((c)=='\t') || ((c)=='\n') || (0x20<=(c) && (c)<=0x7f))
I am currently running this patch on my machine and logs only show messages
for the scripts generated by ./DoTest.sh tool.
$ grep -ri 'request_module' /var/log/messages
...
Oct 26 21:01:30 javelin kernel: [ 154.155980] request_module: failed to load: binfmt-660d
Oct 26 21:01:30 javelin kernel: [ 154.158161] request_module: failed to load: binfmt-660d
Oct 26 21:37:14 javelin kernel: [ 2293.030330] request_module: failed to load: binfmt-660d
Oct 26 21:40:07 javelin kernel: [ 2465.829154] request_module: failed to load: binfmt-660d
I'd appreciate any comments/suggestions that you may have.
===
diff --git a/fs/exec.c b/fs/exec.c
index 8b9011b..7615812 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1369,65 +1369,69 @@ 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) (((c)=='\t') || ((c)=='\n') || (0x20<=(c) && (c)<=0x7f))
+
+ if (!printable(bprm->buf[0]) || !printable(bprm->buf[1])
+ || !printable(bprm->buf[2]) || !printable(bprm->buf[3]))
+ {
+ char name[12] = "\0";
+ struct module *mod = NULL;
+ unsigned short *usp = (unsigned short *)(&bprm->buf[2]);
+
+ snprintf(name, sizeof(name), "binfmt-%04x", *usp);
+ mod = find_module(name);
+
+ /* request_module is called if and only if - `name' module is NOT
+ * loaded and at least 1 of the first 4 bytes is NOT printable.
+ */
+ if (!mod && request_module(name))
+ printk(KERN_WARNING "request_module: failed to load: %s\n", name);
+ }
+
#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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/