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
| ||
|
Date: Wed, 23 Dec 2020 18:59:10 +0100 From: Horatiu Vultur <horatiu.vultur@...rochip.com> To: Rasmus Villemoes <rasmus.villemoes@...vas.dk> CC: <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>, Jakub Kicinski <kuba@...nel.org>, "David S. Miller" <davem@...emloft.net> Subject: Re: [PATCH net 1/2] net: mrp: fix definitions of MRP test packets The 12/23/2020 15:45, Rasmus Villemoes wrote: Hi Rasmus, > > Wireshark says that the MRP test packets cannot be decoded - and the > reason for that is that there's a two-byte hole filled with garbage > between the "transitions" and "timestamp" members. > > So Wireshark decodes the two garbage bytes and the top two bytes of > the timestamp written by the kernel as the timestamp value (which thus > fluctuates wildly), and interprets the lower two bytes of the > timestamp as a new (type, length) pair, which is of course broken. > > While my copy of the MRP standard is still under way [*], I cannot > imagine the standard specifying a two-byte hole here, and whoever > wrote the Wireshark decoding code seems to agree with that. > > The struct definitions live under include/uapi/, but they are not > really part of any kernel<->userspace API/ABI, so fixing the > definitions by adding the packed attribute should not cause any > compatibility issues. > > The remaining on-the-wire packet formats likely also don't contain > holes, but pahole and manual inspection says the current definitions > suffice. So adding the packed attribute to those is not strictly > needed, but might be done for good measure. > > [*] I will never understand how something hidden behind a +1000$ > paywall can be called a standard. > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@...vas.dk> > --- > include/uapi/linux/mrp_bridge.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/uapi/linux/mrp_bridge.h b/include/uapi/linux/mrp_bridge.h > index 6aeb13ef0b1e..d1d0cf65916d 100644 > --- a/include/uapi/linux/mrp_bridge.h > +++ b/include/uapi/linux/mrp_bridge.h > @@ -96,7 +96,7 @@ struct br_mrp_ring_test_hdr { > __be16 state; > __be16 transitions; > __be32 timestamp; > -}; > +} __attribute__((__packed__)); Yes, I agree that this should be packed but it also needs to be 32 bit alligned, so extra 2 bytes are needed. The same will apply also for 'br_mrp_ring_topo_hdr' > > struct br_mrp_ring_topo_hdr { > __be16 prio; > @@ -141,7 +141,7 @@ struct br_mrp_in_test_hdr { > __be16 state; > __be16 transitions; > __be32 timestamp; > -}; > +} __attribute__((__packed__)); > > struct br_mrp_in_topo_hdr { > __u8 sa[ETH_ALEN]; > -- > 2.23.0 > -- /Horatiu
Powered by blists - more mailing lists