On Fri, 2007-12-07 at 12:11 -0500, Paul Moore wrote:
> This patch fixes a number of small but potentially troublesome things in the
> XFRM/IPsec code:
>
> * Use the 'audit_enabled' variable already in include/linux/audit.h
> Removed the need for extern declarations local to each XFRM audit fuction
>
> * Convert 'sid' to 'secid'
> The 'sid' name is specific to SELinux, 'secid' is the common naming
> convention used by the kernel when refering to tokenized LSM labels
>
> * Convert address display to use standard NIP* macros
> Similar to what was recently done with the SPD audit code, this also
> includes the removal of some unnecessary memcpy() calls
>
> * Move common code to xfrm_audit_common_stateinfo()
> Code consolidation from the "less is more" book on software development
>
> * Convert the SPI in audit records to host byte order
> The current SPI values in the audit record are being displayed in
> network byte order, probably not what was intended
>
> * Proper spacing around commas in function arguments
> Minor style tweak since I was already touching the code
>
> Signed-off-by: Paul Moore <paul.moore@xxxxxx>
Acked-by: Eric Paris <eparis@xxxxxxxxxx>
although it does make me wonder why audit_log_start doesn't just check
audit_enabled itself.... Anyway, this patch looks good.
> ---
>
> include/linux/xfrm.h | 2 +
> include/net/xfrm.h | 18 ++++++------
> net/xfrm/xfrm_policy.c | 15 +++++-----
> net/xfrm/xfrm_state.c | 69 +++++++++++++++++++++--------------------------
> security/selinux/xfrm.c | 20 +++++++-------
> 5 files changed, 58 insertions(+), 66 deletions(-)
>
> diff --git a/include/linux/xfrm.h b/include/linux/xfrm.h
> index b58adc5..f75a337 100644
> --- a/include/linux/xfrm.h
> +++ b/include/linux/xfrm.h
> @@ -31,7 +31,7 @@ struct xfrm_sec_ctx {
> __u8 ctx_doi;
> __u8 ctx_alg;
> __u16 ctx_len;
> - __u32 ctx_sid;
> + __u32 ctx_secid;
> char ctx_str[0];
> };
>
> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> index 58dfa82..c02e230 100644
> --- a/include/net/xfrm.h
> +++ b/include/net/xfrm.h
> @@ -462,7 +462,7 @@ struct xfrm_audit
> };
>
> #ifdef CONFIG_AUDITSYSCALL
> -static inline struct audit_buffer *xfrm_audit_start(u32 auid, u32 sid)
> +static inline struct audit_buffer *xfrm_audit_start(u32 auid, u32 secid)
> {
> struct audit_buffer *audit_buf = NULL;
> char *secctx;
> @@ -475,8 +475,8 @@ static inline struct audit_buffer *xfrm_audit_start(u32 auid, u32 sid)
>
> audit_log_format(audit_buf, "auid=%u", auid);
>
> - if (sid != 0 &&
> - security_secid_to_secctx(sid, &secctx, &secctx_len) == 0) {
> + if (secid != 0 &&
> + security_secid_to_secctx(secid, &secctx, &secctx_len) == 0) {
> audit_log_format(audit_buf, " subj=%s", secctx);
> security_release_secctx(secctx, secctx_len);
> } else
> @@ -485,13 +485,13 @@ static inline struct audit_buffer *xfrm_audit_start(u32 auid, u32 sid)
> }
>
> extern void xfrm_audit_policy_add(struct xfrm_policy *xp, int result,
> - u32 auid, u32 sid);
> + u32 auid, u32 secid);
> extern void xfrm_audit_policy_delete(struct xfrm_policy *xp, int result,
> - u32 auid, u32 sid);
> + u32 auid, u32 secid);
> extern void xfrm_audit_state_add(struct xfrm_state *x, int result,
> - u32 auid, u32 sid);
> + u32 auid, u32 secid);
> extern void xfrm_audit_state_delete(struct xfrm_state *x, int result,
> - u32 auid, u32 sid);
> + u32 auid, u32 secid);
> #else
> #define xfrm_audit_policy_add(x, r, a, s) do { ; } while (0)
> #define xfrm_audit_policy_delete(x, r, a, s) do { ; } while (0)
> @@ -621,13 +621,13 @@ extern int xfrm_selector_match(struct xfrm_selector *sel, struct flowi *fl,
>
> #ifdef CONFIG_SECURITY_NETWORK_XFRM
> /* If neither has a context --> match
> - * Otherwise, both must have a context and the sids, doi, alg must match
> + * Otherwise, both must have a context and the secids, doi, alg must match
> */
> static inline int xfrm_sec_ctx_match(struct xfrm_sec_ctx *s1, struct xfrm_sec_ctx *s2)
> {
> return ((!s1 && !s2) ||
> (s1 && s2 &&
> - (s1->ctx_sid == s2->ctx_sid) &&
> + (s1->ctx_secid == s2->ctx_secid) &&
> (s1->ctx_doi == s2->ctx_doi) &&
> (s1->ctx_alg == s2->ctx_alg)));
> }
> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index b702bd8..75f25c4 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -23,6 +23,7 @@
> #include <linux/netfilter.h>
> #include <linux/module.h>
> #include <linux/cache.h>
> +#include <linux/audit.h>
> #include <net/xfrm.h>
> #include <net/ip.h>
>
> @@ -2150,15 +2151,14 @@ static inline void xfrm_audit_common_policyinfo(struct xfrm_policy *xp,
> }
> }
>
> -void
> -xfrm_audit_policy_add(struct xfrm_policy *xp, int result, u32 auid, u32 sid)
> +void xfrm_audit_policy_add(struct xfrm_policy *xp, int result,
> + u32 auid, u32 secid)
> {
> struct audit_buffer *audit_buf;
> - extern int audit_enabled;
>
> if (audit_enabled == 0)
> return;
> - audit_buf = xfrm_audit_start(sid, auid);
> + audit_buf = xfrm_audit_start(secid, auid);
> if (audit_buf == NULL)
> return;
> audit_log_format(audit_buf, " op=SPD-add res=%u", result);
> @@ -2167,15 +2167,14 @@ xfrm_audit_policy_add(struct xfrm_policy *xp, int result, u32 auid, u32 sid)
> }
> EXPORT_SYMBOL_GPL(xfrm_audit_policy_add);
>
> -void
> -xfrm_audit_policy_delete(struct xfrm_policy *xp, int result, u32 auid, u32 sid)
> +void xfrm_audit_policy_delete(struct xfrm_policy *xp, int result,
> + u32 auid, u32 secid)
> {
> struct audit_buffer *audit_buf;
> - extern int audit_enabled;
>
> if (audit_enabled == 0)
> return;
> - audit_buf = xfrm_audit_start(sid, auid);
> + audit_buf = xfrm_audit_start(secid, auid);
> if (audit_buf == NULL)
> return;
> audit_log_format(audit_buf, " op=SPD-delete res=%u", result);
> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index cf43c49..b291a82 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -19,6 +19,7 @@
> #include <linux/ipsec.h>
> #include <linux/module.h>
> #include <linux/cache.h>
> +#include <linux/audit.h>
> #include <asm/uaccess.h>
>
> #include "xfrm_hash.h"
> @@ -1997,67 +1998,59 @@ void __init xfrm_state_init(void)
> static inline void xfrm_audit_common_stateinfo(struct xfrm_state *x,
> struct audit_buffer *audit_buf)
> {
> - if (x->security)
> - audit_log_format(audit_buf, " sec_alg=%u sec_doi=%u sec_obj=%s",
> - x->security->ctx_alg, x->security->ctx_doi,
> - x->security->ctx_str);
> + struct xfrm_sec_ctx *ctx = x->security;
> + u32 spi = ntohl(x->id.spi);
>
> - switch(x->props.family) {
> - case AF_INET:
> - audit_log_format(audit_buf, " src=%u.%u.%u.%u dst=%u.%u.%u.%u",
> - NIPQUAD(x->props.saddr.a4),
> - NIPQUAD(x->id.daddr.a4));
> - break;
> - case AF_INET6:
> - {
> - struct in6_addr saddr6, daddr6;
> -
> - memcpy(&saddr6, x->props.saddr.a6,
> - sizeof(struct in6_addr));
> - memcpy(&daddr6, x->id.daddr.a6,
> - sizeof(struct in6_addr));
> - audit_log_format(audit_buf,
> - " src=" NIP6_FMT " dst=" NIP6_FMT,
> - NIP6(saddr6), NIP6(daddr6));
> - }
> - break;
> - }
> + if (ctx)
> + audit_log_format(audit_buf, " sec_alg=%u sec_doi=%u sec_obj=%s",
> + ctx->ctx_alg, ctx->ctx_doi, ctx->ctx_str);
> +
> + switch(x->props.family) {
> + case AF_INET:
> + audit_log_format(audit_buf,
> + " src=" NIPQUAD_FMT " dst=" NIPQUAD_FMT,
> + NIPQUAD(x->props.saddr.a4),
> + NIPQUAD(x->id.daddr.a4));
> + break;
> + case AF_INET6:
> + audit_log_format(audit_buf,
> + " src=" NIP6_FMT " dst=" NIP6_FMT,
> + NIP6(*(struct in6_addr *)x->props.saddr.a6),
> + NIP6(*(struct in6_addr *)x->id.daddr.a6));
> + break;
> + }
> +
> + audit_log_format(audit_buf, " spi=%u(0x%x)", spi, spi);
> }
>
> -void
> -xfrm_audit_state_add(struct xfrm_state *x, int result, u32 auid, u32 sid)
> +void xfrm_audit_state_add(struct xfrm_state *x, int result,
> + u32 auid, u32 secid)
> {
> struct audit_buffer *audit_buf;
> - extern int audit_enabled;
>
> if (audit_enabled == 0)
> return;
> - audit_buf = xfrm_audit_start(sid, auid);
> + audit_buf = xfrm_audit_start(secid, auid);
> if (audit_buf == NULL)
> return;
> - audit_log_format(audit_buf, " op=SAD-add res=%u",result);
> + audit_log_format(audit_buf, " op=SAD-add res=%u", result);
> xfrm_audit_common_stateinfo(x, audit_buf);
> - audit_log_format(audit_buf, " spi=%lu(0x%lx)",
> - (unsigned long)x->id.spi, (unsigned long)x->id.spi);
> audit_log_end(audit_buf);
> }
> EXPORT_SYMBOL_GPL(xfrm_audit_state_add);
>
> -void
> -xfrm_audit_state_delete(struct xfrm_state *x, int result, u32 auid, u32 sid)
> +void xfrm_audit_state_delete(struct xfrm_state *x, int result,
> + u32 auid, u32 secid)
> {
> struct audit_buffer *audit_buf;
> - extern int audit_enabled;
>
> if (audit_enabled == 0)
> return;
> - audit_buf = xfrm_audit_start(sid, auid);
> + audit_buf = xfrm_audit_start(secid, auid);
> if (audit_buf == NULL)
> return;
> - audit_log_format(audit_buf, " op=SAD-delete res=%u",result);
> + audit_log_format(audit_buf, " op=SAD-delete res=%u", result);
> xfrm_audit_common_stateinfo(x, audit_buf);
> - audit_log_format(audit_buf, " spi=%lu(0x%lx)",
> - (unsigned long)x->id.spi, (unsigned long)x->id.spi);
> audit_log_end(audit_buf);
> }
> EXPORT_SYMBOL_GPL(xfrm_audit_state_delete);
> diff --git a/security/selinux/xfrm.c b/security/selinux/xfrm.c
> index e076039..c925880 100644
> --- a/security/selinux/xfrm.c
> +++ b/security/selinux/xfrm.c
> @@ -85,7 +85,7 @@ int selinux_xfrm_policy_lookup(struct xfrm_policy *xp, u32 fl_secid, u8 dir)
> if (!selinux_authorizable_ctx(ctx))
> return -EINVAL;
>
> - sel_sid = ctx->ctx_sid;
> + sel_sid = ctx->ctx_secid;
> }
> else
> /*
> @@ -132,7 +132,7 @@ int selinux_xfrm_state_pol_flow_match(struct xfrm_state *x, struct xfrm_policy *
> /* Not a SELinux-labeled SA */
> return 0;
>
> - state_sid = x->security->ctx_sid;
> + state_sid = x->security->ctx_secid;
>
> if (fl->secid != state_sid)
> return 0;
> @@ -175,13 +175,13 @@ int selinux_xfrm_decode_session(struct sk_buff *skb, u32 *sid, int ckall)
> struct xfrm_sec_ctx *ctx = x->security;
>
> if (!sid_set) {
> - *sid = ctx->ctx_sid;
> + *sid = ctx->ctx_secid;
> sid_set = 1;
>
> if (!ckall)
> break;
> }
> - else if (*sid != ctx->ctx_sid)
> + else if (*sid != ctx->ctx_secid)
> return -EINVAL;
> }
> }
> @@ -232,7 +232,7 @@ static int selinux_xfrm_sec_ctx_alloc(struct xfrm_sec_ctx **ctxp,
> ctx->ctx_str[str_len] = 0;
> rc = security_context_to_sid(ctx->ctx_str,
> str_len,
> - &ctx->ctx_sid);
> + &ctx->ctx_secid);
>
> if (rc)
> goto out;
> @@ -240,7 +240,7 @@ static int selinux_xfrm_sec_ctx_alloc(struct xfrm_sec_ctx **ctxp,
> /*
> * Does the subject have permission to set security context?
> */
> - rc = avc_has_perm(tsec->sid, ctx->ctx_sid,
> + rc = avc_has_perm(tsec->sid, ctx->ctx_secid,
> SECCLASS_ASSOCIATION,
> ASSOCIATION__SETCONTEXT, NULL);
> if (rc)
> @@ -264,7 +264,7 @@ not_from_user:
>
> ctx->ctx_doi = XFRM_SC_DOI_LSM;
> ctx->ctx_alg = XFRM_SC_ALG_SELINUX;
> - ctx->ctx_sid = sid;
> + ctx->ctx_secid = sid;
> ctx->ctx_len = str_len;
> memcpy(ctx->ctx_str,
> ctx_str,
> @@ -341,7 +341,7 @@ int selinux_xfrm_policy_delete(struct xfrm_policy *xp)
> int rc = 0;
>
> if (ctx)
> - rc = avc_has_perm(tsec->sid, ctx->ctx_sid,
> + rc = avc_has_perm(tsec->sid, ctx->ctx_secid,
> SECCLASS_ASSOCIATION,
> ASSOCIATION__SETCONTEXT, NULL);
>
> @@ -383,7 +383,7 @@ int selinux_xfrm_state_delete(struct xfrm_state *x)
> int rc = 0;
>
> if (ctx)
> - rc = avc_has_perm(tsec->sid, ctx->ctx_sid,
> + rc = avc_has_perm(tsec->sid, ctx->ctx_secid,
> SECCLASS_ASSOCIATION,
> ASSOCIATION__SETCONTEXT, NULL);
>
> @@ -412,7 +412,7 @@ int selinux_xfrm_sock_rcv_skb(u32 isec_sid, struct sk_buff *skb,
>
> if (x && selinux_authorizable_xfrm(x)) {
> struct xfrm_sec_ctx *ctx = x->security;
> - sel_sid = ctx->ctx_sid;
> + sel_sid = ctx->ctx_secid;
> break;
> }
> }
>
> --
> Linux-audit mailing list
> Linux-audit@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/linux-audit
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@xxxxxxxxxxxxx with
the words "unsubscribe selinux" without quotes as the message.
