Re: [PATCH 2/4] audit: rework execve audit
On Tue, 2007-06-05 at 16:39 -0700, Andrew Morton wrote: > On Tue, 05 Jun 2007 17:05:25 +0200 > Peter Zijlstra <[EMAIL PROTECTED]> 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. This is right after the execve call, and before we've hit userspace, so at this time there is no runnable context with access to the memory (except this one). > > @@ -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 Fair enough. :-/ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] audit: rework execve audit
On Tue, 05 Jun 2007 17:05:25 +0200 Peter Zijlstra <[EMAIL PROTECTED]> 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.c2007-06-05 09:51:42.0 +0200 > +++ linux-2.6-2/fs/exec.c 2007-06-05 10:03:11.0 +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 _ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] audit: rework execve audit
On Tue, 05 Jun 2007 17:05:25 +0200 Peter Zijlstra [EMAIL PROTECTED] 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.c2007-06-05 09:51:42.0 +0200 +++ linux-2.6-2/fs/exec.c 2007-06-05 10:03:11.0 +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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] audit: rework execve audit
On Tue, 2007-06-05 at 16:39 -0700, Andrew Morton wrote: On Tue, 05 Jun 2007 17:05:25 +0200 Peter Zijlstra [EMAIL PROTECTED] 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. This is right after the execve call, and before we've hit userspace, so at this time there is no runnable context with access to the memory (except this one). @@ -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 Fair enough. :-/ - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/