[PATCH] monitor: clamp max_len in print_packet

Chad Spensky <[email protected]>
Newsgroups org.kernel.vger.linux-bluetooth
Message-ID <[email protected]>
print_packet() derives the snprintf() bound for the packet text from the
terminal width and never bounds it against the space left in line[]:

	char line[LINE_MAX], ts_str[96], pid_str[140];
	int col = num_columns();
	...
	int max_len = col - len - extra_len - ts_len - 3;

	if (max_len <= 0) {
		extra = NULL;
		max_len = col - len - ts_len - 3;
	}

	n = snprintf(line + pos, max_len + 1, "%s%s",
					label ? ": " : "", text);

max_len can leave range in both directions, and either aborts under
_FORTIFY_SOURCE with "*** buffer overflow detected ***: terminated":

 - The existing max_len <= 0 recovery drops extra and recomputes, but if
   the prefix alone exceeds the column budget the result is still
   negative, and max_len + 1 then underflows when converted to size_t.
   len includes the "comm[pid]: " prefix built from struct ucred, which
   is only present when reading from the monitor socket, so this is
   reachable at ordinary widths. On a host with a large kernel.pid_max
   the pid is 7 digits, so a long process name plus a long label is
   enough and btmon dies mid-capture. Replaying the same traffic from a
   btsnoop file never reproduces it, because there is no ucred and hence
   no prefix.

 - col larger than sizeof(line) makes max_len + 1 exceed the remaining
   buffer. LINE_MAX raised the bar but did not remove it.

A negative max_len is also used to index line[] when truncating the
text, writing before the start of the buffer, so this is an
out-of-bounds write and not only an abort.

The check only triggers at _FORTIFY_SOURCE=3; at =2 the runtime pos
offset defeats __builtin_object_size and the check is elided, which is
why this is mostly seen on distributions defaulting to =3.

Clamp max_len to the space remaining in line[], and only write the
ellipsis when there is room for it.

Reproduced and verified by building the current logic and the patched
logic with -O2 -D_FORTIFY_SOURCE=3 and replaying a capture at a range of
terminal widths:

	cols   before   after
	  10   abort    ok
	  20   abort    ok
	  40   abort    ok
	  80   ok       ok
	 200   ok       ok
	2000   ok       ok
	3000   abort    ok
	5000   abort    ok

Reported-at: https://github.com/bluez/bluez/issues/1104
---
 monitor/packet.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/monitor/packet.c b/monitor/packet.c
index 0d3b23cc3..df6c0a819 100644
--- a/monitor/packet.c
+++ b/monitor/packet.c
@@ -603,13 +603,26 @@ static void print_packet(struct timeval *tv, struct ucred *cred, char ident,
 			max_len = col - len - ts_len - 3;
 		}
 
+		/* col comes from the terminal and len includes the optional
+		 * "comm[pid]: " prefix, so max_len can still be negative
+		 * here, or larger than the space left in line[]. Both
+		 * overflow the snprintf below, and a negative value also
+		 * indexes before line[].
+		 */
+		if (max_len > (int) sizeof(line) - pos - 1)
+			max_len = (int) sizeof(line) - pos - 1;
+		if (max_len < 0)
+			max_len = 0;
+
 		n = snprintf(line + pos, max_len + 1, "%s%s",
 						label ? ": " : "", text);
 		if (n > max_len) {
-			line[pos + max_len - 1] = '.';
-			line[pos + max_len - 2] = '.';
-			if (line[pos + max_len - 3] == ' ')
-				line[pos + max_len - 3] = '.';
+			if (max_len >= 3) {
+				line[pos + max_len - 1] = '.';
+				line[pos + max_len - 2] = '.';
+				if (line[pos + max_len - 3] == ' ')
+					line[pos + max_len - 3] = '.';
+			}
 
 			n = max_len;
 		}
-- 
2.43.0
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.