lib: parse_canframe: guard delimiter reads with length checks

As Alex J pointed out in comment
https://github.com/linux-can/can-utils/issues/632#issuecomment-5746692502
the delimiters to detect CAN CC/FD/XL frames were read from the input
data without checking the length of that input data. This could lead
to an out-of-bounds read and to unintended detection of incorrect content.

Add a length check before reading those delimiters and also add/change
some comments to clarify the reasons for some assignments.

Reported-by: Alex J <jipaionut@gmail.com>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>

.github/workflows/compile.yml

@@ -75,7 +75,6 @@ jobs:
                 gcc \
                 gcc-aarch64-linux-gnu \
                 gcc-arm-linux-gnueabihf \
-                gcc-mips-linux-gnu \
                 libgps-dev \
                 make
 
@@ -139,13 +138,6 @@ jobs:
         podman exec -i stable cmake -DCMAKE_BUILD_TYPE=Debug -DCMAKE_TOOLCHAIN_FILE=cmake/${toolchain}.cmake -DENABLE_WERROR=ON -DENABLE_GPS=${gps} -B build-${toolchain}
         podman exec -i stable cmake --build build-${toolchain}
 
-    - name: Configure & Build with mips-linux-gnu-gcc
-      env:
-        toolchain: mips-linux-gnu-gcc
-      run: |
-        podman exec -i stable cmake -DCMAKE_BUILD_TYPE=Debug -DCMAKE_TOOLCHAIN_FILE=cmake/${toolchain}.cmake -DENABLE_WERROR=ON -B build-${toolchain}
-        podman exec -i stable cmake --build build-${toolchain}
-
     - name: Configure & Build with gcc (Makefile)
       env:
         cc: gcc

bcmserver.c

@@ -153,7 +153,7 @@ int main(void)
 
 	char buf[MAXLEN];
 	char format[FORMATSZ];
-	char rxmsg[50];
+	char rxmsg[64];
 
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wpragmas"
@@ -234,7 +234,7 @@ int main(void)
 	}
 
 	while (1) {
-
+again:
 		FD_ZERO(&readfds);
 		FD_SET(sc, &readfds);
 		FD_SET(sa, &readfds);
@@ -242,6 +242,8 @@ int main(void)
 		select((sc > sa)?sc+1:sa+1, &readfds, NULL, NULL, NULL);
 
 		if (FD_ISSET(sc, &readfds)) {
+			size_t size = sizeof(rxmsg);
+			int len = 0, res;
 
 			recvfrom(sc, &msg, sizeof(msg), 0,
 				 (struct sockaddr*)&caddr, &caddrlen);
@@ -249,17 +251,35 @@ int main(void)
 			ifr.ifr_ifindex = caddr.can_ifindex;
 			ioctl(sc, SIOCGIFNAME, &ifr);
 
-			sprintf(rxmsg, "< %s %03X %d ", ifr.ifr_name,
+			res = snprintf(rxmsg, size, "< %s %03X %d ", ifr.ifr_name,
-				msg.msg_head.can_id, msg.frame.can_dlc);
+				       msg.msg_head.can_id, msg.frame.can_dlc);
+			if (res < 0 || (size_t)res >= size) {
+				printf("Error: rxmsg buffer (size %zu) too small for data.\n", size);
+				continue;
+			}
 
-			for ( i = 0; i < msg.frame.can_dlc; i++)
+			len += res;
-				sprintf(rxmsg + strlen(rxmsg), "%02X ",
+
-					msg.frame.data[i]);
+			for (i = 0; i < msg.frame.can_dlc; i++) {
+				res = snprintf(rxmsg + len, size - len, "%02X ", msg.frame.data[i]);
+				if (res < 0 || (size_t)res >= (size - len)) {
+					printf("Error: rxmsg buffer (size %zu) too small for data.\n", size);
+					goto again;
+				}
+
+				len += res;
+			}
 
 			/* delimiter '\0' for Adobe(TM) Flash(TM) XML sockets */
-			strcat(rxmsg, ">\0");
+			res = snprintf(rxmsg + len, size - len, ">");
+			if (res < 0 || (size_t)res >= (size - len)) {
+				printf("Error: rxmsg buffer (size %zu) too small for data.\n", size);
+				continue;
+			}
 
-			send(sa, rxmsg, strlen(rxmsg) + 1, 0);
+			len += res;
+
+			send(sa, rxmsg, len + 1, 0);
 		}
 
 

canerrsim.c

@@ -25,6 +25,7 @@
 #include <linux/can/error.h>
 #include <linux/can/raw.h>
 #include <net/if.h>
+#include <stdarg.h>
 #include <stdbool.h>
 #include <stdint.h>
 #include <stdio.h>
@@ -122,27 +123,25 @@ void show_help_and_exit()
 	exit(EXIT_SUCCESS);
 }
 
