Re: [PATCH] improved xfrm_audit_log() patch

2007-08-23 Thread Joy Latten 
On Wed, 2007-08-22 at 20:05 -0700, David Miller wrote:
 I would suggest, at this point, to make purpose built situation
 specific interfaces that pass specific objects (the ones being
 operated upon) to the audit layer.
 
 Let the audit layer pick out the bits it actually wants in the
 format it likes.
 
 For example, if we're creating a template, pass the policy and
 the templace to the audit layer via a function called:
 
 xfrm_audit_template_add()
 
 or something like that.  That function only needs two arguments.
 
 All of these call sites will rarely need more than 2 or 3 arguments in
 any given situation, and the on-stack audit thing will be gone too.
 
 This is the suggestion I made to you over a month ago, but you choose
 to do the on-stack thing.
 
I misunderstood. My bad.

For clarification, I plan on removing xfrm_audit_log() and replacing it
with more specific ipsec audit interfaces. 

For example, when auditing the addition of a policy, either
xfrm_user_audit_policy_add(xp, result, skb) or
pfkey_audit_policy_add(xp, result) will get called. 
I need two because xfrm_user gets loginuid/secid from netlink/skb
and pfkey gets it from audit_get_loginuid(). 
Each will setup and format audit buffer according
to what they want.

Also, for deleting, there will be pfkey_audit_policy_delete(xp, result)
and xfrm_user_audit_policy_delete(xp, result, skb).

 You must make this cost absolutely nothing when it is either
 not configured, and have next to no cost when not enabled at
 run time.  And it is very doable.

The new ipsec audit functions can be ifdef'd with CONFIG_AUDITSYSCALL
just as xfrm_audit_log() was so that there is no cost when 
audit is not configured. 

Let me know if this is better.

Regards,
Joy
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] improved xfrm_audit_log() patch

2007-08-23 Thread David Miller 
From: Joy Latten [EMAIL PROTECTED]
Date: Thu, 23 Aug 2007 12:15:10 -0500

 For example, when auditing the addition of a policy, either
 xfrm_user_audit_policy_add(xp, result, skb) or
 pfkey_audit_policy_add(xp, result) will get called. 
 I need two because xfrm_user gets loginuid/secid from netlink/skb
 and pfkey gets it from audit_get_loginuid(). 
 Each will setup and format audit buffer according
 to what they want.
 
 Also, for deleting, there will be pfkey_audit_policy_delete(xp, result)
 and xfrm_user_audit_policy_delete(xp, result, skb).

This sounds great.

How cheap is the auditing enabled test?  Perhaps it can
be even inlined into the xfrm audit hooks.
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] improved xfrm_audit_log() patch

2007-08-22 Thread David Miller 
From: David Miller [EMAIL PROTECTED]
Date: Tue, 21 Aug 2007 00:24:05 -0700 (PDT)

 Looks good, applied to net-2.6.24, thanks Joy.

Something is still buggered up in this patch, you can't add this local
audit_info variable unconditionally to these functions, and
alternatively you also can't add a bunch of ifdefs to xfrm_user.c to
cover it up either.

  CC [M]  net/xfrm/xfrm_user.o
