Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965540Ab2JZRiv (ORCPT ); Fri, 26 Oct 2012 13:38:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44589 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758911Ab2JZRiu (ORCPT ); Fri, 26 Oct 2012 13:38:50 -0400 Date: Fri, 26 Oct 2012 23:08:20 +0530 (IST) From: P J P X-X-Sender: pjp@javelin.pnq.redhat.com To: Al Viro cc: Kees Cook , linux-kernel@vger.kernel.org, Andrew Morton , Josh Triplett , Serge Hallyn , linux-fsdevel@vger.kernel.org, halfdog Subject: Re: [PATCH] exec: do not leave bprm->interp on stack In-Reply-To: <20121025123843.GJ2616@ZenIV.linux.org.uk> Message-ID: References: <20121024232032.GA31129@www.outflux.net> <20121025041620.GH2616@ZenIV.linux.org.uk> <20121025120952.GI2616@ZenIV.linux.org.uk> <20121025123843.GJ2616@ZenIV.linux.org.uk> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6169 Lines: 179 +-- 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/