* 3.6-rc4 audit_log_d_path oops.
@ 2012-09-06 13:46 Dave Jones
2012-09-06 15:08 ` Check all returns from audit_log_start Dave Jones
2012-09-06 15:16 ` 3.6-rc4 audit_log_d_path oops Dave Jones
0 siblings, 2 replies; 10+ messages in thread
From: Dave Jones @ 2012-09-06 13:46 UTC (permalink / raw)
To: Linux Kernel; +Cc: Al Viro
Hit this in overnight fuzz testing..
BUG: unable to handle kernel NULL pointer dereference at 0000000000000020
IP: [<ffffffff81103365>] audit_log_d_path+0x35/0xf0
PGD 12fded067 PUD 142c06067 PMD 0
Oops: 0000 [#1] SMP
Modules linked in: tun fuse ipt_ULOG binfmt_misc nfnetlink nfc caif_socket caif phonet can llc2 pppoe pppox ppp_generic slhc irda crc_ccitt rds af_key decnet rose x25 atm netrom appletalk ipx p8023 psnap p8022 llc ax25 lockd sunrpc bluetooth rfkill ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 ip6table_filter ip6_tables nf_conntrack_ipv4 nf_defrag_ipv4 xt_state nf_conntrack kvm_intel kvm crc32c_intel ghash_clmulni_intel microcode pcspkr i2c_i801 e1000e uinput i915 video i2c_algo_bit drm_kms_helper drm i2c_core
CPU 5
Pid: 7007, comm: trinity-child5 Not tainted 3.6.0-rc4+ #36
RIP: 0010:[<ffffffff81103365>] [<ffffffff81103365>] audit_log_d_path+0x35/0xf0
RSP: 0018:ffff880116b33ec8 EFLAGS: 00010246
RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000035
RDX: ffffffff819dbf5d RSI: ffffffff819da5bb RDI: 0000000000000000
RBP: ffff880116b33ee8 R08: ffff88001942533e R09: 0000000000000000
R10: 0000000000000001 R11: 0000000000000000 R12: ffff880116b33f38
R13: ffff880116b33f38 R14: ffffffff819fa1eb R15: 0000000000000005
FS: 00007f5742581740(0000) GS:ffff880148600000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000020 CR3: 00000001046d3000 CR4: 00000000001407e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process trinity-child5 (pid: 7007, threadinfo ffff880116b32000, task ffff880019424d00)
Stack:
ffff880116b33ee8 0000000000000000 ffff880116b33f38 ffff880019424d00
ffff880116b33f18 ffffffff81103522 00000000000001d0 ffff880119ed6500
00000000000001d0 0000000000000109 ffff880116b33f78 ffffffff811ebb95
Call Trace:
[<ffffffff81103522>] audit_log_link_denied+0x92/0x100
[<ffffffff811ebb95>] sys_linkat+0x195/0x1e0
[<ffffffff813573de>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[<ffffffff816a50ed>] system_call_fastpath+0x1a/0x1f
Code: 5d e8 4c 89 65 f0 4c 89 6d f8 0f 1f 44 00 00 48 85 f6 48 89 fb 49 89 d5 74 11 48 89 f2 31 c0 48 c7 c6 bb a5 9d 81 e8 8b e1 ff ff <8b> 73 20 40 f6 c6 01 75 62 48 8b 3d 33 33 93 01 48 85 ff 74 4e
RIP [<ffffffff81103365>] audit_log_d_path+0x35/0xf0
RSP <ffff880116b33ec8>
CR2: 0000000000000020
---[ end trace 85b88c850143bb1c ]---
That's here in kernel/audit.c
1433
1434 /* We will allow 11 spaces for ' (deleted)' to be appended */
1435 pathname = kmalloc(PATH_MAX+11, ab->gfp_mask);
'ab' is NULL.
Looks like audit_log_link_denied needs to handle potential failure from
audit_log_start and abort early. (oddly, it looks like every other
function called there checks for !ab.)
Maybe additional code should be added here to printk the audit message
to dmesg so that we don't lose it entirely, but for now, minimal fix.
Signed-off-by: Dave Jones <davej@redhat.com>
diff --git a/kernel/audit.c b/kernel/audit.c
index ea3b7b6..c3e85bb 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1466,6 +1466,9 @@ void audit_log_link_denied(const char *operation, struct path *link)
ab = audit_log_start(current->audit_context, GFP_KERNEL,
AUDIT_ANOM_LINK);
+ if (!ab)
+ return;
+
audit_log_format(ab, "op=%s action=denied", operation);
audit_log_format(ab, " pid=%d comm=", current->pid);
audit_log_untrustedstring(ab, current->comm);
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Check all returns from audit_log_start
2012-09-06 13:46 3.6-rc4 audit_log_d_path oops Dave Jones
@ 2012-09-06 15:08 ` Dave Jones
2012-09-06 15:36 ` Eric Paris
2012-09-06 15:16 ` 3.6-rc4 audit_log_d_path oops Dave Jones
1 sibling, 1 reply; 10+ messages in thread
From: Dave Jones @ 2012-09-06 15:08 UTC (permalink / raw)
To: Linux Kernel, Al Viro; +Cc: eparis
Following on from the previous patch that fixed an oops, these
are all the other similar code patterns in the tree with the same
checks added. I never saw these causing problems, but checking
this everywhere seems to make more sense than every subsequent
routine that gets passed 'ab' having to check it.
Later we could remove all those same checks from audit_log_format
and friends. For now, this just prevents similar bugs being introduced
as the one in my previous patch.
Signed-off-by: Dave Jones <davej@redhat.com>
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 36abf2a..2df27a7 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -665,7 +665,7 @@ extern __printf(4, 5)
void audit_log(struct audit_context *ctx, gfp_t gfp_mask, int type,
const char *fmt, ...);
-extern struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, int type);
+extern struct audit_buffer __must_check *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, int type);
extern __printf(2, 3)
void audit_log_format(struct audit_buffer *ab, const char *fmt, ...);
extern void audit_log_end(struct audit_buffer *ab);
diff --git a/kernel/audit.c b/kernel/audit.c
index ea3b7b6..832c549 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -271,6 +271,9 @@ static int audit_log_config_change(char *function_name, int new, int old,
int rc = 0;
ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
+ if (!ab)
+ return -EINVAL;
+
audit_log_format(ab, "%s=%d old=%d auid=%u ses=%u", function_name, new,
old, loginuid, sessionid);
if (sid) {
@@ -632,6 +635,9 @@ static int audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type,
}
*ab = audit_log_start(NULL, GFP_KERNEL, msg_type);
+ if (!*ab)
+ return -EINVAL;
+
audit_log_format(*ab, "pid=%d uid=%u auid=%u ses=%u",
pid, uid, auid, ses);
if (sid) {
@@ -1145,7 +1151,7 @@ static inline void audit_get_stamp(struct audit_context *ctx,
* will be written at syscall exit. If there is no associated task, then
* task context (ctx) should be NULL.
*/
-struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
+struct audit_buffer __must_check *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
int type)
{
struct audit_buffer *ab = NULL;
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index ed206fd..2580edf 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -462,13 +462,15 @@ static void kill_rules(struct audit_tree *tree)
if (rule->tree) {
/* not a half-baked one */
ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
- audit_log_format(ab, "op=");
- audit_log_string(ab, "remove rule");
- audit_log_format(ab, " dir=");
- audit_log_untrustedstring(ab, rule->tree->pathname);
- audit_log_key(ab, rule->filterkey);
- audit_log_format(ab, " list=%d res=1", rule->listnr);
- audit_log_end(ab);
+ if (ab) {
+ audit_log_format(ab, "op=");
+ audit_log_string(ab, "remove rule");
+ audit_log_format(ab, " dir=");
+ audit_log_untrustedstring(ab, rule->tree->pathname);
+ audit_log_key(ab, rule->filterkey);
+ audit_log_format(ab, " list=%d res=1", rule->listnr);
+ audit_log_end(ab);
+ }
rule->tree = NULL;
list_del_rcu(&entry->list);
list_del(&entry->rule.list);
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 4b96415..1f39dcf 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2707,6 +2707,8 @@ void audit_core_dumps(long signr)
return;
ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_ANOM_ABEND);
+ if (!ab)
+ return;
audit_log_abend(ab, "memory violation", signr);
audit_log_end(ab);
}
@@ -2716,6 +2718,8 @@ void __audit_seccomp(unsigned long syscall, long signr, int code)
struct audit_buffer *ab;
ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_ANOM_ABEND);
+ if (!ab)
+ return;
audit_log_abend(ab, "seccomp", signr);
audit_log_format(ab, " syscall=%ld", syscall);
audit_log_format(ab, " compat=%d", is_compat_task());
diff --git a/security/integrity/ima/ima_audit.c b/security/integrity/ima/ima_audit.c
index 7a57f67..6fa60ff 100644
--- a/security/integrity/ima/ima_audit.c
+++ b/security/integrity/ima/ima_audit.c
@@ -38,6 +38,9 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode,
return;
ab = audit_log_start(current->audit_context, GFP_KERNEL, audit_msgno);
+ if (!ab)
+ return;
+
audit_log_format(ab, "pid=%d uid=%u auid=%u ses=%u",
current->pid, current_cred()->uid,
audit_get_loginuid(current),
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 1a95830..2c39e18 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -276,6 +276,8 @@ static int ima_parse_rule(char *rule, struct ima_measure_rule_entry *entry)
int result = 0;
ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_INTEGRITY_RULE);
+ if (!ab)
+ return -EINVAL;
entry->uid = -1;
entry->action = UNKNOWN;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 6c77f63..59c5929 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2802,6 +2802,8 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name,
audit_size = 0;
}
ab = audit_log_start(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR);
+ if (!ab)
+ return -EINVAL;
audit_log_format(ab, "op=setxattr invalid_context=");
audit_log_n_untrustedstring(ab, value, audit_size);
audit_log_end(ab);
@@ -5318,6 +5320,8 @@ static int selinux_setprocattr(struct task_struct *p,
else
audit_size = size;
ab = audit_log_start(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR);
+ if (!ab)
+ return -EINVAL;
audit_log_format(ab, "op=fscreate invalid_context=");
audit_log_n_untrustedstring(ab, value, audit_size);
audit_log_end(ab);
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: 3.6-rc4 audit_log_d_path oops.
2012-09-06 13:46 3.6-rc4 audit_log_d_path oops Dave Jones
2012-09-06 15:08 ` Check all returns from audit_log_start Dave Jones
@ 2012-09-06 15:16 ` Dave Jones
2012-09-06 15:34 ` Eric Paris
2012-09-06 16:32 ` Kees Cook
1 sibling, 2 replies; 10+ messages in thread
From: Dave Jones @ 2012-09-06 15:16 UTC (permalink / raw)
To: Linux Kernel, Al Viro; +Cc: Kees Cook
On Thu, Sep 06, 2012 at 09:46:28AM -0400, Dave Jones wrote:
> Hit this in overnight fuzz testing..
>
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000020
> IP: [<ffffffff81103365>] audit_log_d_path+0x35/0xf0
> PGD 12fded067 PUD 142c06067 PMD 0
> Oops: 0000 [#1] SMP
> Modules linked in: tun fuse ipt_ULOG binfmt_misc nfnetlink nfc caif_socket caif phonet can llc2 pppoe pppox ppp_generic slhc irda crc_ccitt rds af_key decnet rose x25 atm netrom appletalk ipx p8023 psnap p8022 llc ax25 lockd sunrpc bluetooth rfkill ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 ip6table_filter ip6_tables nf_conntrack_ipv4 nf_defrag_ipv4 xt_state nf_conntrack kvm_intel kvm crc32c_intel ghash_clmulni_intel microcode pcspkr i2c_i801 e1000e uinput i915 video i2c_algo_bit drm_kms_helper drm i2c_core
> CPU 5
> Pid: 7007, comm: trinity-child5 Not tainted 3.6.0-rc4+ #36
> RIP: 0010:[<ffffffff81103365>] [<ffffffff81103365>] audit_log_d_path+0x35/0xf0
> RSP: 0018:ffff880116b33ec8 EFLAGS: 00010246
> RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000035
> RDX: ffffffff819dbf5d RSI: ffffffff819da5bb RDI: 0000000000000000
> RBP: ffff880116b33ee8 R08: ffff88001942533e R09: 0000000000000000
> R10: 0000000000000001 R11: 0000000000000000 R12: ffff880116b33f38
> R13: ffff880116b33f38 R14: ffffffff819fa1eb R15: 0000000000000005
> FS: 00007f5742581740(0000) GS:ffff880148600000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000020 CR3: 00000001046d3000 CR4: 00000000001407e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> Process trinity-child5 (pid: 7007, threadinfo ffff880116b32000, task ffff880019424d00)
> Stack:
> ffff880116b33ee8 0000000000000000 ffff880116b33f38 ffff880019424d00
> ffff880116b33f18 ffffffff81103522 00000000000001d0 ffff880119ed6500
> 00000000000001d0 0000000000000109 ffff880116b33f78 ffffffff811ebb95
> Call Trace:
> [<ffffffff81103522>] audit_log_link_denied+0x92/0x100
> [<ffffffff811ebb95>] sys_linkat+0x195/0x1e0
> [<ffffffff813573de>] ? trace_hardirqs_on_thunk+0x3a/0x3f
> [<ffffffff816a50ed>] system_call_fastpath+0x1a/0x1f
> Code: 5d e8 4c 89 65 f0 4c 89 6d f8 0f 1f 44 00 00 48 85 f6 48 89 fb 49 89 d5 74 11 48 89 f2 31 c0 48 c7 c6 bb a5 9d 81 e8 8b e1 ff ff <8b> 73 20 40 f6 c6 01 75 62 48 8b 3d 33 33 93 01 48 85 ff 74 4e
> RIP [<ffffffff81103365>] audit_log_d_path+0x35/0xf0
> RSP <ffff880116b33ec8>
> CR2: 0000000000000020
> ---[ end trace 85b88c850143bb1c ]---
>
> That's here in kernel/audit.c
>
> 1433
> 1434 /* We will allow 11 spaces for ' (deleted)' to be appended */
> 1435 pathname = kmalloc(PATH_MAX+11, ab->gfp_mask);
>
> 'ab' is NULL.
>
> Looks like audit_log_link_denied needs to handle potential failure from
> audit_log_start and abort early. (oddly, it looks like every other
> function called there checks for !ab.)
>
> Maybe additional code should be added here to printk the audit message
> to dmesg so that we don't lose it entirely, but for now, minimal fix.
>
> Signed-off-by: Dave Jones <davej@redhat.com>
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index ea3b7b6..c3e85bb 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1466,6 +1466,9 @@ void audit_log_link_denied(const char *operation, struct path *link)
>
> ab = audit_log_start(current->audit_context, GFP_KERNEL,
> AUDIT_ANOM_LINK);
> + if (!ab)
> + return;
> +
> audit_log_format(ab, "op=%s action=denied", operation);
> audit_log_format(ab, " pid=%d comm=", current->pid);
> audit_log_untrustedstring(ab, current->comm);
Added Kees, as this was introduced in a51d9eaa41866ab6b4b6ecad7b621f8b66ece0dc
I just realised, the funny thing about this is that the machine running that test
had selinux/audit disabled. And yet here we are, screwing around with audit buffers.
Should there be a test on audit_enable=0 in audit_log_link_denied() ?
I'm now curious how much more of the audit code is getting run through similar lack of tests
Dave
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: 3.6-rc4 audit_log_d_path oops.
2012-09-06 15:16 ` 3.6-rc4 audit_log_d_path oops Dave Jones
@ 2012-09-06 15:34 ` Eric Paris
2012-09-06 16:32 ` Kees Cook
1 sibling, 0 replies; 10+ messages in thread
From: Eric Paris @ 2012-09-06 15:34 UTC (permalink / raw)
To: Dave Jones, Linux Kernel, Al Viro, Kees Cook
On Thu, Sep 6, 2012 at 11:16 AM, Dave Jones <davej@redhat.com> wrote:
> On Thu, Sep 06, 2012 at 09:46:28AM -0400, Dave Jones wrote:
> > Hit this in overnight fuzz testing..
> >
> > BUG: unable to handle kernel NULL pointer dereference at 0000000000000020
> > IP: [<ffffffff81103365>] audit_log_d_path+0x35/0xf0
> > PGD 12fded067 PUD 142c06067 PMD 0
> > Oops: 0000 [#1] SMP
> > Modules linked in: tun fuse ipt_ULOG binfmt_misc nfnetlink nfc caif_socket caif phonet can llc2 pppoe pppox ppp_generic slhc irda crc_ccitt rds af_key decnet rose x25 atm netrom appletalk ipx p8023 psnap p8022 llc ax25 lockd sunrpc bluetooth rfkill ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 ip6table_filter ip6_tables nf_conntrack_ipv4 nf_defrag_ipv4 xt_state nf_conntrack kvm_intel kvm crc32c_intel ghash_clmulni_intel microcode pcspkr i2c_i801 e1000e uinput i915 video i2c_algo_bit drm_kms_helper drm i2c_core
> > CPU 5
> > Pid: 7007, comm: trinity-child5 Not tainted 3.6.0-rc4+ #36
> > RIP: 0010:[<ffffffff81103365>] [<ffffffff81103365>] audit_log_d_path+0x35/0xf0
> > RSP: 0018:ffff880116b33ec8 EFLAGS: 00010246
> > RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000035
> > RDX: ffffffff819dbf5d RSI: ffffffff819da5bb RDI: 0000000000000000
> > RBP: ffff880116b33ee8 R08: ffff88001942533e R09: 0000000000000000
> > R10: 0000000000000001 R11: 0000000000000000 R12: ffff880116b33f38
> > R13: ffff880116b33f38 R14: ffffffff819fa1eb R15: 0000000000000005
> > FS: 00007f5742581740(0000) GS:ffff880148600000(0000) knlGS:0000000000000000
> > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: 0000000000000020 CR3: 00000001046d3000 CR4: 00000000001407e0
> > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> > Process trinity-child5 (pid: 7007, threadinfo ffff880116b32000, task ffff880019424d00)
> > Stack:
> > ffff880116b33ee8 0000000000000000 ffff880116b33f38 ffff880019424d00
> > ffff880116b33f18 ffffffff81103522 00000000000001d0 ffff880119ed6500
> > 00000000000001d0 0000000000000109 ffff880116b33f78 ffffffff811ebb95
> > Call Trace:
> > [<ffffffff81103522>] audit_log_link_denied+0x92/0x100
> > [<ffffffff811ebb95>] sys_linkat+0x195/0x1e0
> > [<ffffffff813573de>] ? trace_hardirqs_on_thunk+0x3a/0x3f
> > [<ffffffff816a50ed>] system_call_fastpath+0x1a/0x1f
> > Code: 5d e8 4c 89 65 f0 4c 89 6d f8 0f 1f 44 00 00 48 85 f6 48 89 fb 49 89 d5 74 11 48 89 f2 31 c0 48 c7 c6 bb a5 9d 81 e8 8b e1 ff ff <8b> 73 20 40 f6 c6 01 75 62 48 8b 3d 33 33 93 01 48 85 ff 74 4e
> > RIP [<ffffffff81103365>] audit_log_d_path+0x35/0xf0
> > RSP <ffff880116b33ec8>
> > CR2: 0000000000000020
> > ---[ end trace 85b88c850143bb1c ]---
> >
> > That's here in kernel/audit.c
> >
> > 1433
> > 1434 /* We will allow 11 spaces for ' (deleted)' to be appended */
> > 1435 pathname = kmalloc(PATH_MAX+11, ab->gfp_mask);
> >
> > 'ab' is NULL.
> >
> > Looks like audit_log_link_denied needs to handle potential failure from
> > audit_log_start and abort early. (oddly, it looks like every other
> > function called there checks for !ab.)
> >
> > Maybe additional code should be added here to printk the audit message
> > to dmesg so that we don't lose it entirely, but for now, minimal fix.
> >
> > Signed-off-by: Dave Jones <davej@redhat.com>
> >
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index ea3b7b6..c3e85bb 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -1466,6 +1466,9 @@ void audit_log_link_denied(const char *operation, struct path *link)
> >
> > ab = audit_log_start(current->audit_context, GFP_KERNEL,
> > AUDIT_ANOM_LINK);
> > + if (!ab)
> > + return;
> > +
> > audit_log_format(ab, "op=%s action=denied", operation);
> > audit_log_format(ab, " pid=%d comm=", current->pid);
> > audit_log_untrustedstring(ab, current->comm);
>
> Added Kees, as this was introduced in a51d9eaa41866ab6b4b6ecad7b621f8b66ece0dc
>
> I just realised, the funny thing about this is that the machine running that test
> had selinux/audit disabled. And yet here we are, screwing around with audit buffers.
>
> Should there be a test on audit_enable=0 in audit_log_link_denied() ?
>
> I'm now curious how much more of the audit code is getting run through similar lack of tests
Possible there are other places. The usual idium is to put a a static
inline audit_[blah] in the header file which just does if
(!audit_dummy_context()) return __audit_[blah]; return 0;
Thus if audit is off or compiled out nothing gets run.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Check all returns from audit_log_start
2012-09-06 15:08 ` Check all returns from audit_log_start Dave Jones
@ 2012-09-06 15:36 ` Eric Paris
2012-09-06 15:47 ` Dave Jones
0 siblings, 1 reply; 10+ messages in thread
From: Eric Paris @ 2012-09-06 15:36 UTC (permalink / raw)
To: Dave Jones, Linux Kernel, Al Viro, eparis
On Thu, Sep 6, 2012 at 11:08 AM, Dave Jones <davej@redhat.com> wrote:
> Following on from the previous patch that fixed an oops, these
> are all the other similar code patterns in the tree with the same
> checks added. I never saw these causing problems, but checking
> this everywhere seems to make more sense than every subsequent
> routine that gets passed 'ab' having to check it.
>
> Later we could remove all those same checks from audit_log_format
> and friends. For now, this just prevents similar bugs being introduced
> as the one in my previous patch.
>
> Signed-off-by: Dave Jones <davej@redhat.com>
Not certain because I haven't looked at what happens with the error
code, but I think this might not be right. auditd can be explictly
told not to audit certain events, in which case it is normal and
expected that ab would come back NULL....
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Check all returns from audit_log_start
2012-09-06 15:36 ` Eric Paris
@ 2012-09-06 15:47 ` Dave Jones
2012-09-06 15:56 ` Dave Jones
0 siblings, 1 reply; 10+ messages in thread
From: Dave Jones @ 2012-09-06 15:47 UTC (permalink / raw)
To: Eric Paris; +Cc: Linux Kernel, Al Viro, eparis
On Thu, Sep 06, 2012 at 11:36:06AM -0400, Eric Paris wrote:
> On Thu, Sep 6, 2012 at 11:08 AM, Dave Jones <davej@redhat.com> wrote:
> > Following on from the previous patch that fixed an oops, these
> > are all the other similar code patterns in the tree with the same
> > checks added. I never saw these causing problems, but checking
> > this everywhere seems to make more sense than every subsequent
> > routine that gets passed 'ab' having to check it.
> >
> > Later we could remove all those same checks from audit_log_format
> > and friends. For now, this just prevents similar bugs being introduced
> > as the one in my previous patch.
> >
> > Signed-off-by: Dave Jones <davej@redhat.com>
>
> Not certain because I haven't looked at what happens with the error
> code, but I think this might not be right. auditd can be explictly
> told not to audit certain events, in which case it is normal and
> expected that ab would come back NULL....
Ugh, that's a lot messier to have to audit every function that gets
passed 'ab' to make sure it has a NULL check, but ok I'll go look at it.
hopefully audit_log_link_denied was a one off.
Dave
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Check all returns from audit_log_start
2012-09-06 15:47 ` Dave Jones
@ 2012-09-06 15:56 ` Dave Jones
0 siblings, 0 replies; 10+ messages in thread
From: Dave Jones @ 2012-09-06 15:56 UTC (permalink / raw)
To: Eric Paris, Linux Kernel, Al Viro, eparis
On Thu, Sep 06, 2012 at 11:47:49AM -0400, Dave Jones wrote:
> > Not certain because I haven't looked at what happens with the error
> > code, but I think this might not be right. auditd can be explictly
> > told not to audit certain events, in which case it is normal and
> > expected that ab would come back NULL....
>
> Ugh, that's a lot messier to have to audit every function that gets
> passed 'ab' to make sure it has a NULL check, but ok I'll go look at it.
>
> hopefully audit_log_link_denied was a one off.
ok, from a quick look-over, that does seem to be the case.
This still feels like a nasty trap waiting for someone to walk into again though.
Dave
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: 3.6-rc4 audit_log_d_path oops.
2012-09-06 15:16 ` 3.6-rc4 audit_log_d_path oops Dave Jones
2012-09-06 15:34 ` Eric Paris
@ 2012-09-06 16:32 ` Kees Cook
2012-09-06 16:45 ` Dave Jones
1 sibling, 1 reply; 10+ messages in thread
From: Kees Cook @ 2012-09-06 16:32 UTC (permalink / raw)
To: Dave Jones, Linux Kernel, Al Viro, Kees Cook
On Thu, Sep 6, 2012 at 8:16 AM, Dave Jones <davej@redhat.com> wrote:
> On Thu, Sep 06, 2012 at 09:46:28AM -0400, Dave Jones wrote:
> > Hit this in overnight fuzz testing..
> >
> > BUG: unable to handle kernel NULL pointer dereference at 0000000000000020
> > IP: [<ffffffff81103365>] audit_log_d_path+0x35/0xf0
> > PGD 12fded067 PUD 142c06067 PMD 0
> > Oops: 0000 [#1] SMP
> > Modules linked in: tun fuse ipt_ULOG binfmt_misc nfnetlink nfc caif_socket caif phonet can llc2 pppoe pppox ppp_generic slhc irda crc_ccitt rds af_key decnet rose x25 atm netrom appletalk ipx p8023 psnap p8022 llc ax25 lockd sunrpc bluetooth rfkill ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 ip6table_filter ip6_tables nf_conntrack_ipv4 nf_defrag_ipv4 xt_state nf_conntrack kvm_intel kvm crc32c_intel ghash_clmulni_intel microcode pcspkr i2c_i801 e1000e uinput i915 video i2c_algo_bit drm_kms_helper drm i2c_core
> > CPU 5
> > Pid: 7007, comm: trinity-child5 Not tainted 3.6.0-rc4+ #36
> > RIP: 0010:[<ffffffff81103365>] [<ffffffff81103365>] audit_log_d_path+0x35/0xf0
> > RSP: 0018:ffff880116b33ec8 EFLAGS: 00010246
> > RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000035
> > RDX: ffffffff819dbf5d RSI: ffffffff819da5bb RDI: 0000000000000000
> > RBP: ffff880116b33ee8 R08: ffff88001942533e R09: 0000000000000000
> > R10: 0000000000000001 R11: 0000000000000000 R12: ffff880116b33f38
> > R13: ffff880116b33f38 R14: ffffffff819fa1eb R15: 0000000000000005
> > FS: 00007f5742581740(0000) GS:ffff880148600000(0000) knlGS:0000000000000000
> > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: 0000000000000020 CR3: 00000001046d3000 CR4: 00000000001407e0
> > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> > Process trinity-child5 (pid: 7007, threadinfo ffff880116b32000, task ffff880019424d00)
> > Stack:
> > ffff880116b33ee8 0000000000000000 ffff880116b33f38 ffff880019424d00
> > ffff880116b33f18 ffffffff81103522 00000000000001d0 ffff880119ed6500
> > 00000000000001d0 0000000000000109 ffff880116b33f78 ffffffff811ebb95
> > Call Trace:
> > [<ffffffff81103522>] audit_log_link_denied+0x92/0x100
> > [<ffffffff811ebb95>] sys_linkat+0x195/0x1e0
> > [<ffffffff813573de>] ? trace_hardirqs_on_thunk+0x3a/0x3f
> > [<ffffffff816a50ed>] system_call_fastpath+0x1a/0x1f
> > Code: 5d e8 4c 89 65 f0 4c 89 6d f8 0f 1f 44 00 00 48 85 f6 48 89 fb 49 89 d5 74 11 48 89 f2 31 c0 48 c7 c6 bb a5 9d 81 e8 8b e1 ff ff <8b> 73 20 40 f6 c6 01 75 62 48 8b 3d 33 33 93 01 48 85 ff 74 4e
> > RIP [<ffffffff81103365>] audit_log_d_path+0x35/0xf0
> > RSP <ffff880116b33ec8>
> > CR2: 0000000000000020
> > ---[ end trace 85b88c850143bb1c ]---
> >
> > That's here in kernel/audit.c
> >
> > 1433
> > 1434 /* We will allow 11 spaces for ' (deleted)' to be appended */
> > 1435 pathname = kmalloc(PATH_MAX+11, ab->gfp_mask);
> >
> > 'ab' is NULL.
> >
> > Looks like audit_log_link_denied needs to handle potential failure from
> > audit_log_start and abort early. (oddly, it looks like every other
> > function called there checks for !ab.)
> >
> > Maybe additional code should be added here to printk the audit message
> > to dmesg so that we don't lose it entirely, but for now, minimal fix.
> >
> > Signed-off-by: Dave Jones <davej@redhat.com>
> >
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index ea3b7b6..c3e85bb 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -1466,6 +1466,9 @@ void audit_log_link_denied(const char *operation, struct path *link)
> >
> > ab = audit_log_start(current->audit_context, GFP_KERNEL,
> > AUDIT_ANOM_LINK);
> > + if (!ab)
> > + return;
> > +
> > audit_log_format(ab, "op=%s action=denied", operation);
> > audit_log_format(ab, " pid=%d comm=", current->pid);
> > audit_log_untrustedstring(ab, current->comm);
This fix looks fine to me.
> Added Kees, as this was introduced in a51d9eaa41866ab6b4b6ecad7b621f8b66ece0dc
>
> I just realised, the funny thing about this is that the machine running that test
> had selinux/audit disabled. And yet here we are, screwing around with audit buffers.
The intent was to have this message show up in dmesg even if auditd
wasn't running, and even if the specific process wasn't being
explicitly audited.
> Should there be a test on audit_enable=0 in audit_log_link_denied() ?
>
> I'm now curious how much more of the audit code is getting run through similar lack of tests
What is the condition in which audit_log_start fails?
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: 3.6-rc4 audit_log_d_path oops.
2012-09-06 16:32 ` Kees Cook
@ 2012-09-06 16:45 ` Dave Jones
2012-09-06 17:50 ` Kees Cook
0 siblings, 1 reply; 10+ messages in thread
From: Dave Jones @ 2012-09-06 16:45 UTC (permalink / raw)
To: Kees Cook; +Cc: Linux Kernel, Al Viro
On Thu, Sep 06, 2012 at 09:32:49AM -0700, Kees Cook wrote:
> > I just realised, the funny thing about this is that the machine running that test
> > had selinux/audit disabled. And yet here we are, screwing around with audit buffers.
>
> The intent was to have this message show up in dmesg even if auditd
> wasn't running, and even if the specific process wasn't being
> explicitly audited.
>
> > Should there be a test on audit_enable=0 in audit_log_link_denied() ?
> >
> > I'm now curious how much more of the audit code is getting run through similar lack of tests
>
> What is the condition in which audit_log_start fails?
in the case of that oops, given I had booted with audit=0, I suspect it was hitting the first check...
1157 if (audit_initialized != AUDIT_INITIALIZED)
1158 return NULL;
Dave
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: 3.6-rc4 audit_log_d_path oops.
2012-09-06 16:45 ` Dave Jones
@ 2012-09-06 17:50 ` Kees Cook
0 siblings, 0 replies; 10+ messages in thread
From: Kees Cook @ 2012-09-06 17:50 UTC (permalink / raw)
To: Dave Jones, Kees Cook, Linux Kernel, Al Viro
On Thu, Sep 6, 2012 at 9:45 AM, Dave Jones <davej@redhat.com> wrote:
> On Thu, Sep 06, 2012 at 09:32:49AM -0700, Kees Cook wrote:
> > > I just realised, the funny thing about this is that the machine running that test
> > > had selinux/audit disabled. And yet here we are, screwing around with audit buffers.
> >
> > The intent was to have this message show up in dmesg even if auditd
> > wasn't running, and even if the specific process wasn't being
> > explicitly audited.
> >
> > > Should there be a test on audit_enable=0 in audit_log_link_denied() ?
> > >
> > > I'm now curious how much more of the audit code is getting run through similar lack of tests
> >
> > What is the condition in which audit_log_start fails?
>
> in the case of that oops, given I had booted with audit=0, I suspect it was hitting the first check...
>
> 1157 if (audit_initialized != AUDIT_INITIALIZED)
> 1158 return NULL;
Ah-ha, okay. Yeah, I'm fine with the fix you had. If _start fails, just return.
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-09-06 17:50 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-09-06 13:46 3.6-rc4 audit_log_d_path oops Dave Jones
2012-09-06 15:08 ` Check all returns from audit_log_start Dave Jones
2012-09-06 15:36 ` Eric Paris
2012-09-06 15:47 ` Dave Jones
2012-09-06 15:56 ` Dave Jones
2012-09-06 15:16 ` 3.6-rc4 audit_log_d_path oops Dave Jones
2012-09-06 15:34 ` Eric Paris
2012-09-06 16:32 ` Kees Cook
2012-09-06 16:45 ` Dave Jones
2012-09-06 17:50 ` Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).