netdev - Re: [net-next PATCH 14/16] iptables socket match

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 02 Oct 2008 11:26:36 +0200
From:	Patrick McHardy <kaber@...sh.net>
To:	KOVACS Krisztian <hidden@....bme.hu>
CC:	David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
	netfilter-devel@...r.kernel.org
Subject: Re: [net-next PATCH 14/16] iptables socket match

KOVACS Krisztian wrote:
> Add iptables 'socket' match, which matches packets for which a TCP/UDP
> socket lookup succeeds.
>   

It seems sufficiently different from what xt_owner does to justify a 
separate module.
Minor nitpicking:

> +static int
> +extract_icmp_fields(const struct sk_buff *skb,
> +		    u8 *protocol,
> +		    __be32 *raddr,
> +		    __be32 *laddr,
> +		    __be16 *rport,
> +		    __be16 *lport)
> +{
> +	struct iphdr *outside_iph = ip_hdr(skb);
> +	struct iphdr *inside_iph, _inside_iph;
> +	struct icmphdr *icmph, _icmph;
> +	__be16 *ports, _ports[2];
> +
> +	icmph = skb_header_pointer(skb, outside_iph->ihl << 2, sizeof(_icmph), &_icmph);
>   

The "ihl << 2" is repeating multiple times. Maybe just store it in a 
variable,
also it would be nicer to use ip_hdrlen().

> +	if (icmph == NULL)
> +		return 1;
> +
> +	switch (icmph->type) {
> +	case ICMP_DEST_UNREACH:
> +	case ICMP_SOURCE_QUENCH:
> +	case ICMP_REDIRECT:
> +	case ICMP_TIME_EXCEEDED:
> +	case ICMP_PARAMETERPROB:
> +		break;
> +	default:
> +		return 1;
> +	}
> +
> +	inside_iph = skb_header_pointer(skb, (outside_iph->ihl << 2) + sizeof(struct icmphdr), sizeof(_inside_iph), &_inside_iph);
>   

And these lines (few more below) should break at 80 characters.

> +	if (inside_iph == NULL)
> +		return -EINVAL;
>   
What is the return convention here? It seems this should also return 1 
as the
other exit paths.

> +static bool
> +socket_mt(const struct sk_buff *skb,
> +	  const struct net_device *in,
> +	  const struct net_device *out,
> +	  const struct xt_match *match,
> +	  const void *matchinfo,
> +	  int offset,
> +	  unsigned int protoff,
> +	  bool *hotdrop)
> +{
>   
...
> +	if (iph->protocol == IPPROTO_UDP || iph->protocol == IPPROTO_TCP) {
> +		hp = skb_header_pointer(skb, ip_hdrlen(skb), sizeof(_hdr), &_hdr);
> +		if (hp == NULL)
> +			return false;
> +
> +		protocol = iph->protocol;
> +		saddr = iph->saddr;
> +		sport = hp->source;
> +		daddr = iph->daddr;
> +		dport = hp->dest;
> +
> +	}
> +	else if (iph->protocol == IPPROTO_ICMP) {
>   

Please put the else on the same line as the closing brace.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html