[patch] net/xfrm: fix crash in ipsec audit logging

[patch] net/xfrm: fix crash in ipsec audit logging

[Ingo Molnar]

Subject: [patch] net/xfrm: fix crash in ipsec audit logging
From: Ingo Molnar <mingo@elte.hu>

i got the following crash when restarting a (timed out) ipsec session on 
2.6.20-rc1-rt4:

BUG: unable to handle kernel NULL pointer dereference at virtual address 000000a2
 printing eip:
c0320f67
*pde = 00000000
stopped custom tracer.
Oops: 0000 [#1]
PREEMPT SMP 
Modules linked in: xfrm4_mode_tunnel deflate zlib_deflate twofish twofish_common serpent blowfish cbc ecb xcbc sha256 crypto_null aes des blkcipher xfrm4_tunnel tunnel4 ipcomp esp4 ah4 af_key radeon drm hidp l2cap bluetooth sunrpc ipv6 n_hdlc ppp_synctty ppp_async crc_ccitt ppp_generic slhc nf_conntrack_netbios_ns ipt_REJECT nf_conntrack_ipv4 xt_state xt_tcpudp iptable_filter ip_tables x_tables dm_mirror dm_multipath dm_mod raid1 video sbs i2c_ec button battery asus_acpi ac parport_pc lp parport floppy snd_via82xx_modem snd_seq_dummy snd_via82xx snd_ac97_codec snd_seq_oss snd_seq_midi_event ac97_bus bt878 tuner snd_seq bttv video_buf ir_common snd_pcm_oss snd_mixer_oss videodev v4l1_compat v4l2_common i2c_algo_bit snd_pcm btcx_risc tveeprom compat_ioctl32 pcspkr e100 skge mii i2c_viapro snd_timer gameport snd_mpu401_uart snd_rawmidi snd_seq_device snd_page_alloc snd i2c_core soundcore k8temp hwmon ide_cd serio_raw cdrom sata_via pata_via ata_generic libata sd_mod scsi_mod ext3 jbd ehci_hcd ohci_hcd uhci_hcd
CPU:    0
EIP:    0060:[<c0320f67>]    Not tainted VLI
EFLAGS: 00010246   (2.6.20-rc1.1.rt4.0006 #1)
EIP is at xfrm_audit_log+0x116/0x423
eax: 00000000   ebx: 00000586   ecx: 00000000   edx: c03dfc79
esi: 000001f4   edi: 00000000   ebp: f5229a48   esp: f522999c
ds: 007b   es: 007b   ss: 0068   preempt: 00000001
Process pluto (pid: 12885, ti=f5229000 task=f7c06af0 task.ti=f5229000)
Stack: f7c5be00 c03f6e49 000001f4 f3860a80 00000000 f52299bc c02ca13f 00000000 
       f52299dc c02ca247 00000006 f52299e4 c01254ba 00000001 e86fd000 00000000 
       f52299e4 c02ca27d f7c5be00 f8d928b9 f5229a10 c0155d14 f3860a80 fffffffd 
Call Trace:
 [<c0329c69>] xfrm_get_policy+0x108/0x24f
 [<c03274a9>] xfrm_user_rcv_msg+0x132/0x155
 [<c02e3f23>] netlink_run_queue+0x61/0xcd
 [<c0326eeb>] xfrm_netlink_rcv+0x27/0x3b
 [<c02e3fa2>] netlink_data_ready+0x13/0x54
 [<c02e1fcf>] netlink_sendskb+0x1f/0x37
 [<c02e3a55>] netlink_unicast+0x1aa/0x1b2
 [<c02e3cce>] netlink_sendmsg+0x271/0x27d
 [<c02c47b8>] sock_aio_write+0x104/0x110
 [<c017d703>] do_sync_write+0xb2/0xef
 [<c017dd46>] vfs_write+0xc5/0x165
 [<c017e51a>] sys_write+0x3d/0x61
 [<c0103ffd>] syscall_call+0x7/0xb
 [<457d78b2>] 0x457d78b2
 =======================
---------------------------
| preempt count: 00000001 ]
| 1-level deep critical section nesting:
----------------------------------------
.. [<c0334075>] .... __spin_lock_irqsave+0x25/0x5e
.....[<c01057db>] ..   ( <= die+0x42/0x201)

skipping trace printing on CPU#0 != -1
Code: 24 e8 3e 5b e3 ff eb 08 8b 45 9c e8 43 9f e3 ff 83 7d 0c 00 74 12 8b 4d 0c 0f b7 99 9c 00 00 00 8b 91 18 01 00 00 eb 10 8b 45 10 <0f> b7 98 a2 00 00 00 8b 90 d8 01 00 00 85 d2 74 29 8d 42 08 89 
EIP: [<c0320f67>] xfrm_audit_log+0x116/0x423 SS:ESP 0068:f522999c

the reason for the crash is that we pass both 'xp' and 'x' as NULL into 
xfrm_audit_log(), which thus has no other option but to crash.

The fix i think is to check for the presence of xp sooner and return 
with -ENOENT, and to pass in the 'delete' flag as the result to the 
audit subsystem, not the 'xp != NULL' boolen value.

(this is a new v2.6.19->v2.6.20-rc1 regression, i never had problems 
with this functionality of the kernel.)

Hopefully i got the fix right :-)

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 net/xfrm/xfrm_user.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Index: linux/net/xfrm/xfrm_user.c
===================================================================
--- linux.orig/net/xfrm/xfrm_user.c
+++ linux/net/xfrm/xfrm_user.c
@@ -1268,13 +1268,12 @@ static int xfrm_get_policy(struct sk_buf
 		xp = xfrm_policy_bysel_ctx(type, p->dir, &p->sel, tmp.security, delete);
 		security_xfrm_policy_free(&tmp);
 	}