net/xfrm/xfrm_user.c: In function $,1rx(Bxfrm_add_sa$,1ry(B:
net/xfrm/xfrm_user.c:450: warning: unused variable $,1rx(Baudit_info$,1ry(B
net/xfrm/xfrm_user.c: In function $,1rx(Bxfrm_del_sa$,1ry(B:
net/xfrm/xfrm_user.c:525: warning: unused variable $,1rx(Baudit_info$,1ry(B
net/xfrm/xfrm_user.c: In function $,1rx(Bxfrm_add_policy$,1ry(B:
net/xfrm/xfrm_user.c:1140: warning: unused variable $,1rx(Baudit_info$,1ry(B
net/xfrm/xfrm_user.c: In function $,1rx(Bxfrm_get_policy$,1ry(B:
net/xfrm/xfrm_user.c:1404: warning: unused variable $,1rx(Baudit_info$,1ry(B
net/xfrm/xfrm_user.c: In function $,1rx(Bxfrm_add_pol_expire$,1ry(B:
net/xfrm/xfrm_user.c:1651: warning: unused variable $,1rx(Baudit_info$,1ry(B
net/xfrm/xfrm_user.c: In function $,1rx(Bxfrm_add_sa_expire$,1ry(B:
net/xfrm/xfrm_user.c:1688: warning: unused variable $,1rx(Baudit_info$,1ry(B

So I'm going to revert for now.  Let me know when you have
a fixed version of the patch.

Thanks.
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] improved xfrm_audit_log() patch

2007-08-22 Thread Joy Latten 
On Wed, 2007-08-22 at 12:51 -0700, David Miller wrote:
 From: David Miller [EMAIL PROTECTED]
 Date: Tue, 21 Aug 2007 00:24:05 -0700 (PDT)
 
  Looks good, applied to net-2.6.24, thanks Joy.
 
 Something is still buggered up in this patch, you can't add this local
 audit_info variable unconditionally to these functions, and
 alternatively you also can't add a bunch of ifdefs to xfrm_user.c to
 cover it up either.
 
I wonder if I am subconsciously trying to break a record or 
something! My apologies as time is valuable. 

I mean to get this right. My rationale for using audit_info was to
reduce amount of arguments to xfrm_audit_log(). However, I now like
it better when I just called xfrm_audit_log(NETLINK_CB(skb).loginuid,
NETLINK_CB(skb).sid, ...). User determines where/how to get loginuid and
secid and nothing happens when AUDIT not configured. But would make
xfrm_audit_log() have 7 arguments instead of 6.

My alternative is to remove xfrm_get_auditinfo() out of the 
#ifdef CONFIG_AUDITSYSCALL and always fill in audit_info
regardless if AUDIT is configured or not. Less calls to
xfrm_audit_log() but perhaps unnecessary info when AUDIT 
not configured.

Would first solution be acceptable?

Joy   


-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] improved xfrm_audit_log() patch

2007-08-22 Thread David Miller 
From: Joy Latten [EMAIL PROTECTED]
Date: Wed, 22 Aug 2007 20:29:17 -0500

 On Wed, 2007-08-22 at 12:51 -0700, David Miller wrote:
  From: David Miller [EMAIL PROTECTED]
  Date: Tue, 21 Aug 2007 00:24:05 -0700 (PDT)
  
   Looks good, applied to net-2.6.24, thanks Joy.
  
  Something is still buggered up in this patch, you can't add this local
  audit_info variable unconditionally to these functions, and
  alternatively you also can't add a bunch of ifdefs to xfrm_user.c to
  cover it up either.
  
 I wonder if I am subconsciously trying to break a record or 
 something! My apologies as time is valuable. 
 
 I mean to get this right. My rationale for using audit_info was to
 reduce amount of arguments to xfrm_audit_log(). However, I now like
 it better when I just called xfrm_audit_log(NETLINK_CB(skb).loginuid,
 NETLINK_CB(skb).sid, ...). User determines where/how to get loginuid and
 secid and nothing happens when AUDIT not configured. But would make
 xfrm_audit_log() have 7 arguments instead of 6.
 
 My alternative is to remove xfrm_get_auditinfo() out of the 
 #ifdef CONFIG_AUDITSYSCALL and always fill in audit_info
 regardless if AUDIT is configured or not. Less calls to
 xfrm_audit_log() but perhaps unnecessary info when AUDIT 
 not configured.
 
 Would first solution be acceptable?

I don't like either of these ideas, sorry.

I would suggest, at this point, to make purpose built situation
specific interfaces that pass specific objects (the ones being
operated upon) to the audit layer.

Let the audit layer pick out the bits it actually wants in the
format it likes.

For example, if we're creating a template, pass the policy and
the templace to the audit layer via a function called:

xfrm_audit_template_add()

or something like that.  That function only needs two arguments.

All of these call sites will rarely need more than 2 or 3 arguments in
any given situation, and the on-stack audit thing will be gone too.

This is the suggestion I made to you over a month ago, but you choose
to do the on-stack thing.

You must make this cost absolutely nothing when it is either
not configured, and have next to no cost when not enabled at
run time.  And it is very doable.
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] improved xfrm_audit_log() patch

2007-08-21 Thread David Miller 
From: Joy Latten [EMAIL PROTECTED]
Date: Wed, 15 Aug 2007 11:16:29 -0500

 On Tue, 2007-08-07 at 18:32 -0700, David Miller wrote:
 From: Joy Latten [EMAIL PROTECTED]
 Date: Thu, 2 Aug 2007 15:56:47 -0500
 
  @@ -426,10 +426,15 @@ struct xfrm_audit
   };
   
   #ifdef CONFIG_AUDITSYSCALL
  -extern void xfrm_audit_log(uid_t auid, u32 secid, int type, int result,
  -  struct xfrm_policy *xp, struct xfrm_state *x);
  +extern void xfrm_audit_log(struct xfrm_audit audit_info, int result,
  + __be32 flowid, struct xfrm_policy *xp, 
  + struct xfrm_state *x, char *buf);
 
 Passing audit_info as an aggregate argument puts them into
 previous argument registers, or if they are not enough it
 goes either partially of wholly onto the stack, depending
 upon architecture.
 
 In fact you've made the argument register usage worse than
 in your previous revision. :-/
 
 Perhaps you meant to pass struct xfrm_audit * instead?
 
 Revised patch to pass pointer to struct xfrm_audit.
 Sorry, I missed that.
 
 Signed-off-by: Joy Latten [EMAIL PROTECTED]

Looks good, applied to net-2.6.24, thanks Joy.

-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] improved xfrm_audit_log() patch

2007-08-15 Thread Joy Latten 
On Tue, 2007-08-07 at 18:32 -0700, David Miller wrote:
From: Joy Latten [EMAIL PROTECTED]
Date: Thu, 2 Aug 2007 15:56:47 -0500

 @@ -426,10 +426,15 @@ struct xfrm_audit
  };
  
  #ifdef CONFIG_AUDITSYSCALL
 -extern void xfrm_audit_log(uid_t auid, u32 secid, int type, int result,
 -struct xfrm_policy *xp, struct xfrm_state *x);
 +extern void xfrm_audit_log(struct xfrm_audit audit_info, int result,
 +   __be32 flowid, struct xfrm_policy *xp, 
 +   struct xfrm_state *x, char *buf);

Passing audit_info as an aggregate argument puts them into
previous argument registers, or if they are not enough it
goes either partially of wholly onto the stack, depending
upon architecture.

In fact you've made the argument register usage worse than
in your previous revision. :-/

Perhaps you meant to pass struct xfrm_audit * instead?

Revised patch to pass pointer to struct xfrm_audit.
Sorry, I missed that.

Signed-off-by: Joy Latten [EMAIL PROTECTED]


diff -urpN linux-2.6.22/include/linux/audit.h 
linux-2.6.22.patch/include/linux/audit.h
--- linux-2.6.22/include/linux/audit.h  2007-08-14 18:13:53.0 -0500
+++ linux-2.6.22.patch/include/linux/audit.h2007-08-14 19:08:42.0 
-0500
@@ -112,6 +112,7 @@
 #define AUDIT_MAC_IPSEC_DELSA  1412/* Delete a XFRM state */
 #define AUDIT_MAC_IPSEC_ADDSPD 1413/* Add a XFRM policy */
 #define AUDIT_MAC_IPSEC_DELSPD 1414/* Delete a XFRM policy */
+#define AUDIT_MAC_IPSEC_EVENT  1415/* Audit IPSec events */
 
 #define AUDIT_FIRST_KERN_ANOM_MSG   1700
 #define AUDIT_LAST_KERN_ANOM_MSG1799
diff -urpN linux-2.6.22/include/net/xfrm.h linux-2.6.22.patch/include/net/xfrm.h
--- linux-2.6.22/include/net/xfrm.h 2007-08-14 18:13:53.0 -0500
+++ linux-2.6.22.patch/include/net/xfrm.h   2007-08-14 19:08:42.0 
-0500
@@ -426,10 +426,15 @@ struct xfrm_audit
 };
 
 #ifdef CONFIG_AUDITSYSCALL
-extern void xfrm_audit_log(uid_t auid, u32 secid, int type, int result,
-   struct xfrm_policy *xp, struct xfrm_state *x);
+extern void xfrm_audit_log(struct xfrm_audit *audit_info, int result,
+  __be32 flowid, struct xfrm_policy *xp, 
+  struct xfrm_state *x, char *buf);
+
+extern void xfrm_get_auditinfo(struct sk_buff *skb, 
+  struct xfrm_audit *audit_info);
 #else
-#define xfrm_audit_log(a,s,t,r,p,x) do { ; } while (0)
+#define xfrm_audit_log(a,r,f,p,s,b) do { ; } while (0)
+#define xfrm_get_auditinfo(s, a) do { ; } while (0)
 #endif /* CONFIG_AUDITSYSCALL */
 
 static inline void xfrm_pol_hold(struct xfrm_policy *policy)
diff -urpN linux-2.6.22/net/key/af_key.c linux-2.6.22.patch/net/key/af_key.c
--- linux-2.6.22/net/key/af_key.c   2007-08-14 18:13:53.0 -0500
+++ linux-2.6.22.patch/net/key/af_key.c 2007-08-14 19:08:42.0 -0500
@@ -1450,6 +1450,7 @@ static int pfkey_add(struct sock *sk, st
struct xfrm_state *x;
int err;
struct km_event c;
+   struct xfrm_audit audit_info;
 
x = pfkey_msg2xfrm_state(hdr, ext_hdrs);
if (IS_ERR(x))
@@ -1461,8 +1462,8 @@ static int pfkey_add(struct sock *sk, st
else
err = xfrm_state_update(x);
 
-   xfrm_audit_log(audit_get_loginuid(current-audit_context), 0,
-  AUDIT_MAC_IPSEC_ADDSA, err ? 0 : 1, NULL, x);
+   xfrm_get_auditinfo(0, audit_info);
+   xfrm_audit_log(audit_info, err ? 0 : 1, 0, 0, x, SAD-add);
 
if (err  0) {
x-km.state = XFRM_STATE_DEAD;
@@ -1487,6 +1488,7 @@ static int pfkey_delete(struct sock *sk,
struct xfrm_state *x;
struct km_event c;
int err;
+   struct xfrm_audit audit_info;
 
if (!ext_hdrs[SADB_EXT_SA-1] ||
!present_and_same_family(ext_hdrs[SADB_EXT_ADDRESS_SRC-1],
@@ -1515,8 +1517,9 @@ static int pfkey_delete(struct sock *sk,
c.event = XFRM_MSG_DELSA;
km_state_notify(x, c);
 out:
-   xfrm_audit_log(audit_get_loginuid(current-audit_context), 0,
-  AUDIT_MAC_IPSEC_DELSA, err ? 0 : 1, NULL, x);
+   xfrm_get_auditinfo(0, audit_info);
+   xfrm_audit_log(audit_info, err ? 0 : 1, 0, 0, x, SAD-delete);
+
xfrm_state_put(x);
 
return err;
@@ -1691,8 +1694,7 @@ static int pfkey_flush(struct sock *sk, 
if (proto == 0)
return -EINVAL;
 
-   audit_info.loginuid = audit_get_loginuid(current-audit_context);
-   audit_info.secid = 0;
+   xfrm_get_auditinfo(0, audit_info);
err = xfrm_state_flush(proto, audit_info);
if (err)
return err;
@@ -2182,6 +2184,7 @@ static int pfkey_spdadd(struct sock *sk,
struct xfrm_policy *xp;
struct km_event c;
struct sadb_x_sec_ctx *sec_ctx;
+   struct xfrm_audit audit_info;
 
if (!present_and_same_family(ext_hdrs[SADB_EXT_ADDRESS_SRC-1],

Re: [PATCH] improved xfrm_audit_log() patch

2007-08-07 Thread David Miller 
From: Joy Latten [EMAIL PROTECTED]
Date: Thu, 2 Aug 2007 15:56:47 -0500

 @@ -426,10 +426,15 @@ struct xfrm_audit
  };
  
  #ifdef CONFIG_AUDITSYSCALL
 -extern void xfrm_audit_log(uid_t auid, u32 secid, int type, int result,
 - struct xfrm_policy *xp, struct xfrm_state *x);
 +extern void xfrm_audit_log(struct xfrm_audit audit_info, int result,
 +__be32 flowid, struct xfrm_policy *xp, 
 +struct xfrm_state *x, char *buf);

Passing audit_info as an aggregate argument puts them into
previous argument registers, or if they are not enough it
goes either partially of wholly onto the stack, depending
upon architecture.

In fact you've made the argument register usage worse than
in your previous revision. :-/

Perhaps you meant to pass struct xfrm_audit * instead?
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] improved xfrm_audit_log() patch

2007-08-02 Thread Joy Latten 
Sorry for delay, here is xfrm_audit_log() modification with 
recommended changes. Let me know if this looks better. 

Regards,
Joy

Signed-off-by: Joy Latten [EMAIL PROTECTED]


diff -urpN linux-2.6.22/include/linux/audit.h 
linux-2.6.22.patch10/include/linux/audit.h
--- linux-2.6.22/include/linux/audit.h  2007-08-01 11:49:23.0 -0500
+++ linux-2.6.22.patch10/include/linux/audit.h  2007-08-01 13:11:14.0 
-0500
@@ -112,6 +112,7 @@
 #define AUDIT_MAC_IPSEC_DELSA  1412/* Delete a XFRM state */
 #define AUDIT_MAC_IPSEC_ADDSPD 1413/* Add a XFRM policy */
 #define AUDIT_MAC_IPSEC_DELSPD 1414/* Delete a XFRM policy */
+#define AUDIT_MAC_IPSEC_EVENT  1415/* Audit IPSec events */
 
 #define AUDIT_FIRST_KERN_ANOM_MSG   1700
 #define AUDIT_LAST_KERN_ANOM_MSG1799
diff -urpN linux-2.6.22/include/net/xfrm.h 
linux-2.6.22.patch10/include/net/xfrm.h
--- linux-2.6.22/include/net/xfrm.h 2007-08-01 11:49:24.0 -0500
+++ linux-2.6.22.patch10/include/net/xfrm.h 2007-08-01 13:11:14.0 
-0500
@@ -426,10 +426,15 @@ struct xfrm_audit
 };
 
 #ifdef CONFIG_AUDITSYSCALL
-extern void xfrm_audit_log(uid_t auid, u32 secid, int type, int result,
-   struct xfrm_policy *xp, struct xfrm_state *x);
+extern void xfrm_audit_log(struct xfrm_audit audit_info, int result,
+  __be32 flowid, struct xfrm_policy *xp, 
+  struct xfrm_state *x, char *buf);
+
+extern void xfrm_get_auditinfo(struct sk_buff *skb, 
+  struct xfrm_audit *audit_info);
 #else
-#define xfrm_audit_log(a,s,t,r,p,x) do { ; } while (0)
+#define xfrm_audit_log(a,r,f,p,s,b) do { ; } while (0)
+#define xfrm_get_auditinfo(s, a) do { ; } while (0)
 #endif /* CONFIG_AUDITSYSCALL */
 
 static inline void xfrm_pol_hold(struct xfrm_policy *policy)
@@ -975,7 +980,7 @@ struct xfrmk_spdinfo {
 
 extern struct xfrm_state *xfrm_find_acq_byseq(u32 seq);
 extern int xfrm_state_delete(struct xfrm_state *x);
-extern int xfrm_state_flush(u8 proto, struct xfrm_audit *audit_info);
+extern int xfrm_state_flush(u8 proto, struct xfrm_audit audit_info);
 extern void xfrm_sad_getinfo(struct xfrmk_sadinfo *si);
 extern void xfrm_spd_getinfo(struct xfrmk_spdinfo *si);
 extern int xfrm_replay_check(struct xfrm_state *x, __be32 seq);
@@ -1032,13 +1037,13 @@ struct xfrm_policy *xfrm_policy_bysel_ct
  struct xfrm_sec_ctx *ctx, int delete,
  int *err);
 struct xfrm_policy *xfrm_policy_byid(u8, int dir, u32 id, int delete, int 
*err);
-int xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
+int xfrm_policy_flush(u8 type, struct xfrm_audit audit_info);
 u32 xfrm_get_acqseq(void);
 void xfrm_alloc_spi(struct xfrm_state *x, __be32 minspi, __be32 maxspi);
 struct xfrm_state * xfrm_find_acq(u8 mode, u32 reqid, u8 proto,
  xfrm_address_t *daddr, xfrm_address_t *saddr,
  int create, unsigned short family);
-extern int xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
+extern int xfrm_policy_flush(u8 type, struct xfrm_audit audit_info);
 extern int xfrm_sk_policy_insert(struct sock *sk, int dir, struct xfrm_policy 
*pol);
 extern int xfrm_bundle_ok(struct xfrm_policy *pol, struct xfrm_dst *xdst,
  struct flowi *fl, int family, int strict);
diff -urpN linux-2.6.22/net/key/af_key.c linux-2.6.22.patch10/net/key/af_key.c
--- linux-2.6.22/net/key/af_key.c   2007-08-01 11:49:42.0 -0500
+++ linux-2.6.22.patch10/net/key/af_key.c   2007-08-01 13:14:01.0 
-0500
@@ -1447,6 +1447,7 @@ static int pfkey_add(struct sock *sk, st
struct xfrm_state *x;
int err;
struct km_event c;
+   struct xfrm_audit audit_info;
 
x = pfkey_msg2xfrm_state(hdr, ext_hdrs);
if (IS_ERR(x))
@@ -1458,8 +1459,8 @@ static int pfkey_add(struct sock *sk, st
else
err = xfrm_state_update(x);
 
-   xfrm_audit_log(audit_get_loginuid(current-audit_context), 0,
-  AUDIT_MAC_IPSEC_ADDSA, err ? 0 : 1, NULL, x);
+   xfrm_get_auditinfo(0, audit_info);
+   xfrm_audit_log(audit_info, err ? 0 : 1, 0, 0, x, SAD-add);
 
if (err  0) {
x-km.state = XFRM_STATE_DEAD;
@@ -1484,6 +1485,7 @@ static int pfkey_delete(struct sock *sk,
struct xfrm_state *x;
struct km_event c;
int err;
+   struct xfrm_audit audit_info;
 
if (!ext_hdrs[SADB_EXT_SA-1] ||
!present_and_same_family(ext_hdrs[SADB_EXT_ADDRESS_SRC-1],
@@ -1512,8 +1514,9 @@ static int pfkey_delete(struct sock *sk,
c.event = XFRM_MSG_DELSA;
km_state_notify(x, c);
 out:
-   xfrm_audit_log(audit_get_loginuid(current-audit_context), 0,
-  AUDIT_MAC_IPSEC_DELSA, err ? 0 : 1, NULL, x);
+   xfrm_get_auditinfo(0, audit_info);
+   xfrm_audit_log(audit_info, err ? 0 : 1, 0,