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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sun, 3 Jan 2021 14:29:50 +0100 From: Horatiu Vultur <horatiu.vultur@...rochip.com> To: Jakub Kicinski <kuba@...nel.org> CC: Rasmus Villemoes <rasmus.villemoes@...vas.dk>, <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>, "David S. Miller" <davem@...emloft.net> Subject: Re: [PATCH net 1/2] net: mrp: fix definitions of MRP test packets The 12/28/2020 14:24, Jakub Kicinski wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Wed, 23 Dec 2020 15:45:32 +0100 Rasmus Villemoes wrote: > > 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__)); > > > > 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]; > > Can we use this opportunity to move the definitions of these structures > out of the uAPI to a normal kernel header? Or maybe we can just remove them, especially if they are not used by the kernel. -- /Horatiu
Powered by blists - more mailing lists