Re: [PATCH 1/1]:reworked ipsec audit

[Steve Grubb <sgrubb@redhat.com>]

On Friday 10 November 2006 16:40, Joy Latten wrote:
> diff -urpN linux-2.6.18.ppc64.orig/include/net/xfrm.h
> linux-2.6.18.ppc64.patch/include/net/xfrm.h ---
> linux-2.6.18.ppc64.orig/include/net/xfrm.h	2006-11-02 09:17:55.000000000
> -0600 +++ linux-2.6.18.ppc64.patch/include/net/xfrm.h	2006-11-05
> 20:16:05.000000000 -0600 @@ -371,9 +371,17 @@ struct xfrm_mgr
>  extern int xfrm_register_km(struct xfrm_mgr *km);
>  extern int xfrm_unregister_km(struct xfrm_mgr *km);
>
> -
>  extern struct xfrm_policy *xfrm_policy_list[XFRM_POLICY_MAX*2];
>
> +/* Audit Information */
> +struct xfrm_audit
> +{
> +	uid_t	loginuid;
> +	u32	secid;
> +};
> +void xfrm_audit_log(uid_t auid, u32 secid, int type, int result,
> +			   struct xfrm_policy *xp, struct xfrm_state *x);
> +
>  static inline void xfrm_pol_hold(struct xfrm_policy *policy)
>  {
>  	if (likely(policy != NULL))
> @@ -904,7 +912,7 @@ extern int xfrm_state_update(struct xfrm
>  extern struct xfrm_state *xfrm_state_lookup(xfrm_address_t *daddr, u32
> spi, u8 proto, unsigned short family); extern struct xfrm_state
> *xfrm_find_acq_byseq(u32 seq);
>  extern int xfrm_state_delete(struct xfrm_state *x);
> -extern void xfrm_state_flush(u8 proto);
> +extern void xfrm_state_flush(u8 proto, struct xfrm_audit audit_info);

This seems to indicate passing the structure by value...did you mean to do it 
that way or by pointer?

> @@ -952,13 +960,12 @@ int xfrm_policy_insert(int dir, struct x
>  struct xfrm_policy *xfrm_policy_bysel_ctx(int dir, struct xfrm_selector
> *sel, struct xfrm_sec_ctx *ctx, int delete);
>  struct xfrm_policy *xfrm_policy_byid(int dir, u32 id, int delete);
> -void xfrm_policy_flush(void);
>  u32 xfrm_get_acqseq(void);
>  void xfrm_alloc_spi(struct xfrm_state *x, u32 minspi, u32 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 void xfrm_policy_flush(void);
> +extern void xfrm_policy_flush(struct xfrm_audit audit_info);

Another pass by value...

> diff -urpN linux-2.6.18.ppc64.orig/net/key/af_key.c
> linux-2.6.18.ppc64.patch/net/key/af_key.c ---
> linux-2.6.18.ppc64.orig/net/key/af_key.c	2006-11-02 09:16:11.000000000
> -0600 +++ linux-2.6.18.ppc64.patch/net/key/af_key.c	2006-11-08
> @@ -1637,12 +1645,14 @@ static int pfkey_flush(struct sock *sk,
>  {
>  	unsigned proto;
>  	struct km_event c;
> +	struct xfrm_audit audit_info;
>
>  	proto = pfkey_satype2proto(hdr->sadb_msg_satype);
>  	if (proto == 0)
>  		return -EINVAL;
>
> -	xfrm_state_flush(proto);
> +	audit_info.loginuid = audit_get_loginuid(current->audit_context);
> +	xfrm_state_flush(proto, audit_info);

So, audit_info.secid is unset at this point....

> @@ -2404,8 +2420,10 @@ static int key_notify_policy_flush(struc
>  static int pfkey_spdflush(struct sock *sk, struct sk_buff *skb, struct
> sadb_msg *hdr, void **ext_hdrs) {
>  	struct km_event c;
> +	struct xfrm_audit audit_info;
>
> -	xfrm_policy_flush();
> +	audit_info.loginuid = audit_get_loginuid(current->audit_context);
> +	xfrm_policy_flush(audit_info);

audit_info.secid is unset

> diff -urpN linux-2.6.18.ppc64.orig/net/xfrm/xfrm_policy.c
> linux-2.6.18.ppc64.patch/net/xfrm/xfrm_policy.c ---
> linux-2.6.18.ppc64.orig/net/xfrm/xfrm_policy.c	2006-11-02
> 09:16:10.000000000 -0600 +++
> linux-2.6.18.ppc64.patch/net/xfrm/xfrm_policy.c	2006-11-08
> @@ -1366,6 +1369,106 @@ int xfrm_bundle_ok(struct xfrm_policy *p
>
>  EXPORT_SYMBOL(xfrm_bundle_ok);
>
> +/* Audit addition and deletion of SAs and ipsec policy */
> +
> +void xfrm_audit_log(uid_t auid, u32 sid, int type, int result,
> +		    struct xfrm_policy *xp, struct xfrm_state *x)
> +{
> +
> +	char *secctx;
> +	u32 secctx_len;
> +	struct xfrm_sec_ctx *sctx = NULL;
> +	struct in6_addr saddr6, daddr6;
> +	struct in_addr saddr, daddr;

One of these two will be wasted because it will be either IPv4 or 6 but never 
both. I'd consider declaring these within the switch/case block that actually 
uses it or maybe use the non-specific struct sockaddr_storage.

>+      struct audit_buffer *audit_buf;
>+      int family;
>+       
>+
>+      audit_buf = audit_log_start(current->audit_context, GFP_ATOMIC, type);
>+      if (audit_buf == NULL)
>+              return;
>+

Also, before calling audit_log_start...you need to see if audit is enabled. 
You should declare an "extern int audit_enabled;" within this function and do 
a "if (audit_enabled == 0)  return;".

-Steve