Re: [RFC][PATCH] exec: Fix use after free of tracepoint trace_sched_process_exec

From: Linus Torvalds
Date: Tue Feb 04 2014 - 19:57:41 EST

On Tue, Feb 4, 2014 at 3:42 PM, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> New version that moves all the ugliness into a static inline helper
> function.

Ok, that's better, but I really think we should just use "getname()"
and "putname()".

That's what the path that *matters* already does (ie the normal
execve() system call), so let's just make all the random cases do the
same thing.

That requires a "getname_kernel()" to create a "struct filename *"
from a kernel string, but hey, that's simple enough.

NOTE! This means that "bprm->filename" is no longer a string: it's a
"struct filename *", so if you want the string, you do
"filename->name".

This actually cleans the normal paths up - look how "open_exec()" used
to create a dummy

struct filename tmp = { .name = name };

on the stack because do_filp_open() wants a 'struct filename' pointer.
I leave that for the external callers (that use it for the
interpreter), but for the main path that actually just goes away,
because now we have that "struct filename *" natively.

It does add code to the special kernel-execve paths, but moving the
"handle errors from getname()" code into do_execve(), even that is
really trivial, eg:

- return do_execve(init_filename,
+ return do_execve(getname_kernel(init_filename),

NOTE NOTE NOTE. This is untested, but it looks fine. If I missed
something, the compiler should warn about bad types. I didn't my the
bprm_flat changes, for example, because that's a no-MMU-only file. And
I might have missed something that didn't match my grep patterns, for
example.

How does this look?

Linus
arch/parisc/hpux/fs.c | 3 +-
arch/tile/mm/elf.c | 2 +-
fs/binfmt_em86.c | 2 +-
fs/binfmt_flat.c | 15 ++++++----
fs/exec.c | 58 ++++++++++++++++++++-------------------
fs/namei.c | 27 ++++++++++++++++++
include/linux/binfmts.h | 3 +-
include/linux/fs.h | 1 +
include/linux/sched.h | 3 +-
include/trace/events/sched.h | 4 +--
init/main.c | 2 +-
kernel/kmod.c | 2 +-
security/apparmor/domain.c | 2 +-
security/commoncap.c | 6 ++--
security/integrity/ima/ima_main.c | 4 +--
security/tomoyo/domain.c | 2 +-
security/tomoyo/tomoyo.c | 2 +-
17 files changed, 86 insertions(+), 52 deletions(-)

diff --git a/arch/parisc/hpux/fs.c b/arch/parisc/hpux/fs.c
index 88d0962de65a..23ebcd678da3 100644
--- a/arch/parisc/hpux/fs.c
+++ b/arch/parisc/hpux/fs.c
@@ -41,11 +41,10 @@ int hpux_execve(struct pt_regs *regs)
if (IS_ERR(filename))
goto out;

- error = do_execve(filename->name,
+ error = do_execve(filename,
(const char __user *const __user *) regs->gr[25],
(const char __user *const __user *) regs->gr[24]);

- putname(filename);

out:
return error;
diff --git a/arch/tile/mm/elf.c b/arch/tile/mm/elf.c
index 23f044e8a7ab..3ad54a7a927c 100644
--- a/arch/tile/mm/elf.c
+++ b/arch/tile/mm/elf.c
@@ -117,7 +117,7 @@ int arch_setup_additional_pages(struct linux_binprm *bprm,
* whatever was passed as the linux_binprm filename.
*/
if (!notify_exec(mm))
- sim_notify_exec(bprm->filename);
+ sim_notify_exec(bprm->filename->name);

retval = setup_vdso_pages();

diff --git a/fs/binfmt_em86.c b/fs/binfmt_em86.c
index f37b08cea1f7..077a76fb6347 100644
--- a/fs/binfmt_em86.c
+++ b/fs/binfmt_em86.c
@@ -62,7 +62,7 @@ static int load_em86(struct linux_binprm *bprm)
* user environment and arguments are stored.
*/
remove_arg_zero(bprm);
- retval = copy_strings_kernel(1, &bprm->filename, bprm);
+ retval = copy_strings_kernel(1, &bprm->filename->name, bprm);
if (retval < 0) return retval;
bprm->argc++;
if (i_arg) {
diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
index d50bbe59da1e..f1377a5d260b 100644
--- a/fs/binfmt_flat.c
+++ b/fs/binfmt_flat.c
@@ -469,7 +469,7 @@ static int load_flat_file(struct linux_binprm * bprm,
}

if (flags & FLAT_FLAG_KTRACE)
- printk("BINFMT_FLAT: Loading file: %s\n", bprm->filename);
+ printk("BINFMT_FLAT: Loading file: %s\n", bprm->filename->name);

if (rev != FLAT_VERSION && rev != OLD_FLAT_VERSION) {
printk("BINFMT_FLAT: bad flat file version 0x%x (supported "
@@ -686,7 +686,7 @@ static int load_flat_file(struct linux_binprm * bprm,

if (flags & FLAT_FLAG_KTRACE)
printk("%s %s: TEXT=%x-%x DATA=%x-%x BSS=%x-%x\n",
- id ? "Lib" : "Load", bprm->filename,
+ id ? "Lib" : "Load", bprm->filename->name,
(int) start_code, (int) end_code,
(int) datapos,
(int) (datapos + data_len),
@@ -818,11 +818,14 @@ static int load_flat_shared_library(int id, struct lib_info *libs)
sprintf(buf, "/lib/lib%d.so", id);

/* Open the file up */
- bprm.filename = buf;
- bprm.file = open_exec(bprm.filename);
+ bprm.filename = getname_kernel(buf);
+ if (IS_ERR(bprm.filename))
+ return res;
+
+ bprm.file = open_exec(buf);
res = PTR_ERR(bprm.file);
if (IS_ERR(bprm.file))
- return res;
+ goto out_putname;

bprm.cred = prepare_exec_creds();
res = -ENOMEM;
@@ -845,6 +848,8 @@ static int load_flat_shared_library(int id, struct lib_info *libs)
out:
allow_write_access(bprm.file);
fput(bprm.file);
+out_putname:
+ putname(bprm.filename);

return(res);
}
diff --git a/fs/exec.c b/fs/exec.c
index e1529b4c79b1..538be413b26b 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -748,11 +748,10 @@ EXPORT_SYMBOL(setup_arg_pages);

#endif /* CONFIG_MMU */

-struct file *open_exec(const char *name)
+static struct file *do_open_exec(struct filename *name)
{
struct file *file;
int err;
- struct filename tmp = { .name = name };
static const struct open_flags open_exec_flags = {
.open_flag = O_LARGEFILE | O_RDONLY | __FMODE_EXEC,
.acc_mode = MAY_EXEC | MAY_OPEN,
@@ -760,7 +759,7 @@ struct file *open_exec(const char *name)
.lookup_flags = LOOKUP_FOLLOW,
};

- file = do_filp_open(AT_FDCWD, &tmp, &open_exec_flags);
+ file = do_filp_open(AT_FDCWD, name, &open_exec_flags);
if (IS_ERR(file))
goto out;

@@ -784,6 +783,12 @@ exit:
fput(file);
return ERR_PTR(err);
}
+
+struct file *open_exec(const char *name)
+{
+ struct filename tmp = { .name = name };
+ return do_open_exec(&tmp);
+}
EXPORT_SYMBOL(open_exec);

int kernel_read(struct file *file, loff_t offset,
@@ -1074,7 +1079,7 @@ int flush_old_exec(struct linux_binprm * bprm)

set_mm_exe_file(bprm->mm, bprm->file);

- filename_to_taskname(bprm->tcomm, bprm->filename, sizeof(bprm->tcomm));
+ filename_to_taskname(bprm->tcomm, bprm->filename->name, sizeof(bprm->tcomm));
/*
* Release all of the old mmap stuff
*/
@@ -1162,7 +1167,7 @@ int prepare_bprm_creds(struct linux_binprm *bprm)
return -ENOMEM;
}

-void free_bprm(struct linux_binprm *bprm)
+static void free_bprm(struct linux_binprm *bprm)
{
free_arg_pages(bprm);
if (bprm->cred) {
@@ -1174,15 +1179,17 @@ void free_bprm(struct linux_binprm *bprm)
fput(bprm->file);
}
/* If a binfmt changed the interp, free it. */
- if (bprm->interp != bprm->filename)
+ if (bprm->interp != bprm->filename->name)
kfree(bprm->interp);
+ if (bprm->filename)
+ putname(bprm->filename);
kfree(bprm);
}

int bprm_change_interp(char *interp, struct linux_binprm *bprm)
{
/* If a binfmt changed the interp, free it first. */
- if (bprm->interp != bprm->filename)
+ if (bprm->interp != bprm->filename->name)
kfree(bprm->interp);
bprm->interp = kstrdup(interp, GFP_KERNEL);
if (!bprm->interp)
@@ -1432,7 +1439,7 @@ static int exec_binprm(struct linux_binprm *bprm)
/*
* sys_execve() executes a new program.
*/
-static int do_execve_common(const char *filename,
+static int do_execve_common(struct filename *filename,
struct user_arg_ptr argv,
struct user_arg_ptr envp)
{
@@ -1441,6 +1448,9 @@ static int do_execve_common(const char *filename,
struct files_struct *displaced;
int retval;

+ if (IS_ERR(filename))
+ return PTR_ERR(filename);
+
/*
* We move the actual failure in case of RLIMIT_NPROC excess from
* set*uid() to execve() because too many poorly written programs
@@ -1466,6 +1476,9 @@ static int do_execve_common(const char *filename,
if (!bprm)
goto out_files;

+ bprm->filename = filename;
+ bprm->interp = filename->name;
+
retval = prepare_bprm_creds(bprm);
if (retval)
goto out_free;
@@ -1473,7 +1486,7 @@ static int do_execve_common(const char *filename,
check_unsafe_exec(bprm);
current->in_execve = 1;

- file = open_exec(filename);
+ file = do_open_exec(filename);
retval = PTR_ERR(file);
if (IS_ERR(file))
goto out_unmark;
@@ -1481,8 +1494,6 @@ static int do_execve_common(const char *filename,
sched_exec();

bprm->file = file;
- bprm->filename = filename;
- bprm->interp = filename;

retval = bprm_mm_init(bprm);
if (retval)
@@ -1500,7 +1511,7 @@ static int do_execve_common(const char *filename,
if (retval < 0)
goto out;

- retval = copy_strings_kernel(1, &bprm->filename, bprm);
+ retval = copy_strings_kernel(1, &bprm->filename->name, bprm);
if (retval < 0)
goto out;

@@ -1539,15 +1550,18 @@ out_unmark:

out_free:
free_bprm(bprm);
+ filename = NULL;

out_files:
if (displaced)
reset_files_struct(displaced);
out_ret:
+ if (filename)
+ putname(filename);
return retval;
}

-int do_execve(const char *filename,
+int do_execve(struct filename *filename,
const char __user *const __user *__argv,
const char __user *const __user *__envp)
{
@@ -1557,7 +1571,7 @@ int do_execve(const char *filename,
}

#ifdef CONFIG_COMPAT
-static int compat_do_execve(const char *filename,
+static int compat_do_execve(struct filename *filename,
const compat_uptr_t __user *__argv,
const compat_uptr_t __user *__envp)
{
@@ -1607,25 +1621,13 @@ SYSCALL_DEFINE3(execve,
const char __user *const __user *, argv,
const char __user *const __user *, envp)
{
- struct filename *path = getname(filename);
- int error = PTR_ERR(path);
- if (!IS_ERR(path)) {
- error = do_execve(path->name, argv, envp);
- putname(path);
- }
- return error;
+ return do_execve(getname(filename), argv, envp);
}
#ifdef CONFIG_COMPAT
asmlinkage long compat_sys_execve(const char __user * filename,
const compat_uptr_t __user * argv,
const compat_uptr_t __user * envp)
{
- struct filename *path = getname(filename);
- int error = PTR_ERR(path);
- if (!IS_ERR(path)) {
- error = compat_do_execve(path->name, argv, envp);
- putname(path);
- }
- return error;
+ return compat_do_execve(getname(filename), argv, envp);
}
#endif
diff --git a/fs/namei.c b/fs/namei.c
index d580df2e6804..593d70a75ebd 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -210,6 +210,33 @@ getname(const char __user * filename)
return getname_flags(filename, 0, NULL);
}

+/*
+ * The "getname_kernel()" interface doesn't do pathnames longer
+ * than EMBEDDED_NAME_MAX. Deal with it - you're a kernel user.
+ */
+struct filename *
+getname_kernel(const char * filename)
+{
+ struct filename *result;
+ char *kname;
+ int len;
+
+ len = strlen(filename);
+ if (len >= EMBEDDED_NAME_MAX)
+ return ERR_PTR(-ENAMETOOLONG);
+
+ result = __getname();
+ if (unlikely(!result))
+ return ERR_PTR(-ENOMEM);
+
+ kname = (char *)result + sizeof(*result);
+ result->name = kname;
+ result->separate = false;
+
+ strlcpy(kname, filename, EMBEDDED_NAME_MAX);
+ return result;
+}
+
#ifdef CONFIG_AUDITSYSCALL
void putname(struct filename *name)
{
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index fd8bf3219ef7..4c4a85accbb1 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -37,7 +37,7 @@ struct linux_binprm {
int unsafe; /* how unsafe this exec is (mask of LSM_UNSAFE_*) */
unsigned int per_clear; /* bits to clear in current->personality */
int argc, envc;
- const char * filename; /* Name of binary as seen by procps */
+ struct filename *filename; /* Name of binary as seen by procps */
const char * interp; /* Name of the binary really executed. Most
of the time same as filename, but could be
different for binfmt_{misc,script} */
@@ -115,7 +115,6 @@ extern int copy_strings_kernel(int argc, const char *const *argv,
extern int prepare_bprm_creds(struct linux_binprm *bprm);
extern void install_exec_creds(struct linux_binprm *bprm);
extern void set_binfmt(struct linux_binfmt *new);
-extern void free_bprm(struct linux_binprm *);
extern ssize_t read_code(struct file *, unsigned long, loff_t, size_t);

#endif /* _LINUX_BINFMTS_H */
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 09f553c59813..d79678c188ad 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2079,6 +2079,7 @@ extern struct file * dentry_open(const struct path *, int, const struct cred *);
extern int filp_close(struct file *, fl_owner_t id);

extern struct filename *getname(const char __user *);
+extern struct filename *getname_kernel(const char *);

enum {
FILE_CREATED = 1,
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 68a0e84463a0..a781dec1cd0b 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -128,6 +128,7 @@ struct bio_list;
struct fs_struct;
struct perf_event_context;
struct blk_plug;
+struct filename;

/*
* List of flags we want to share for kernel threads,
@@ -2311,7 +2312,7 @@ extern void do_group_exit(int);
extern int allow_signal(int);
extern int disallow_signal(int);

-extern int do_execve(const char *,
+extern int do_execve(struct filename *,
const char __user * const __user *,
const char __user * const __user *);
extern long do_fork(unsigned long, unsigned long, unsigned long, int __user *, int __user *);
diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h
index 67e1bbf83695..c1e611e29cb3 100644
--- a/include/trace/events/sched.h
+++ b/include/trace/events/sched.h
@@ -287,13 +287,13 @@ TRACE_EVENT(sched_process_exec,
TP_ARGS(p, old_pid, bprm),

TP_STRUCT__entry(
- __string( filename, bprm->filename )
+ __string( filename, bprm->filename->name )
__field( pid_t, pid )
__field( pid_t, old_pid )
),

TP_fast_assign(
- __assign_str(filename, bprm->filename);
+ __assign_str(filename, bprm->filename->name);
__entry->pid = p->pid;
__entry->old_pid = old_pid;
),
diff --git a/init/main.c b/init/main.c
index 2fd9cef70ee8..eb03090cdced 100644
--- a/init/main.c
+++ b/init/main.c
@@ -812,7 +812,7 @@ void __init load_default_modules(void)
static int run_init_process(const char *init_filename)
{
argv_init[0] = init_filename;
- return do_execve(init_filename,
+ return do_execve(getname_kernel(init_filename),
(const char __user *const __user *)argv_init,
(const char __user *const __user *)envp_init);
}
diff --git a/kernel/kmod.c b/kernel/kmod.c
index b086006c59e7..6b375af4958d 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -239,7 +239,7 @@ static int ____call_usermodehelper(void *data)

commit_creds(new);

- retval = do_execve(sub_info->path,
+ retval = do_execve(getname_kernel(sub_info->path),
(const char __user *const __user *)sub_info->argv,
(const char __user *const __user *)sub_info->envp);
if (!retval)
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 452567d3a08e..0358166d0652 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -372,7 +372,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
if (unconfined(profile) ||
(profile->flags & PFLAG_IX_ON_NAME_ERROR))
error = 0;
- name = bprm->filename;
+ name = bprm->filename->name;
goto audit;
}

diff --git a/security/commoncap.c b/security/commoncap.c
index b9d613e0ef14..43b39e520697 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -449,7 +449,7 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c
if (rc < 0) {
if (rc == -EINVAL)
printk(KERN_NOTICE "%s: get_vfs_caps_from_disk returned %d for %s\n",
- __func__, rc, bprm->filename);
+ __func__, rc, bprm->filename->name);
else if (rc == -ENODATA)
rc = 0;
goto out;
@@ -458,7 +458,7 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c
rc = bprm_caps_from_vfs_caps(&vcaps, bprm, effective, has_cap);
if (rc == -EINVAL)
printk(KERN_NOTICE "%s: cap_from_disk returned %d for %s\n",
- __func__, rc, bprm->filename);
+ __func__, rc, bprm->filename->name);

out:
dput(dentry);
@@ -498,7 +498,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
* for a root user just to cause least surprise to an admin.
*/
if (has_cap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) {
- warn_setuid_and_fcaps_mixed(bprm->filename);
+ warn_setuid_and_fcaps_mixed(bprm->filename->name);
goto skip;
}
/*
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 149ee1119f87..330b706a570e 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -278,8 +278,8 @@ int ima_file_mmap(struct file *file, unsigned long prot)
int ima_bprm_check(struct linux_binprm *bprm)
{
return process_measurement(bprm->file,
- (strcmp(bprm->filename, bprm->interp) == 0) ?
- bprm->filename : bprm->interp,
+ (strcmp(bprm->filename->name, bprm->interp) == 0) ?
+ bprm->filename->name : bprm->interp,
MAY_EXEC, BPRM_CHECK);
}

diff --git a/security/tomoyo/domain.c b/security/tomoyo/domain.c
index 38651454ed08..eb3230cbf158 100644
--- a/security/tomoyo/domain.c
+++ b/security/tomoyo/domain.c
@@ -677,7 +677,7 @@ int tomoyo_find_next_domain(struct linux_binprm *bprm)
{
struct tomoyo_domain_info *old_domain = tomoyo_domain();
struct tomoyo_domain_info *domain = NULL;
- const char *original_name = bprm->filename;
+ const char *original_name = bprm->filename->name;
int retval = -ENOMEM;
bool reject_on_transition_failure = false;
const struct tomoyo_path_info *candidate;
diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index f0b756e27fed..96757c3b1bd8 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -90,7 +90,7 @@ static int tomoyo_bprm_set_creds(struct linux_binprm *bprm)
* for the first time.
*/
if (!tomoyo_policy_loaded)
- tomoyo_load_policy(bprm->filename);
+ tomoyo_load_policy(bprm->filename->name);
#endif
/*
* Release reference to "struct tomoyo_domain_info" stored inside