-void err_exit(const char *msg)
+void __attribute__((format (printf, 1, 2))) err_exit(const char *format, ...)
 {
-	printf("%s", msg);
+	va_list ap;
-	exit(EXIT_FAILURE);
-}
 
-void show_custom_format_and_exit(const char *param, const char *format)
+	va_start(ap, format);
-{
+	vfprintf(stdout, format, ap);
-	char str_buf[80];
+	va_end(ap);
-	sprintf(str_buf, format, param);
+
-	err_exit(str_buf);
+	exit(EXIT_FAILURE);
 }
 
 void show_invalid_option(const char *option)
 {
-	show_custom_format_and_exit(option, "Error: Invalid option %s\n");
+	err_exit("Error: Invalid option %s\n", option);
 }
 
 void show_err_and_exit(const char *err_type)
 {
-	show_custom_format_and_exit(err_type, "Error: You can only have one %s parameter!\n");
+	err_exit("Error: You can only have one %s parameter!\n", err_type);
 }
 
 void show_loc_err_and_exit()
@@ -176,7 +175,6 @@ int main(int argc, char *argv[])
 	struct ifreq ifr;
 	struct can_frame frame;
 	bool show_bits = false, location_processed = false, transceiver_processed = false, arbitration_processed = false;
-	char tmp_str[256];
 
 	printf("CAN Sockets Error Messages Simulator\n");
 	if (argc < 3)
@@ -537,24 +535,25 @@ int main(int argc, char *argv[])
 
 	// create socket
 	if ((sock = socket(PF_CAN, SOCK_RAW, CAN_RAW)) < 0)
-		err_exit("Error while opening socket");
+		err_exit("Error while opening socket\n");
 
 	// set interface name
+	if (strlen(argv[1]) >= IFNAMSIZ)
+		err_exit("Name of CAN device '%s' is too long!\n\n", argv[1]);
+
 	strcpy(ifr.ifr_name, argv[1]); // can0, vcan0...
-	if (ioctl(sock, SIOCGIFINDEX, &ifr) < 0) {
+	if (ioctl(sock, SIOCGIFINDEX, &ifr) < 0)
-		sprintf(tmp_str, "Error setting CAN interface name %s", argv[1]);
+		err_exit("Error setting CAN interface name %s\n", argv[1]);
-		err_exit(tmp_str);
-	}
 
 	// bind socket to the CAN interface
 	addr.can_family = AF_CAN;
 	addr.can_ifindex = ifr.ifr_ifindex;
 	if (bind(sock, (struct sockaddr *)&addr, sizeof(addr)) < 0)
-		err_exit("Error in socket bind");
+		err_exit("Error in socket bind\n");
 
 	// Send CAN error frame
 	if (write(sock, &frame, sizeof(frame)) < 0)
-		err_exit("Error writing to socket");
+		err_exit("Error writing to socket\n");
 	else
 		printf("CAN error frame sent\n");
 

canlogserver.c

@@ -177,7 +177,8 @@ int main(int argc, char **argv)
 	sigset_t sigset;
 	fd_set rdfs;
 	int s[MAXDEV];
-	int socki, accsocket;
+	int socki;
+	int accsocket = -1;
 	canid_t mask[MAXDEV] = {0};
 	canid_t value[MAXDEV] = {0};
 	int inv_filter[MAXDEV] = {0};
