[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