On Sat, Mar 17, 2007 at 07:02:31PM -0400, Steve Grubb wrote:
On Wednesday 14 February 2007 13:08:03 Amy Griffis wrote:
> Reposting with fixes for audit_inc_name_count() return value and
> better conditional context->name_count >= AUDIT_NAMES. Thanks Eric P.
Something is wrong with this patch as its causing slab corruption in the lspp
65 and later kernels. I'll try to figure out what's wrong...
I see one obviously wrong thing: we leave ->name and ->name_put alone in
if (audit_inc_name_count(context, parent))
return;
idx = context->name_count - 1;
audit_copy_inode(&context->names[idx], parent);
right after add_names.
Note that when we do audit_syscall_exit() -> audit_free_names() we do
*not* throw the context away. So ->name and ->name_put are left as-is
since the before audit_free_names(), with ->name pointing to freed memory.
So when we get to the quoted code, we advance ->name_count to entry that
might very well have had stale ->name *and* non-zero ->name_put. Guess
what happens when we do audit_free_names() again on that context?
The rule we need to enforce is "anything past ->name_count is garbage".
So we need to add context->names[idx].name = NULL; in there (->name_put is
harmless after that).
Another comment: for pity sake, simplify those boolean expressions. I mean,
not only
if (found_child || (!found_parent && !found_child)) {
is
if (found_child || !found_parent) {
but in this case it's simply
if (!found_parent) {
since we can't get both found_parent and found_child non-NULL at the same
time. The second one (several lines below) can go from
if (found_parent || (!found_parent && !found_child)) {
to
if (found_parent || !found_child) {
and to
if (!found_child) {
since again, non-NULL found_child means NULL found_parent. At which point
code actually starts to make sense...
--- kernel/auditsc.c 2007-03-19 02:36:00.000000000 -0400
+++ kernel/auditsc.c 2007-03-19 03:23:19.000000000 -0400
@@ -1404,14 +1404,15 @@
}
add_names:
- if (found_child || (!found_parent && !found_child)) {
+ if (!found_parent) {
if (audit_inc_name_count(context, parent))
return;
idx = context->name_count - 1;
+ context->names[idx].name = NULL;
audit_copy_inode(&context->names[idx], parent);
}
- if (found_parent || (!found_parent && !found_child)) {
+ if (!found_child) {
if (audit_inc_name_count(context, inode))
return;
idx = context->name_count - 1;
@@ -1424,6 +1425,8 @@
context->names[idx].name_len = AUDIT_NAME_FULL;
/* don't call __putname() */
context->names[idx].name_put = 0;
+ } else {
+ context->names[idx].name = NULL;
}
if (inode)