Hello, <This is an updated version of the patch sent earlier. I should have been using the returned pointer from d_path instead of the buffer start.> As I was working on the new parsers for ausearch, I ran across some issues with untrusted string logging. Attached is a patch to fix this. Let me go over them as they appear in the patch. * In audit_log_vformat, we expand the skb just enough memory to cover current needs. There's no hysteresis. Imagine a call to log untrusted string passing a string of 4024 bytes and has a space in it. We will make 3000 calls to reallocate the buffer since audit_log_hex calls audit_log_vformat for every character it needs to convert. This is fixed by allocating in multiples of AUDIT_BUFSIZ. * If vsnprintf returns -1, it will mess up the sk buffer space accounting. This is fixed by not calling skb_put with bogus len values. * audit_log_hex was a loop that called audit_log_vformat with %02X for each character. This is very inefficient since conversion from unsigned character to Ascii representation is essentially masking, shifting, and byte lookups. Also, the length of the converted string is well known - its twice the original. Fixed by rewriting the function. *audit_log_untrustedstring had no comments. This makes it hard for someone to understand what the string format will be. * audit_log_d_path was never fixed to use untrustedstring. This could mess up user space parsers. This was fixed to make a temp buffer, call d_path, and log temp buffer using untrustedstring. *avc messages print the comm string without escaping. This was not fixed when we introduced untrustedstring and modified auditsc.c. -Steve Grubb

On Thu, 2005-05-12 at 13:29 -0400, Steve Grubb wrote: The reason I didn't do it at the time was because I didn't see a simple way to treat the _entire_ path, prefix and all, as a single string to be either quoted or not by audit_log_untrustedstring() as appropriate. I've changed your version slightly so it looks like this... /* This is a helper-function to print the escaped d_path */ void audit_log_d_path(struct audit_buffer *ab, const char *prefix, struct dentry *dentry, struct vfsmount *vfsmnt) { char *p, *path; int plen = 0; if (prefix) plen = strlen(prefix); /* We will allow 11 spaces for ' (deleted)' to be appended */ path = kmalloc(PATH_MAX+11+plen, GFP_KERNEL); if (!path) { audit_log_format(ab, "<no memory>"); return; } p = d_path(dentry, vfsmnt, path+plen, PATH_MAX+11); if (IS_ERR(p)) { /* Should never happen since we send PATH_MAX */ /* FIXME: can we save some information here? */ audit_log_format(ab, "<too long>"); } else { /* Prepend prefix to path, then log the whole thing together either quoted or unquoted as appropriate */ if (plen) { p -= plen; memcpy(p, prefix, plen); } audit_log_untrustedstring(ab, p); } kfree(path); } -- dwmw2