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

From: Linus Torvalds
Date: Tue Feb 04 2014 - 22:37:52 EST

On Tue, Feb 4, 2014 at 5:10 PM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> Umm... Interactions with aushit might be interesting.

Freudian slip or intentional? :-)

> It hooks into getname() and putname(); I'm not up to doing analysis
> right now [...]

Right you are. I was actually aware of that, but grepping for things
it all looked fine. But I got confused by all the insane audit
wrappers, and you're right, it needs some massaging for audit
handling.

And that audit code really is aushit. I think I found a bug in it
while just scanning it: if audit_alloc_name() fails, the filename will
never be added to the audit lists, and name_count will never be
incremented. But then when we call audit_putname it won't actually put
the name, so it all just leaks - and if you have AUDIT_DEBUG enabled
you'd eventually see an error.

I wonder if we could get rid of some of that crap, and make the audit
code use dentry_path() instead of trying to save off pathnames like
that. But I don't know what the audit code actually *uses* the
pathnames for, so what do I know.

Eric? Can you please explain?

Also, here's a slightly updated patch. The change is that:
- getname_kernel() will now clear 'filename->aname'
- cleared 'aname' for regular getname too before calling
audit_getname(), so that if that one fails, it will be NULL.
- audit_putname() will consider a NULL aname to be the same as not
being in audit context, and just do a final_putname() on it.

That should fix the audit filename leak too, afaik.

Eric, please take a look. As well as explain the audit name thing if possible.

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 | 30 ++++++++++++++++++++
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/auditsc.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 +-
18 files changed, 90 insertions(+), 53 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..385f7817bfcc 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -196,6 +196,7 @@ recopy:
goto error;

result->uptr = filename;
+ result->aname = NULL;
audit_getname(result);
return result;

@@ -210,6 +211,35 @@ 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->uptr = NULL;
+ result->aname = NULL;
+ 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/auditsc.c b/kernel/auditsc.c
index 10176cd5956a..7aef2f4b6c64 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1719,7 +1719,7 @@ void audit_putname(struct filename *name)
struct audit_context *context = current->audit_context;

BUG_ON(!context);
- if (!context->in_syscall) {
+ if (!name->aname || !context->in_syscall) {
#if AUDIT_DEBUG == 2
printk(KERN_ERR "%s:%d(:%d): final_putname(%p)\n",
__FILE__, __LINE__, context->serial, name);
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