logd: klogd: parse error
Resolve three areas missing taglen checking. Add some additional
limit-checking paranoia. Problem started when p was allowed to
go beyond the size of the incoming buffer in some blind
p = cp + 1 fragments, placed the check for that after them all
before harm could be done, rather than in each location.
Bug: 25620123
Change-Id: Ib5687fd30ef0cd3ba3bc0df310b436ad675ccabc
diff --git a/logd/LogKlog.cpp b/logd/LogKlog.cpp
index da5e78d..2a3f52f 100644
--- a/logd/LogKlog.cpp
+++ b/logd/LogKlog.cpp
@@ -582,7 +582,7 @@
// Some may view the following as an ugly heuristic, the desire is to
// beautify the kernel logs into an Android Logging format; the goal is
// admirable but costly.
- while ((isspace(*p) || !*p) && (p < &buf[len])) {
+ while ((p < &buf[len]) && (isspace(*p) || !*p)) {
++p;
}
if (p >= &buf[len]) { // timestamp, no content
@@ -596,7 +596,7 @@
const char *bt, *et, *cp;
bt = p;
- if (!fast<strncmp>(p, "[INFO]", 6)) {
+ if ((taglen >= 6) && !fast<strncmp>(p, "[INFO]", 6)) {
// <PRI>[<TIME>] "[INFO]"<tag> ":" message
bt = p + 6;
taglen -= 6;
@@ -620,7 +620,9 @@
p = cp + 1;
} else if (taglen) {
size = et - bt;
- if ((*bt == *cp) && fast<strncmp>(bt + 1, cp + 1, size - 1)) {
+ if ((taglen > size) && // enough space for match plus trailing :
+ (*bt == *cp) && // ubber fast<strncmp> pair
+ fast<strncmp>(bt + 1, cp + 1, size - 1)) {
// <PRI>[<TIME>] <tag>_host '<tag>.<num>' : message
if (!fast<strncmp>(bt + size - 5, "_host", 5)
&& !fast<strncmp>(bt + 1, cp + 1, size - 6)) {
@@ -694,7 +696,7 @@
p = cp + 1;
}
}
- }
+ } /* else no tag */
size = etag - tag;
if ((size <= 1)
// register names like x9
@@ -721,8 +723,12 @@
taglen = mp - tag;
}
}
+ // Deal with sloppy and simplistic harmless p = cp + 1 etc above.
+ if (len < (size_t)(p - buf)) {
+ p = &buf[len];
+ }
// skip leading space
- while ((isspace(*p) || !*p) && (p < &buf[len])) {
+ while ((p < &buf[len]) && (isspace(*p) || !*p)) {
++p;
}
// truncate trailing space or nuls
@@ -735,16 +741,26 @@
p = " ";
b = 1;
}
+ // paranoid sanity check, can not happen ...
if (b > LOGGER_ENTRY_MAX_PAYLOAD) {
b = LOGGER_ENTRY_MAX_PAYLOAD;
}
+ if (taglen > LOGGER_ENTRY_MAX_PAYLOAD) {
+ taglen = LOGGER_ENTRY_MAX_PAYLOAD;
+ }
+ // calculate buffer copy requirements
size_t n = 1 + taglen + 1 + b + 1;
- int rc = n;
- if ((taglen > n) || (b > n)) { // Can not happen ...
- rc = -EINVAL;
- return rc;
+ // paranoid sanity check, first two just can not happen ...
+ if ((taglen > n) || (b > n) || (n > USHRT_MAX)) {
+ return -EINVAL;
}
+ // Careful.
+ // We are using the stack to house the log buffer for speed reasons.
+ // If we malloc'd this buffer, we could get away without n's USHRT_MAX
+ // test above, but we would then required a max(n, USHRT_MAX) as
+ // truncating length argument to logbuf->log() below. Gain is protection
+ // of stack sanity and speedup, loss is truncated long-line content.
char newstr[n];
char *np = newstr;
@@ -763,8 +779,8 @@
np[b] = '\0';
// Log message
- rc = logbuf->log(LOG_ID_KERNEL, now, uid, pid, tid, newstr,
- (n <= USHRT_MAX) ? (unsigned short) n : USHRT_MAX);
+ int rc = logbuf->log(LOG_ID_KERNEL, now, uid, pid, tid, newstr,
+ (unsigned short) n);
// notify readers
if (!rc) {