[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20210625122453.5e2fe304@oasis.local.home>
Date: Fri, 25 Jun 2021 12:24:53 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Yun Zhou <yun.zhou@...driver.com>
Cc: <linux-kernel@...r.kernel.org>, <kernel-hardening@...ts.openwall.com>,
<ying.xue@...driver.com>, <zhiquan.li@...driver.com>
Subject: Re: [PATCH 1/2] seq_buf: fix overflow when length is bigger than 8
On Fri, 25 Jun 2021 23:53:47 +0800
Yun Zhou <yun.zhou@...driver.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@...driver.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
--
2.29.2
That solves the first bug, and is easy to backport.
The second bug, is that data doesn't go forward (as you stated in your
original patch) which would be:
diff --git a/lib/seq_buf.c b/lib/seq_buf.c
index eb68b5b3eb26..39b9374d3a1e 100644
--- a/lib/seq_buf.c
+++ b/lib/seq_buf.c
@@ -244,13 +244,14 @@ int seq_buf_putmem_hex(struct seq_buf *s, const void *mem,
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;
+
+ len -= start_len;
+ data += start_len;
}
return 0;
}
Why are you making it so complicated?
-- Steve
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
