Re: [PATCH 2/4] audit: rework execve audit

Andrew Morton
2007-06-05T23:40:08Z

On Tue, 05 Jun 2007 17:05:25 +0200
Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:

> The purpose of audit_bprm() is to log the argv array to a userspace daemon at
> the end of the execve system call. Since user-space hasn't had time to run,
> this array is still in pristine state on the process' stack; so no need to copy
> it, we can just grab it from there.
>
> In order to minimize the damage to audit_log_*() copy each string into a
> temporary kernel buffer first.
>
> Currently the audit code requires that the full argument vector fits in a
> single packet. So currently it does clip the argv size to a (sysctl) limit, but
> only when execve auditing is enabled.
>
> If the audit protocol gets extended to allow for multiple packets this check
> can be removed.
>
> ...
>

Please try to avoid trigger-happiness with the BUG_ON()s..

> struct audit_aux_data_socketcall {
> @@ -834,6 +834,47 @@ static int audit_log_pid_context(struct
> return rc;
> }
>
> +static void audit_log_execve_info(struct audit_buffer *ab,
> + struct audit_aux_data_execve *axi)
> +{
> + int i;
> + long len;
> + const char __user *p = (const char __user *)axi->mm->arg_start;
> +
> + if (axi->mm != current->mm)
> + return; /* execve failed, no additional info */
> +
> + for (i = 0; i < axi->argc; i++, p += len) {
> + long ret;
> + char *tmp;
> +
> + len = strnlen_user(p, MAX_ARG_PAGES*PAGE_SIZE);
> + /*
> + * We just created this mm, if we can't find the strings
> + * we just copied in something is _very_ wrong.
> + */
> + BUG_ON(!len);
> +
> + tmp = kmalloc(len, GFP_KERNEL);
> + if (!tmp) {
> + audit_panic("out of memory for argv string\n");
> + break;
> + }
> +
> + ret = copy_from_user(tmp, p, len);
> + /*
> + * There is no reason for this copy to be short.
> + */
> + BUG_ON(ret);

You sure? What happens if another thread does munmap() in parallel?

I think I'll make this WARN_ON just out of principle.

> + audit_log_format(ab, "a%d=", i);
> + audit_log_untrustedstring(ab, tmp);
> + audit_log_format(ab, "\n");
> +
> + kfree(tmp);
> + }
> +}
> +
>
> ...
>
> ===================================================================
> --- linux-2.6-2.orig/fs/exec.c 2007-06-05 09:51:42.000000000 +0200
> +++ linux-2.6-2/fs/exec.c 2007-06-05 10:03:11.000000000 +0200
> @@ -1154,6 +1154,7 @@ int do_execve(char * filename,
> {
> struct linux_binprm *bprm;
> struct file *file;
> + unsigned long tmp;
> int retval;
> int i;
>
> @@ -1208,9 +1209,11 @@ int do_execve(char * filename,
> if (retval < 0)
> goto out;
>
> + tmp = bprm->p;
> retval = copy_strings(bprm->argc, argv, bprm);
> if (retval < 0)
> goto out;
> + bprm->argv_len = tmp - bprm->p;

--- a/include/linux/kernel.h~a
+++ a/include/linux/kernel.h
@@ -5,6 +5,8 @@
* 'kernel.h' contains some often-used function prototypes etc
*/

+#define tmp don't call your variables tmp!
+
#ifdef __KERNEL__

#include <stdarg.h>
_