netdev - RE: [PATCH v2] packet: Deliver VLAN TPID to userspace

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 for Android: free password hash cracker in your pocket

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

Date:	Tue, 10 Dec 2013 12:59:15 -0000
From:	"David Laight" <David.Laight@...LAB.COM>
To:	"Atzm Watanabe" <atzm@...atosphere.co.jp>, <netdev@...r.kernel.org>
Cc:	"Stephen Hemminger" <stephen@...workplumber.org>,
	"Ben Hutchings" <bhutchings@...arflare.com>,
	"David Miller" <davem@...emloft.net>,
	"Daniel Borkmann" <dborkman@...hat.com>
Subject: RE: [PATCH v2] packet: Deliver VLAN TPID to userspace

> From: Atzm Watanabe
> Sent: 10 December 2013 12:41
> After the 802.1AD support, userspace packet receivers (packet dumper,
> software switch, and the like) need how to know VLAN TPID in order to
> reconstruct original tagged frame.
...
> -#define TP_STATUS_VLAN_VALID	(1 << 4) /* auxdata has valid tp_vlan_tci */
> +#define TP_STATUS_VLAN_VALID	(1 << 4) /* auxdata has valid tp_vlan_tci and tp_vlan_tpid */

How can userland (I presume) determine whether tp_vlan_tpid is valid?

...
> +struct tpacket_hdr_variant2 {
> +	__u32	tp_rxhash;
> +	__u32	tp_vlan_tci;
> +	__u32	tp_vlan_tpid;
> +};
> +
>  struct tpacket3_hdr {
>  	__u32		tp_next_offset;
>  	__u32		tp_sec;
> @@ -153,6 +159,7 @@ struct tpacket3_hdr {
>  	/* pkt_hdr variants */
>  	union {
>  		struct tpacket_hdr_variant1 hv1;
> +		struct tpacket_hdr_variant2 hv2;
>  	};
>  };

You've defined a new header variant, but all the code seems to rely
on the fact that the 'new' variant is identical to the old one
with the addition of an extra field at the end.

In which case it ought to be valid to just extend the old header variant.
If the header really can change format there ought to be a discriminating
member somewhere - which you don't seem to have changed.
 
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 9d70f13..3c75878 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -975,11 +975,15 @@ static void prb_clear_rxhash(struct tpacket_kbdq_core *pkc,
>  static void prb_fill_vlan_info(struct tpacket_kbdq_core *pkc,
>  			struct tpacket3_hdr *ppd)
>  {
> +	BUILD_BUG_ON(TPACKET_ALIGN(sizeof(*ppd)) != 48);

I'd add a comment about why check matters.
(ie the fact that it can safely grow until that is no longer true.)

	David


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