Skip to contentSkip to conversation

name_count array overrun

PATCH In review Steve Grubb 2 participants 5 messages Sep 7, 2006 – Sep 27, 2006 Audit

lore
Share & tools
lore

Steve Grubb

author
#0 lore raw

PATCH 1/1 name_count array overrun

Hello,

The below patch closes an unbounded use of name_count. This can lead to oopses in some new file systems.

Signed-off-by Steve Grubb <[email protected]>
kernel/auditsc.c +25 -3
diff -urp linux-2.6.17.x86_64.orig/kernel/auditsc.c linux-2.6.17.x86_64/kernel/auditsc.c
@@ -1281,7 +1281,15 @@ void __audit_inode(const char *name, con
 		 * associated name? */
 		if (context->name_count >= AUDIT_NAMES - AUDIT_NAMES_RESERVED)
 			return;
-		idx = context->name_count++;
+		idx = context->name_count;
+		if (context->name_count == (AUDIT_NAMES - 1)) {
+			printk(KERN_DEBUG
+				"name_count maxed and losing entry [%d]=%s\n",
+				context->name_count, 
+				context->names[context->name_count].name ?:
+				"(null)");
+		} else
+			context->name_count++;
 		context->names[idx].name = NULL;
 #if AUDIT_DEBUG
 		++context->ino_count;

@@ -1333,7 +1341,13 @@ void __audit_inode_child(const char *dna
 		}
 
 update_context:
-	idx = context->name_count++;
+	idx = context->name_count;
+	if (context->name_count == (AUDIT_NAMES - 1)) {
+		printk(KERN_DEBUG "name_count maxed and losing entry [%d]=%s\n",
+			context->name_count, 
+			context->names[context->name_count].name ?: "(null)");
+	} else
+		context->name_count++;
 #if AUDIT_DEBUG
 	context->ino_count++;
 #endif

@@ -1351,7 +1365,15 @@ update_context:
 	/* A parent was not found in audit_names, so copy the inode data for the
 	 * provided parent. */
 	if (!found_name) {
-		idx = context->name_count++;
+		idx = context->name_count;
+		if (context->name_count == (AUDIT_NAMES - 1)) {
+			printk(KERN_DEBUG 
+				"name_count maxed and losing entry [%d]=%s\n",
+				context->name_count, 
+				context->names[context->name_count].name ?:
+				"(null)");
+		} else
+			context->name_count++;
 #if AUDIT_DEBUG
 		context->ino_count++;
 #endif
#1 lore raw

Steve Grubb wrote: [Thu Sep 07 2006, 02:00:06PM EDT]

Quoted 12 lines

Hello,

The below patch closes an unbounded use of name_count. This can lead to oopses in some new file systems.

Signed-off-by Steve Grubb <[email protected]>

diff -urp linux-2.6.17.x86_64.orig/kernel/auditsc.c linux-2.6.17.x86_64/kernel/auditsc.c

kernel/auditsc.c void __audit_inode(const char *name, con · line 1283

Quoted 3 lines
kernel/auditsc.c +0 -0
@@ -1281,7 +1281,15 @@ void __audit_inode(const char *name, con
 		 * associated name? */
 		if (context->name_count >= AUDIT_NAMES - AUDIT_NAMES_RESERVED)
 			return;

What about this conditional, which translates to context->name_count >= 13? Leaving the code as is means we'll never reach the new printk below, where context->name_count == 19.

kernel/auditsc.c void __audit_inode(const char *name, con · line 1290

Quoted 8 lines
kernel/auditsc.c +7 -1
-		idx = context->name_count++;
+		idx = context->name_count;
+		if (context->name_count == (AUDIT_NAMES - 1)) {
+			printk(KERN_DEBUG
+				"name_count maxed and losing entry [%d]=%s\n",
+				context->name_count, 
+				context->names[context->name_count].name ?:
+				"(null)");

This is a little misleading, since the first time we hit it, we haven't lost anything yet. We're only losing data on the second and following times we hit it.

Did you consider just dropping any data encountered after we've filled AUDIT_NAMES, instead of copying over the data for the last element?

Quoted 43 lines
kernel/auditsc.c +18 -3
+		} else
+			context->name_count++;
 		context->names[idx].name = NULL;
 #if AUDIT_DEBUG
 		++context->ino_count;

@@ -1333,7 +1341,13 @@ void __audit_inode_child(const char *dna
 		}
 
 update_context:
-	idx = context->name_count++;
+	idx = context->name_count;
+	if (context->name_count == (AUDIT_NAMES - 1)) {
+		printk(KERN_DEBUG "name_count maxed and losing entry [%d]=%s\n",
+			context->name_count, 
+			context->names[context->name_count].name ?: "(null)");
+	} else
+		context->name_count++;
 #if AUDIT_DEBUG
 	context->ino_count++;
 #endif

@@ -1351,7 +1365,15 @@ update_context:
 	/* A parent was not found in audit_names, so copy the inode data for the
 	 * provided parent. */
 	if (!found_name) {
-		idx = context->name_count++;
+		idx = context->name_count;
+		if (context->name_count == (AUDIT_NAMES - 1)) {
+			printk(KERN_DEBUG 
+				"name_count maxed and losing entry [%d]=%s\n",
+				context->name_count, 
+				context->names[context->name_count].name ?:
+				"(null)");
+		} else
+			context->name_count++;
 #if AUDIT_DEBUG
 		context->ino_count++;
 #endif
 
--

Linux-audit mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/linux-audit

author
#2 lore raw
Quoted 3 lines · Amy Griffis

What about this conditional, which translates to context->name_count >= 13? Leaving the code as is means we'll never reach the new printk below, where context->name_count == 19.

Good point, I'll drop that part.

Quoted 11 lines
Quoted 8 lines

-             idx = context->name_count++;
+             idx = context->name_count;
+             if (context->name_count == (AUDIT_NAMES - 1)) { +                     printk(KERN_DEBUG
+                             "name_count maxed and losing entry [%d]=%s\n", +                             context->name_count, +                             context->names[context->name_count].name ?:
+                             "(null)");

Did you consider just dropping any data encountered after we've filled AUDIT_NAMES, instead of copying over the data for the last element?

That might be better. Is this the way we want to handle it? If there's no objections, I'll repost a patch tomorrow.

Thanks,
-Steve

author
#3 lore raw

PATCH 1/1 name_count array overrun

In reply to Amy Griffis
Quoted 2 lines · Amy Griffis

Did you consider just dropping any data encountered after we've filled AUDIT_NAMES, instead of copying over the data for the last element?

OK, corrected patch follows.

The below patch closes an unbounded use of name_count. This can lead to oopses in some new file systems.

Signed-off-by Steve Grubb <[email protected]>

diff -urp linux-2.6.18.x86_64.orig/kernel/auditsc.c linux-2.6.18.x86_64/kernel/auditsc.c
--- linux-2.6.18.x86_64.orig/kernel/auditsc.c 2006-09-24
08:24:27.000000000 -0400
+++ linux-2.6.18.x86_64/kernel/auditsc.c 2006-09-24 08:42:01.000000000 -0400
@@ -1347,7 +1347,13 @@ void __audit_inode_child(const char *dna
}

update_context:
- idx = context->name_count++;
+ idx = context->name_count;
+ if (context->name_count == AUDIT_NAMES) {
+ printk(KERN_DEBUG "name_count maxed and losing %s\n",
+ found_name ?: "(null)");
+ return;
+ }
+ context->name_count++;
#if AUDIT_DEBUG
context->ino_count++;
#endif
@@ -1365,7 +1371,18 @@ update_context:
/* A parent was not found in audit_names, so copy the inode data for the
* provided parent. */
if (!found_name) {
- idx = context->name_count++;
+ idx = context->name_count;
+ if (context->name_count == AUDIT_NAMES) {
+ printk(KERN_DEBUG
+ "name_count maxed and losing parent inode data: dev=%02x:%02x rdev=%02x:
%02x, inode=%lu",
+ MAJOR(parent->i_sb->s_dev),
+ MINOR(parent->i_sb->s_dev),
+ MAJOR(parent->i_rdev),
+ MINOR(parent->i_rdev),
+ parent->i_ino);
+ return;
+ }
+ context->name_count++;
#if AUDIT_DEBUG
context->ino_count++;
#endif

#4 lore raw

Steve Grubb wrote: [Sun Sep 24 2006, 08:56:57AM EDT]

Quoted 5 lines · Amy Griffis
Quoted 2 lines

Did you consider just dropping any data encountered after we've filled AUDIT_NAMES, instead of copying over the data for the last element?

OK, corrected patch follows.

Sorry for the delayed response, I've been out of town.

Do we want a similar printk in __audit_inode()?

Quoted 48 lines

The below patch closes an unbounded use of name_count. This can lead to oopses in some new file systems.

Signed-off-by Steve Grubb <[email protected]>

diff -urp linux-2.6.18.x86_64.orig/kernel/auditsc.c linux-2.6.18.x86_64/kernel/auditsc.c
--- linux-2.6.18.x86_64.orig/kernel/auditsc.c 2006-09-24
08:24:27.000000000 -0400
+++ linux-2.6.18.x86_64/kernel/auditsc.c 2006-09-24 08:42:01.000000000 -0400

kernel/auditsc.c +19 -2
@@ -1347,7 +1347,13 @@ void __audit_inode_child(const char *dna
 		}
 
 update_context:
-	idx = context->name_count++;
+	idx = context->name_count;
+	if (context->name_count == AUDIT_NAMES) {
+		printk(KERN_DEBUG "name_count maxed and losing %s\n",
+			found_name ?: "(null)");
+		return;
+	} 
+	context->name_count++;
 #if AUDIT_DEBUG
 	context->ino_count++;
 #endif

@@ -1365,7 +1371,18 @@ update_context:
 	/* A parent was not found in audit_names, so copy the inode data for the
 	 * provided parent. */
 	if (!found_name) {
-		idx = context->name_count++;
+		idx = context->name_count;
+		if (context->name_count == AUDIT_NAMES) {
+			printk(KERN_DEBUG 
+				"name_count maxed and losing parent inode data: dev=%02x:%02x rdev=%02x:
 %02x, inode=%lu",
+				MAJOR(parent->i_sb->s_dev),
+				MINOR(parent->i_sb->s_dev),
+				MAJOR(parent->i_rdev),
+				MINOR(parent->i_rdev),
+				parent->i_ino);
+			return;
+		}
+		context->name_count++;
 #if AUDIT_DEBUG
 		context->ino_count++;

#endif