Re: [PATCH V2] audit: normalize NETFILTER_PKT
Richard Guy Briggs <rgb(a)redhat.com> wrote:
Simplify and eliminate flipping in and out of message fields, relying
on nfmark
the way we do for audit_key.
+struct nfpkt_par {
+ int ipv;
+ const void *saddr;
+ const void *daddr;
+ u8 proto;
+};
This is problematic, see below for why.
-static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
+static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb, struct nfpkt_par
*apar)
{
struct iphdr _iph;
const struct iphdr *ih;
+ apar->ipv = 4;
ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
- if (!ih) {
- audit_log_format(ab, " truncated=1");
+ if (!ih)
return;
Removing this "truncated" has the consequence that this can later log
"saddr=0.0.0.0 daddr=0.0.0.0" if we return here.
This cannot happen for ip(6)tables because ip stack discards broken l3 headers
before the netfilter hooks get called, but its possible with NFPROTO_BRIDGE.
Perhaps you will need to change audit_ip4/6 to return "false" when it can't
get the l3 information now so we only log zero addresses when the packet
really did contain them.
- audit_log_format(ab, " saddr=%pI4 daddr=%pI4 ipid=%hu
proto=%hhu",
- &ih->saddr, &ih->daddr, ntohs(ih->id), ih->protocol);
Alternatively, one could keep this around. In fact, why is this (re)moved
in first place? This move of audit_log_format() seems to only reason
why *apar struct is required.
AFAICS this now does:
ab = new()
log(ab, mark);
audit_ip4(&apar);
log(&apar);
so might as well keep the log() call within the audit_ip4/6 function.
- audit_proto(ab, skb, ih->protocol, ih->ihl * 4);
+ apar->saddr = &ih->saddr;
+ apar->daddr = &ih->daddr;
+ apar->proto = ih->protocol;
}
Caution. skb_header_pointer() may copy from non-linear skb part
into _iph, which is on stack, so apar->saddr may be stale once
function returns. So if you really want to remove the audit_log_format()
of the saddr/daddr then you need to copy the ip addresses here.
(We guarantee its linear for ip stack but not for NFPROTO_BRIDGE and this function
is also called for the bridge version of the target).
static unsigned int
audit_tg(struct sk_buff *skb, const struct xt_action_param *par)
{
- const struct xt_audit_info *info = par->targinfo;
struct audit_buffer *ab;
+ struct nfpkt_par apar = {
+ -1, NULL, NULL, -1,
+ };
I suggest to use
struct nfpkt_par apar = {
.family = par->family,
};
if apar is required for some reason.
ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
if (ab == NULL)
goto errout;
- audit_log_format(ab, "action=%hhu hook=%u len=%u inif=%s outif=%s",
- info->type, par->hooknum, skb->len,
- par->in ? par->in->name : "?",
- par->out ? par->out->name : "?");
-
- if (skb->mark)
- audit_log_format(ab, " mark=%#x", skb->mark);
+ audit_log_format(ab, " mark=%#x", skb->mark ?: -1);
-1 will be logged as 0xffffffff, no? whats wrong with
audit_log_format(ab, " mark=%#x", skb->mark); ?
if (skb->dev && skb->dev->type == ARPHRD_ETHER) {
- audit_log_format(ab, " smac=%pM dmac=%pM macproto=0x%04x",
- eth_hdr(skb)->h_source, eth_hdr(skb)->h_dest,
- ntohs(eth_hdr(skb)->h_proto));
-
This ARPHDR_ETHER test is no longer needed after logging
of ether addresses is removed.