| Message ID | 20210625155348.58266-1-yun.zhou@windriver.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] seq_buf: fix overflow when length is bigger than 8 | expand |
On Fri, 25 Jun 2021 23:53:47 +0800 Yun Zhou <yun.zhou@windriver.com> wrote: > There's two variables being increased in that loop (i and j), and i > follows the raw data, and j follows what is being written into the buffer. > We should compare 'i' to MAX_MEMHEX_BYTES or compare 'j' to HEX_CHARS. > Otherwise, if 'j' goes bigger than HEX_CHARS, it will overflow the > destination buffer. > > This bug was introduced by commit 6d2289f3faa71dcc ("tracing: Make > trace_seq_putmem_hex() more robust") No it wasn't. The bug was in the original code: 5e3ca0ec76fce ("ftrace: introduce the "hex" output method") Which had this: > static notrace int > trace_seq_putmem_hex(struct trace_seq *s, void *mem, size_t len) > { > unsigned char hex[HEX_CHARS]; > unsigned char *data; > unsigned char byte; > int i, j; > > BUG_ON(len >= HEX_CHARS); If len is 16 (and HEX_CHARS is 17) the bug wouldn't happen. > > data = mem; > > #ifdef __BIG_ENDIAN > for (i = 0, j = 0; i < len; i++) { > #else > for (i = len-1, j = 0; i >= 0; i--) { > #endif The above starts at len-1 (15) and will iterate 15 times. > byte = data[i]; > > hex[j] = byte & 0x0f; > if (hex[j] >= 10) > hex[j] += 'a' - 10; > else > hex[j] += '0'; > j++; > > hex[j] = byte >> 4; > if (hex[j] >= 10) > hex[j] += 'a' - 10; > else > hex[j] += '0'; > j++; j is incremented twice for every loop, and if len was 15, that is 30 times. Needless to say, once i iterated 9 times, then j would be 18, and one more than the size of hex. And boom, it breaks. > } > hex[j] = ' '; > j++; > > return trace_seq_putmem(s, hex, j); > } > > Signed-off-by: Yun Zhou <yun.zhou@windriver.com> > --- > lib/seq_buf.c | 29 +++++++++++------------------ > 1 file changed, 11 insertions(+), 18 deletions(-) > > diff --git a/lib/seq_buf.c b/lib/seq_buf.c > index 6aabb609dd87..aa2f666e584e 100644 > --- a/lib/seq_buf.c > +++ b/lib/seq_buf.c > @@ -210,7 +210,8 @@ int seq_buf_putmem(struct seq_buf *s, const void *mem, unsigned int len) > * seq_buf_putmem_hex - write raw memory into the buffer in ASCII hex > * @s: seq_buf descriptor > * @mem: The raw memory to write its hex ASCII representation of > - * @len: The length of the raw memory to copy (in bytes) > + * @len: The length of the raw memory to copy (in bytes). > + * It can be not larger than 8. > * > * This is similar to seq_buf_putmem() except instead of just copying the > * raw memory into the buffer it writes its ASCII representation of it > @@ -228,27 +229,19 @@ int seq_buf_putmem_hex(struct seq_buf *s, const void *mem, > > WARN_ON(s->size == 0); > > - while (len) { > - start_len = min(len, HEX_CHARS - 1); > + start_len = min(len, MAX_MEMHEX_BYTES); > #ifdef __BIG_ENDIAN > - for (i = 0, j = 0; i < start_len; i++) { > + for (i = 0, j = 0; i < start_len; i++) { > #else > - for (i = start_len-1, j = 0; i >= 0; i--) { > + for (i = start_len-1, j = 0; i >= 0; i--) { > #endif > - hex[j++] = hex_asc_hi(data[i]); > - hex[j++] = hex_asc_lo(data[i]); > - } > - if (WARN_ON_ONCE(j == 0 || j/2 > len)) > - break; > - > - /* j increments twice per loop */ > - len -= j / 2; > - hex[j++] = ' '; > - > - seq_buf_putmem(s, hex, j); > - if (seq_buf_has_overflowed(s)) > - return -1; > + hex[j++] = hex_asc_hi(data[i]); > + hex[j++] = hex_asc_lo(data[i]); > } > + > + seq_buf_putmem(s, hex, j); > + if (seq_buf_has_overflowed(s)) > + return -1; > return 0; > } > The above is *way* too complex for a backport that should go back to the beginning. You were partially, correct, and the proper fix would be: diff --git a/lib/seq_buf.c b/lib/seq_buf.c index 707453f5d58e..eb68b5b3eb26 100644 --- a/lib/seq_buf.c +++ b/lib/seq_buf.c @@ -229,8 +229,10 @@ int seq_buf_putmem_hex(struct seq_buf *s, const void *mem, WARN_ON(s->size == 0); + BUILD_BUG_ON(MAX_MEMHEX_BYTES * 2 >= HEX_CHARS); + while (len) { - start_len = min(len, HEX_CHARS - 1); + start_len = min(len, MAX_MEMHEX_BYTES - 1); #ifdef __BIG_ENDIAN for (i = 0, j = 0; i < start_len; i++) { #else
diff --git a/lib/seq_buf.c b/lib/seq_buf.c index 6aabb609dd87..aa2f666e584e 100644 --- a/lib/seq_buf.c +++ b/lib/seq_buf.c @@ -210,7 +210,8 @@ int seq_buf_putmem(struct seq_buf *s, const void *mem, unsigned int len) * seq_buf_putmem_hex - write raw memory into the buffer in ASCII hex * @s: seq_buf descriptor * @mem: The raw memory to write its hex ASCII representation of - * @len: The length of the raw memory to copy (in bytes) + * @len: The length of the raw memory to copy (in bytes). + * It can be not larger than 8. * * This is similar to seq_buf_putmem() except instead of just copying the * raw memory into the buffer it writes its ASCII representation of it @@ -228,27 +229,19 @@ int seq_buf_putmem_hex(struct seq_buf *s, const void *mem, WARN_ON(s->size == 0); - while (len) { - start_len = min(len, HEX_CHARS - 1); + start_len = min(len, MAX_MEMHEX_BYTES); #ifdef __BIG_ENDIAN - for (i = 0, j = 0; i < start_len; i++) { + for (i = 0, j = 0; i < start_len; i++) { #else - for (i = start_len-1, j = 0; i >= 0; i--) { + for (i = start_len-1, j = 0; i >= 0; i--) { #endif - hex[j++] = hex_asc_hi(data[i]); - hex[j++] = hex_asc_lo(data[i]); - } - if (WARN_ON_ONCE(j == 0 || j/2 > len)) - break; - - /* j increments twice per loop */ - len -= j / 2; - hex[j++] = ' '; - - seq_buf_putmem(s, hex, j); - if (seq_buf_has_overflowed(s)) - return -1; + hex[j++] = hex_asc_hi(data[i]); + hex[j++] = hex_asc_lo(data[i]); } + + seq_buf_putmem(s, hex, j); + if (seq_buf_has_overflowed(s)) + return -1; return 0; }
There's two variables being increased in that loop (i and j), and i follows the raw data, and j follows what is being written into the buffer. We should compare 'i' to MAX_MEMHEX_BYTES or compare 'j' to HEX_CHARS. Otherwise, if 'j' goes bigger than HEX_CHARS, it will overflow the destination buffer. This bug was introduced by commit 6d2289f3faa71dcc ("tracing: Make trace_seq_putmem_hex() more robust") Signed-off-by: Yun Zhou <yun.zhou@windriver.com> --- lib/seq_buf.c | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-)