netdev - [PATCH net 1/2] net: mrp: fix definitions of MRP test packets

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]

Message-Id: <20201223144533.4145-2-rasmus.villemoes@prevas.dk>
Date:   Wed, 23 Dec 2020 15:45:32 +0100
From:   Rasmus Villemoes <rasmus.villemoes@...vas.dk>
To:     netdev@...r.kernel.org,
        Horatiu Vultur <horatiu.vultur@...rochip.com>
Cc:     linux-kernel@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Rasmus Villemoes <rasmus.villemoes@...vas.dk>
Subject: [PATCH net 1/2] net: mrp: fix definitions of MRP test packets

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];
-- 
2.23.0