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

[Andrew Morton]

On Tue, 05 Jun 2007 17:05:25 +0200
Peter Zijlstra <a.p.zijlstra@xxxxxxxxx> 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>
_
  



-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/