From: Oliver Hartkopp <socketcan@hartkopp.net>
To: Guy Harris <gharris@sonic.net>
Cc: wireshark-dev@wireshark.org,
	linux-can@vger.kernel.org,
	Oliver Hartkopp <socketcan@hartkopp.net>
Subject: [PATCH 1/4] socketcan: simplify CAN packet type detection
Date: Mon, 18 Mar 2024 11:46:40 +0100
Message-ID: <20240318104643.24098-2-socketcan@hartkopp.net>
In-Reply-To: <20240318104643.24098-1-socketcan@hartkopp.net>
References: <20240318104643.24098-1-socketcan@hartkopp.net>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="us-ascii"

The different CAN frame types are defined by Linux SLL_P types in the
sll_protocol field and the length of the frame.

LINUX_SLL_P_CANXL:
The frame length for CAN XL can be 12 + 1 to 12 + 2048 (13 .. 2060) byte.
Additionally the CANXL_XLF flag has to be set.

LINUX_SLL_P_CAN and LINUX_SLL_P_CANFD:
The frame length for CAN CC is 16 byte and for CAN FD it is 72 byte.
Additionally the CANXL_XLF flag is cleared in CAN CC/FD frames.

Simplify the CAN packet type detection by checking exactly the above
rules without checking the CANFD_FDF flag which is not guarantied
over all Linux kernel versions.

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 epan/dissectors/packet-socketcan.c | 56 +++++-------------------------
 1 file changed, 8 insertions(+), 48 deletions(-)

diff --git a/epan/dissectors/packet-socketcan.c b/epan/dissectors/packet-socketcan.c
index e441c44ade..e9fcdb2dd8 100644
--- a/epan/dissectors/packet-socketcan.c
+++ b/epan/dissectors/packet-socketcan.c
@@ -110,10 +110,11 @@ static heur_dtbl_entry_t *heur_dtbl_entry;
 #define CANFD_FLAG_OFFSET  5
 
 #define CANFD_BRS 0x01 /* bit rate switch (second bitrate for payload data) */
 #define CANFD_ESI 0x02 /* error state indicator of the transmitting node */
 
+#define CANXL_FLAGS_OFFSET CAN_LEN_OFFSET
 #define CANXL_LEN_OFFSET   6
 #define CANXL_DATA_OFFSET  12
 
 static dissector_table_t can_id_dissector_table = NULL;
 static dissector_table_t can_extended_id_dissector_table = NULL;
@@ -589,69 +590,28 @@ dissect_socketcan_common(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree, gu
     static int * const canxl_flag_fields[] = {
         &hf_canxl_secflag,
         NULL,
     };
 
-    /*
-     * If we weren't told the type of this frame, check
-     * whether the CANFD_FDF flag is set in the FD flags
-     * field of the header; if so, it's a CAN FD frame.
-     * otherwise, it's a CAN frame.
-     *
-     * However, trust the CANFD_FDF flag only if the only
-     * bits set in the FD flags field are the known bits,
-     * and the two bytes following that field are both
-     * zero.  This is because some older LINKTYPE_CAN_SOCKETCAN
-     * frames had uninitialized junk in the FD flags field,
-     * so we treat a frame with what appears to be uninitialized
-     * junk as being CAN rather than CAN FD, under the assumption
-     * that the CANFD_FDF bit is set because the field is
-     * uninitialized, not because it was explicitly set because
-     * it's a CAN FD frame.  At least some newer code that sets
-     * that flag also makes sure that the fields in question are
-     * initialized, so we assume that if they're not initialized
-     * the code is older code that didn't support CAN FD.
-     */
+    /* determine CAN packet type */
     if (can_packet_type == PACKET_TYPE_UNKNOWN) {
-        guint8 frame_length;
-        guint8 fd_flags;
+        guint8 canxl_flags;
 
         /*
          * Check whether the frame has the CANXL_XLF flag set in what
          * is in the location of the frame length field of a CAN classic
          * or CAN FD frame; if so, then it's a CAN XL frame (and that
          * field is the flags field of that frame).
          */
-        frame_length = tvb_get_guint8(tvb, CAN_LEN_OFFSET);
-        if (frame_length & CANXL_XLF) {
+        canxl_flags = tvb_get_guint8(tvb, CANXL_FLAGS_OFFSET);
+        if ((tvb_reported_length(tvb) > 12) && (canxl_flags & CANXL_XLF)) {
             can_packet_type = PACKET_TYPE_CAN_XL;
         } else {
-            /*
-             * This is a CAN classic or CAN FD frame.
-             * Check whether the flags field has the CANFD_FDF
-             * flag set, has no unknown flag bits set, and has
-             * no bits set in the two reserved fields.  If so,
-             * it's a CAN FD frame; otherwise, it's either a
-             * CAN classic frame, or a frame where the CANFD_FDF
-             * flag is set but where that might just be because
-             * that field contains uninitialized junk rather
-             * than because it's a CAN FD frame, so we treat it
-             * as a CAN classic frame.
-             */
-            fd_flags = tvb_get_guint8(tvb, CANFD_FLAG_OFFSET);
-
-            if ((fd_flags & CANFD_FDF) &&
-                    ((fd_flags & ~(CANFD_BRS | CANFD_ESI | CANFD_FDF)) == 0) &&
-                    tvb_get_guint8(tvb, CANFD_FLAG_OFFSET + 1) == 0 &&
-                    tvb_get_guint8(tvb, CANFD_FLAG_OFFSET + 2) == 0) {
+            if (tvb_reported_length(tvb) == 72)
                 can_packet_type = PACKET_TYPE_CAN_FD;
-            } else {
-                if (tvb_reported_length(tvb) == 72)
-                    can_packet_type = PACKET_TYPE_CAN_FD;
-                else
-                    can_packet_type = PACKET_TYPE_CAN;
-            }
+            else if  (tvb_reported_length(tvb) == 16)
+                can_packet_type = PACKET_TYPE_CAN;
         }
     }
 
     can_info.bus_id = get_bus_id(pinfo);
 
-- 
2.43.0