Re: [PATCH] XFRM: assorted IPsec fixups

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(a)hp.com&gt;

--- 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(a)redhat.com https://www.redhat.com/mailman/listinfo/linux-audit