Re: [PATCH] exec: do not leave bprm->interp on stack

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



+-- 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-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux