netdev - Re: [PATCH net] bridge: br_handle_local

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] [day] [month] [year] [list]

Message-ID: <20131217135542.21976b05@nehalam.linuxnetplumber.net>
Date:	Tue, 17 Dec 2013 13:55:42 -0800
From:	Stephen Hemminger <stephen@...workplumber.org>
To:	Martin Faecknitz <faecknitz@...splots.de>
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH net] bridge: br_handle_local_finish should not return
 zero

On Sat, 14 Dec 2013 21:52:08 +0100
Martin Faecknitz <faecknitz@...splots.de> wrote:

> br_handle_local_finish is called by NF_HOOK(...) after accepting the packet. If
> the return value of NF_HOOK(...) is zero (i.e. the return value of
> br_handle_local_finish), the packet is passed to the network stack. This
> behavior conflicts with netfilter hooks which return NF_STOLEN/NF_QUEUE. In this
> case, NF_HOOK(...) returns also zero (see nf_hook_slow) but
> br_handle_local_finish was never called. The packet will still passed to the
> network stack.
> 
> Signed-off-by: Martin Faecknitz
> 
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -149,7 +149,7 @@ static int br_handle_local_finish(struct sk_buff *skb)
>  	br_vlan_get_tag(skb, &vid);
>  	if (p->flags & BR_LEARNING)
>  		br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vid);
> -	return 0;	 /* process further */
> +	return 1;	 /* process further */
>  }
> 
>  /*
> @@ -208,7 +208,7 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
> 
>  		/* Deliver packet to local host only */
>  		if (NF_HOOK(NFPROTO_BRIDGE, NF_BR_LOCAL_IN, skb, skb->dev,
> -			    NULL, br_handle_local_finish)) {
> +			    NULL, br_handle_local_finish) != 1) {
>  			return RX_HANDLER_CONSUMED; /* consumed by filter */
>  		} else {
>  			*pskb = skb;
> --
> 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

I think the input code was incorrectly assuming the model of that receive logic
should return NET_RX_SUCCESS(0) or NET_RX_DROP(1). (Note DECNET has same bug).

Rather than hardcode in 0 and 1. why not use NF_DROP, NF_ACCEPT instead?


--
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