@@ -286,7 +287,7 @@ int main(int argc, char **argv)
 	inaddr.sin_addr.s_addr = htonl(INADDR_ANY);
 	inaddr.sin_port = htons(port);
 
-	while(bind(socki, (struct sockaddr*)&inaddr, sizeof(inaddr)) < 0) {
+	while(running && bind(socki, (struct sockaddr*)&inaddr, sizeof(inaddr)) < 0) {
 		struct timespec f = {
 			.tv_nsec = 100 * 1000 * 1000,
 		};
@@ -295,18 +296,28 @@ int main(int argc, char **argv)
 		nanosleep(&f, NULL);
 	}
 
+	/*
+	 * Check if loop exited due to signal (during nanosleep) rather than
+	 * successful bind.
+	 */
+	if (!running) {
+		close(socki);
+		return 128 + signal_num;
+	}
+
 	if (listen(socki, 3) != 0) {
 		perror("listen");
 		exit(1);
 	}
 
-	while(1) {
+	while(running) {
 		accsocket = accept(socki, (struct sockaddr*)&clientaddr, &sin_size);
 		if (accsocket > 0) {
 			//printf("accepted\n");
 			if (!fork())
 				break;
 			close(accsocket);
+			accsocket = -1;
 		}
 		else if (errno != EINTR) {
 			perror("accept");
@@ -314,6 +325,17 @@ int main(int argc, char **argv)
 		}
 	}
 
+	/*
+	 * Check if loop exited due to signal (accept returned EINTR) rather
+	 * than successful fork
+	 */
+	if (!running) {
+		if (accsocket > 0)
+			close(accsocket);
+		close(socki);
+		return 128 + signal_num;
+	}
+
 	for (i=0; i<currmax; i++) {
 
 		pr_debug("open %d '%s' m%08X v%08X i%d e%d.\n",

isotpsniffer.c

@@ -59,7 +59,6 @@
 #include <linux/can.h>
 #include <linux/can/isotp.h>
 #include <linux/sockios.h>
-#include <errno.h>
 
 #define NO_CAN_ID 0xFFFFFFFFU
 
@@ -67,8 +66,6 @@
 #define FORMAT_ASCII 2
 #define FORMAT_DEFAULT (FORMAT_ASCII | FORMAT_HEX)
 
-#define PDU_BUF_SIZE 4096
-
 void print_usage(char *prg)
 {
 	fprintf(stderr, "\nUsage: %s [options] <CAN interface>\n", prg);
@@ -82,7 +79,6 @@ void print_usage(char *prg)
 	fprintf(stderr, "         -f <format>  (1 = HEX, 2 = ASCII, 3 = HEX & ASCII - default: %d)\n", FORMAT_DEFAULT);
 	fprintf(stderr, "         -L           (set link layer options for CAN FD)\n");
 	fprintf(stderr, "         -h <len>     (head: print only first <len> bytes)\n");
-	fprintf(stderr, "         -i           (ignore syscall errors to receive malformed PDUs)\n");
 	fprintf(stderr, "\nCAN IDs and addresses are given and expected in hexadecimal values.\n");
 	fprintf(stderr, "\n");
 }
@@ -193,16 +189,15 @@ int main(int argc, char **argv)
 	int head = 0;
 	int timestamp = 0;
 	int format = FORMAT_DEFAULT;
-	int ignore_errors = 0;
 	canid_t src = NO_CAN_ID;
 	canid_t dst = NO_CAN_ID;
 	extern int optind, opterr, optopt;
 	static struct timeval tv, last_tv;
 
-	unsigned char buffer[PDU_BUF_SIZE];
+	unsigned char buffer[4096];
 	int nbytes;
 
-	while ((opt = getopt(argc, argv, "s:d:x:X:h:ct:f:L?i")) != -1) {
+	while ((opt = getopt(argc, argv, "s:d:x:X:h:ct:f:L?")) != -1) {
 		switch (opt) {
 		case 's':
 			src = strtoul(optarg, NULL, 16);
@@ -254,10 +249,6 @@ int main(int argc, char **argv)
 			}
 			break;
 
-		case 'i':
-			ignore_errors = 1;
-			break;
-
 		case '?':
 			print_usage(basename(argv[0]));
 			goto out;
@@ -376,39 +367,33 @@ int main(int argc, char **argv)
 		}
 
 		if (FD_ISSET(s, &rdfs)) {
-			nbytes = read(s, buffer, PDU_BUF_SIZE);
+			nbytes = read(s, buffer, 4096);
 			if (nbytes < 0) {
 				perror("read socket s");
 				r = 1;
-				if(!ignore_errors)
-					goto out;
-			}
-			if (nbytes > (PDU_BUF_SIZE - 1)) {
-				r = 1;
-				fprintf(stderr, "PDU length %d longer than PDU buffer: %s\n", nbytes, strerror(errno));
 				goto out;
 			}
-			if(nbytes > 0)
+			if (nbytes > 4095) {
-				printbuf(buffer, nbytes, color?2:0, timestamp, format,
+				r = 1;
-					 &tv, &last_tv, dst, s, if_name, head);
+				goto out;
+			}
+			printbuf(buffer, nbytes, color?2:0, timestamp, format,
+				 &tv, &last_tv, dst, s, if_name, head);
 		}
 
 		if (FD_ISSET(t, &rdfs)) {
-			nbytes = read(t, buffer, PDU_BUF_SIZE);
+			nbytes = read(t, buffer, 4096);
 			if (nbytes < 0) {
 				perror("read socket t");
 				r = 1;
-				if(!ignore_errors)
-					goto out;
-			}
-			if (nbytes > (PDU_BUF_SIZE - 1)) {
-				r = 1;
-				fprintf(stderr, "PDU length %d longer than PDU buffer: %s\n", nbytes, strerror(errno));
 				goto out;
 			}
-			if(nbytes > 0)
+			if (nbytes > 4095) {
-				printbuf(buffer, nbytes, color?1:0, timestamp, format,
+				r = 1;
-					 &tv, &last_tv, src, t, if_name, head);
+				goto out;
+			}
+			printbuf(buffer, nbytes, color?1:0, timestamp, format,
+				 &tv, &last_tv, src, t, if_name, head);
 		}
 	}
 

j1939_timedate/j1939_timedate_srv.c

@@ -41,16 +41,22 @@ static void gmtime_to_j1939_pgn_65254_td(struct j1939_time_date_packet *tdp)
 	utc_tm = gmtime_r(&now, &utc_tm_buf);
 	local_tm = localtime_r(&now, &local_tm_buf);
 
-	/* Calculate the offsets */
+	if (local_tm) {
-	hour_offset = local_tm->tm_hour - utc_tm->tm_hour;
+		/* Calculate the offsets */
-	minute_offset = local_tm->tm_min - utc_tm->tm_min;
+		hour_offset = local_tm->tm_hour - utc_tm->tm_hour;
+		minute_offset = local_tm->tm_min - utc_tm->tm_min;
 
-	/* Handle date rollover */
+		/* Handle date rollover */
-	if (local_tm->tm_mday != utc_tm->tm_mday) {
+		if (local_tm->tm_mday != utc_tm->tm_mday) {
-		if (local_tm->tm_hour < 12)
+			if (local_tm->tm_hour < 12)
-			hour_offset += 24;  /* past midnight */
+				hour_offset += 24;  /* past midnight */
-		else
+			else
-			hour_offset -= 24;  /* before midnight */
+				hour_offset -= 24;  /* before midnight */
+		}
+	} else {
+		/* The local time offsets cannot be determined at the moment */
+		hour_offset = 0xF9;
+		minute_offset = 0xFF;
 	}		
 
 	/*

j1939_vehicle_position/j1939_vehicle_position_cmn.h

@@ -51,7 +51,7 @@ struct j1939_vp_err_msg {
 #define J1939_VP1_PRIO_DEFAULT				6
 #define J1939_VP1_MAX_TRANSFER_LENGH \
 	sizeof(struct j1939_vp1_packet)
-#define J1939_VP1_REPETITION_RATE_MS			5000
+#define J1939_VP1_REPETITION_RATE_MS			1000
 #define J1939_VP1_JITTER_MS				500
 
 /**
@@ -63,8 +63,8 @@ struct j1939_vp_err_msg {
  *           - Data Length: 4 bytes
  *           - Resolution: 10^-7 deg/bit
  *           - Offset: -210 degrees
- *           - Range: -210 to +211.1008122 degrees
+ *           - Range: -210 to +211.10081215 degrees
- *           - Operating Range: -210 degrees (SOUTH) to +211.108122 degrees
+ *           - Operating Range: -210 degrees (SOUTH) to +211.1081215 degrees
  *             (NORTH)
  *
  * @longitude: Raw longitude position of the vehicle
@@ -72,8 +72,8 @@ struct j1939_vp_err_msg {
  *           - Data Length: 4 bytes
  *           - Resolution: 10^-7 deg/bit
  *           - Offset: -210 degrees
- *           - Range: -210 to +211.1008122 degrees
+ *           - Range: -210 to +211.10081215 degrees
- *           - Operating Range: -210 degrees (WEST) to +211.108122 degrees
+ *           - Operating Range: -210 degrees (WEST) to +211.1081215 degrees
  *             (EAST)
  *
  * This structure defines each component of the Vehicle Position as described in
@@ -142,42 +142,48 @@ j1939_vp1_set_longitude(struct j1939_vp1_packet *packet, int32_t longitude)
 /**
  * struct j1939_vp2_packet - Represents the PGN 64502 Vehicle
  *                                         Position 2 packet
- * FIXME: current packet layout is guessed based on limited information:
- * https://www.isobus.net/isobus/pGNAndSPN/10801?type=PGN
  *
  * @total_satellites: Total number of satellites in view
  *           - SPN: 8128
  *           - Data Length: 1 byte
+ *           - Range: 0 to 250
  *
  * @hdop: Horizontal dilution of precision
  *           - SPN: 8129
  *           - Data Length: 1 byte
  *           - Resolution: 0.1
+ *           - Range: 0.0 to 25.0
  *
  * @vdop: Vertical dilution of precision
  *           - SPN: 8130
  *           - Data Length: 1 byte
  *           - Resolution: 0.1
+ *           - Range: 0.0 to 25.0
  *
  * @pdop: Position dilution of precision
  *           - SPN: 8131
  *           - Data Length: 1 byte
  *           - Resolution: 0.1
+ *           - Range: 0.0 to 25.0
  *
  * @tdop: Time dilution of precision
  *           - SPN: 8132
  *           - Data Length: 1 byte
  *           - Resolution: 0.1
+ *           - Range: 0.0 to 25.0
  *
  * This structure defines each component of the Vehicle Position 2 as described
  * in PGN 64502.
  */
 struct j1939_vp2_packet {
 	uint8_t total_satellites; /* SPN 8128 */
-	uint8_t hdop;			  /* SPN 8129 */
+	uint8_t hdop;             /* SPN 8129 */
-	uint8_t vdop;			  /* SPN 8130 */
+	uint8_t vdop;             /* SPN 8130 */
-	uint8_t pdop;			  /* SPN 8131 */
+	uint8_t pdop;             /* SPN 8131 */
-	uint8_t tdop;			  /* SPN 8132 */
+	uint8_t tdop;             /* SPN 8132 */
+	uint8_t unused5;          /* Always 0xFF */
+	uint8_t unused6;          /* Always 0xFF */
+	uint8_t unused7;          /* Always 0xFF */
 } __attribute__((__packed__));
 
 /**

j1939_vehicle_position/j1939_vehicle_position_srv.c

@@ -270,6 +270,12 @@ static int j1939_vp2_get_data(struct j1939_vp_srv_priv *priv,
 	j1939_vp2_set_pdop(vp2p, pdop);
 	j1939_vp2_set_tdop(vp2p, tdop);
 
+	/* This PG's last 3 bytes are not assigned and hence must be set
+	 * to 0xFF as per J1939-71, section 5.2 */
+	vp2p->unused5 = 0xFF;
+	vp2p->unused6 = 0xFF;
+	vp2p->unused7 = 0xFF;
+
 	return 0;
 }
 

lib.c

@@ -170,21 +170,23 @@ int parse_canframe(char *cs, union cfu *cu)
 
 	memset(cu, 0, sizeof(*cu)); /* init CAN CC/FD/XL frame, e.g. LEN = 0 */
 
-	if (len < 4)
+	if (len >= 4 && cs[3] == CANID_DELIM) { /* 3 digits SFF CAN ID */
-		return 0;
 
-	if (cs[3] == CANID_DELIM) { /* 3 digits SFF */
+		idx = 4; /* start of frame data */
 
-		idx = 4;
+		/* get 3 digits SFF CAN ID value */
 		for (i = 0; i < 3; i++) {
 			if ((tmp = asc2nibble(cs[i])) > 0x0F)
 				return 0;
 			cu->cc.can_id |= tmp << (2 - i) * 4;
 		}
 
-	} else if (cs[5] == CANID_DELIM) { /* 5 digits CAN XL VCID/PRIO*/
+	} else if (len >= 21 && cs[5] == CANID_DELIM && cs[20] == CANID_DELIM) {
+		/* 5 digits CAN XL VCID/PRIO - but also check for 2nd '#' here */
 
-		idx = 6;
+		idx = 6; /* start of CAN XL frame extra content (AF, SDT, etc) */
+
+		/* get 5 digits CAN XL VCID/PRIO */
 		for (i = 0; i < 5; i++) {
 			if ((tmp = asc2nibble(cs[i])) > 0x0F)
 				return 0;
@@ -196,9 +198,11 @@ int parse_canframe(char *cs, union cfu *cu)
 		cu->xl.prio &= CANXL_PRIO_MASK;
 		cu->xl.prio |= tmp;
 
-	} else if (cs[8] == CANID_DELIM) { /* 8 digits EFF */
+	} else if (len >= 9 && cs[8] == CANID_DELIM) { /* 8 digits EFF CAN ID */
 
-		idx = 9;
+		idx = 9; /* start of frame data */
+
+		/* get 8 digits EFF CAN ID value */
 		for (i = 0; i < 8; i++) {
 			if ((tmp = asc2nibble(cs[i])) > 0x0F)
 				return 0;
@@ -239,11 +243,12 @@ int parse_canframe(char *cs, union cfu *cu)
 		cu->fd.flags |= CANFD_FDF; /* dual-use */
 		idx += 2;
 
-	} else if (cs[idx + 14] == CANID_DELIM) { /* CAN XL frame '#80:00:11223344#' */
+	} else if (idx == 6) { /* CAN XL frame extra content '#80:00:11223344#' */
 		maxdlen = CANXL_MAX_DLEN;
 		mtu = CANXL_MTU;
-		data = cu->xl.data; /* fill CAN XL data */
+		data = cu->xl.data; /* overwrite pointer to CAN XL data */
 
+		/* get CAN XL frame extra content */
 		if ((cs[idx + 2] != XL_HDR_DELIM) || (cs[idx + 5] != XL_HDR_DELIM))
 			return 0;
 
@@ -277,6 +282,7 @@ int parse_canframe(char *cs, union cfu *cu)
 		idx++; /* skip CANID_DELIM */
 	}
 
+	/* copy CAN frame data content */
 	for (i = 0, dlen = 0; i < maxdlen; i++) {
 		if (cs[idx] == DATA_SEPERATOR) /* skip (optional) separator */
 			idx++;

mcp251xfd/mcp251xfd-regmap.c

@@ -72,8 +72,7 @@ int mcp251xfd_regmap_read(struct mcp251xfd_priv *priv,
 		return 0;
 
 	/* maybe it's something like "spi0.0" */
-	tmp = strchr(file_path, '/');
+	if (strchr(file_path, '/'))
-	if (tmp)
 		return -ENOENT;
 
 	/* first try literally */