-	if (delete)
-		xfrm_audit_log(NETLINK_CB(skb).loginuid, NETLINK_CB(skb).sid,
-			       AUDIT_MAC_IPSEC_DELSPD, (xp) ? 1 : 0, xp, NULL);
-
 	if (xp == NULL)
 		return -ENOENT;
 
+	xfrm_audit_log(NETLINK_CB(skb).loginuid, NETLINK_CB(skb).sid,
+		       AUDIT_MAC_IPSEC_DELSPD, delete, xp, NULL);
+
 	if (!delete) {
 		struct sk_buff *resp_skb;
 

Re: [patch] net/xfrm: fix crash in ipsec audit logging

[jamal]

On Tue, 2006-26-12 at 18:56 +0100, Ingo Molnar wrote:


> +	xfrm_audit_log(NETLINK_CB(skb).loginuid, NETLINK_CB(skb).sid,
> +		       AUDIT_MAC_IPSEC_DELSPD, delete, xp, NULL);
> +
>  	if (!delete) {
>  		struct sk_buff *resp_skb;


You could move the call into the else from above if (!delete) maybe?
Otherwise you have to add back the "if (delete)" check since that
function could be used to either retrieve (which is not subject to an
audit) or delete an xp.

cheers,
jamal
 

Re: [patch] net/xfrm: fix crash in ipsec audit logging

[Joy Latten]

On Tue, 2006-12-26 at 13:37 -0500, jamal wrote:
>On Tue, 2006-26-12 at 18:56 +0100, Ingo Molnar wrote:
> 
> 
> > +	xfrm_audit_log(NETLINK_CB(skb).loginuid, NETLINK_CB(skb).sid,
> > +		       AUDIT_MAC_IPSEC_DELSPD, delete, xp, NULL);
> > +
> >  	if (!delete) {
> >  		struct sk_buff *resp_skb;
> 
> 
> You could move the call into the else from above if (!delete) maybe?
> Otherwise you have to add back the "if (delete)" check since that
> function could be used to either retrieve (which is not subject to an
> audit) or delete an xp.
> 
> cheers,
> jamal
>  

My apologies as I am just reading my email.

Yes, I think in the else part of the "if (!delete)".

I also added a check to xfrm_audit_log() such that if both xfrm
and policy are NULL, we return. There isn't anything to audit
since we are only auditing creation and deletion of xfrm and 
policy.

Ingo, could you try this patch and let me know if everything works ok
for you. I have built and test in my environment, but not tested as
you are using it.  

Regards,
Joy

Signed-off-by: Joy Latten <latten@austin.ibm.com>

--------------------------------------------------------------------------

diff -urpN linux-2.6.19.orig/net/xfrm/xfrm_policy.c linux-2.6.19/net/xfrm/xfrm_policy.c
--- linux-2.6.19.orig/net/xfrm/xfrm_policy.c	2007-01-02 14:24:14.000000000 -0600
+++ linux-2.6.19/net/xfrm/xfrm_policy.c	2007-01-02 14:28:24.000000000 -0600
@@ -2003,6 +2003,9 @@ void xfrm_audit_log(uid_t auid, u32 sid,
 	if (audit_enabled == 0)
 		return;
 
+	if ((x == NULL) && (xp == NULL))
+		return;
+
 	audit_buf = audit_log_start(current->audit_context, GFP_ATOMIC, type);
 	if (audit_buf == NULL)
 	return;
diff -urpN linux-2.6.19.orig/net/xfrm/xfrm_user.c linux-2.6.19/net/xfrm/xfrm_user.c
--- linux-2.6.19.orig/net/xfrm/xfrm_user.c	2007-01-02 14:24:14.000000000 -0600
+++ linux-2.6.19/net/xfrm/xfrm_user.c	2007-01-02 14:28:14.000000000 -0600
@@ -1268,10 +1268,6 @@ static int xfrm_get_policy(struct sk_buf
 		xp = xfrm_policy_bysel_ctx(type, p->dir, &p->sel, tmp.security, delete);
 		security_xfrm_policy_free(&tmp);
 	}
-	if (delete)
-		xfrm_audit_log(NETLINK_CB(skb).loginuid, NETLINK_CB(skb).sid,
-			       AUDIT_MAC_IPSEC_DELSPD, (xp) ? 1 : 0, xp, NULL);
-
 	if (xp == NULL)
 		return -ENOENT;
 
@@ -1289,6 +1285,10 @@ static int xfrm_get_policy(struct sk_buf
 	} else {
 		if ((err = security_xfrm_policy_delete(xp)) != 0)
 			goto out;
+
+		xfrm_audit_log(NETLINK_CB(skb).loginuid, NETLINK_CB(skb).sid,
+			       AUDIT_MAC_IPSEC_DELSPD, (xp) ? 1 : 0, xp, NULL);
+
 		c.data.byid = p->index;
 		c.event = nlh->nlmsg_type;
 		c.seq = nlh->nlmsg_seq;