Richard Guy Briggs <rgb@xxxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
