* [PATCH] name_count array overrun
@ 2006-09-28 18:31 Steve Grubb
2006-09-29 12:46 ` Alexander Viro
0 siblings, 1 reply; 7+ messages in thread
From: Steve Grubb @ 2006-09-28 18:31 UTC (permalink / raw)
To: Linux Audit
Hi,
This patch removes the rdev logging from the previous patch
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 <sgrubb@redhat.com>
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, inode=%lu",
+ MAJOR(parent->i_sb->s_dev),
+ MINOR(parent->i_sb->s_dev),
+ parent->i_ino);
+ return;
+ }
+ context->name_count++;
#if AUDIT_DEBUG
context->ino_count++;
#endif
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] name_count array overrun
2006-09-28 18:31 [PATCH] name_count array overrun Steve Grubb
@ 2006-09-29 12:46 ` Alexander Viro
0 siblings, 0 replies; 7+ messages in thread
From: Alexander Viro @ 2006-09-29 12:46 UTC (permalink / raw)
To: Steve Grubb; +Cc: Linux Audit
On Thu, Sep 28, 2006 at 02:31:32PM -0400, Steve Grubb wrote:
> Hi,
>
> This patch removes the rdev logging from the previous patch
>
> 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 <sgrubb@redhat.com>
ACK, but please repost with a mailer that wouldn't eat patches. It's
got serious tab damage.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] name_count array overrun
@ 2006-09-07 18:00 Steve Grubb
2006-09-07 20:43 ` Amy Griffis
0 siblings, 1 reply; 7+ messages in thread
From: Steve Grubb @ 2006-09-07 18:00 UTC (permalink / raw)
To: Linux Audit
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 <sgrubb@redhat.com>
diff -urp linux-2.6.17.x86_64.orig/kernel/auditsc.c linux-2.6.17.x86_64/kernel/auditsc.c
--- linux-2.6.17.x86_64.orig/kernel/auditsc.c 2006-08-29 11:21:20.000000000 -0400
+++ linux-2.6.17.x86_64/kernel/auditsc.c 2006-08-29 15:15:28.000000000 -0400
@@ -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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] name_count array overrun
2006-09-07 18:00 Steve Grubb
@ 2006-09-07 20:43 ` Amy Griffis
2006-09-07 20:53 ` Steve Grubb
2006-09-24 12:56 ` Steve Grubb
0 siblings, 2 replies; 7+ messages in thread
From: Amy Griffis @ 2006-09-07 20:43 UTC (permalink / raw)
To: linux-audit
Steve Grubb wrote: [Thu Sep 07 2006, 02:00:06PM EDT]
> 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 <sgrubb@redhat.com>
>
>
> diff -urp linux-2.6.17.x86_64.orig/kernel/auditsc.c linux-2.6.17.x86_64/kernel/auditsc.c
> --- linux-2.6.17.x86_64.orig/kernel/auditsc.c 2006-08-29 11:21:20.000000000 -0400
> +++ linux-2.6.17.x86_64/kernel/auditsc.c 2006-08-29 15:15:28.000000000 -0400
> @@ -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.
> - 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?
> + } 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
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] name_count array overrun
2006-09-07 20:43 ` Amy Griffis
@ 2006-09-07 20:53 ` Steve Grubb
2006-09-24 12:56 ` Steve Grubb
1 sibling, 0 replies; 7+ messages in thread
From: Steve Grubb @ 2006-09-07 20:53 UTC (permalink / raw)
To: linux-audit
On Thursday 07 September 2006 16:43, Amy Griffis wrote:
> 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.
> > - 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
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH] name_count array overrun
2006-09-07 20:43 ` Amy Griffis
2006-09-07 20:53 ` Steve Grubb
@ 2006-09-24 12:56 ` Steve Grubb
2006-09-27 21:04 ` Amy Griffis
1 sibling, 1 reply; 7+ messages in thread
From: Steve Grubb @ 2006-09-24 12:56 UTC (permalink / raw)
To: linux-audit
On Thursday 07 September 2006 16:43, Amy Griffis wrote:
> 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 <sgrubb@redhat.com>
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] name_count array overrun
2006-09-24 12:56 ` Steve Grubb
@ 2006-09-27 21:04 ` Amy Griffis
0 siblings, 0 replies; 7+ messages in thread
From: Amy Griffis @ 2006-09-27 21:04 UTC (permalink / raw)
To: linux-audit
Steve Grubb wrote: [Sun Sep 24 2006, 08:56:57AM EDT]
> On Thursday 07 September 2006 16:43, Amy Griffis wrote:
> > 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()?
> 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 <sgrubb@redhat.com>
>
>
> 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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2006-09-29 12:46 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-09-28 18:31 [PATCH] name_count array overrun Steve Grubb
2006-09-29 12:46 ` Alexander Viro
-- strict thread matches above, loose matches on Subject: below --
2006-09-07 18:00 Steve Grubb
2006-09-07 20:43 ` Amy Griffis
2006-09-07 20:53 ` Steve Grubb
2006-09-24 12:56 ` Steve Grubb
2006-09-27 21:04 ` Amy Griffis
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox