[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID:
<SN6PR11MB3008E8AED96764C86DDC2F6C9F059@SN6PR11MB3008.namprd11.prod.outlook.com>
Date: Sat, 26 Jun 2021 00:57:18 +0000
From: "Zhou, Yun" <Yun.Zhou@...driver.com>
To: Steven Rostedt <rostedt@...dmis.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>,
"Xue, Ying" <Ying.Xue@...driver.com>,
"Li, Zhiquan"
<Zhiquan.Li@...driver.com>
Subject:
回复: [PATCH 1/2] seq_buf: fix overflow when length is bigger than 8
Hi Steve,
Thanks very much for your very careful and clear reply. Your suggestions are very helpful. I was not sure whether you would accept the enhancement patch before, so I fixed the bug more thoroughly, which is really complicated.
I will follow your suggestions and requirements to redo the patch ASAP.
Best Regards,
Yun
________________________________
发件人: Steven Rostedt <rostedt@...dmis.org>
发送时间: 2021年6月26日 0:24
收件人: Zhou, Yun <Yun.Zhou@...driver.com>
抄送: linux-kernel@...r.kernel.org <linux-kernel@...r.kernel.org>; kernel-hardening@...ts.openwall.com <kernel-hardening@...ts.openwall.com>; Xue, Ying <Ying.Xue@...driver.com>; Li, Zhiquan <Zhiquan.Li@...driver.com>
主题: Re: [PATCH 1/2] seq_buf: fix overflow when length is bigger than 8
[Please note: This e-mail is from an EXTERNAL e-mail address]
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
Content of type "text/html" skipped
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.
