* [PATCH 1/2] seq_buf: fix overflow when length is bigger than 8 @ 2021-06-25 15:53 Yun Zhou 2021-06-25 15:53 ` [PATCH 2/2] seq_buf: Make trace_seq_putmem_hex() support data longer " Yun Zhou 2021-06-25 16:24 ` [PATCH 1/2] seq_buf: fix overflow when length is bigger " Steven Rostedt 0 siblings, 2 replies; 4+ messages in thread From: Yun Zhou @ 2021-06-25 15:53 UTC (permalink / raw) To: rostedt; +Cc: linux-kernel, kernel-hardening, ying.xue, zhiquan.li 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(-) 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; } -- 2.26.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] seq_buf: Make trace_seq_putmem_hex() support data longer than 8 2021-06-25 15:53 [PATCH 1/2] seq_buf: fix overflow when length is bigger than 8 Yun Zhou @ 2021-06-25 15:53 ` Yun Zhou 2021-06-25 16:24 ` [PATCH 1/2] seq_buf: fix overflow when length is bigger " Steven Rostedt 1 sibling, 0 replies; 4+ messages in thread From: Yun Zhou @ 2021-06-25 15:53 UTC (permalink / raw) To: rostedt; +Cc: linux-kernel, kernel-hardening, ying.xue, zhiquan.li At present, trace_seq_putmem_hex() can only support data with length of 8 or less, which greatly limits its application scope. If we want to dump longer data blocks, we need to repeatedly call macro SEQ_PUT_HEX_FIELD. I think it is a bit redundant, and multiple function calls also affect the performance. This patch is to perfect the commit 6d2289f3faa7 ("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, 18 insertions(+), 11 deletions(-) diff --git a/lib/seq_buf.c b/lib/seq_buf.c index aa2f666e584e..98580a5c32c0 100644 --- a/lib/seq_buf.c +++ b/lib/seq_buf.c @@ -210,8 +210,7 @@ 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). - * It can be not larger than 8. + * @len: The length of the raw memory to copy (in bytes) * * 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 @@ -229,19 +228,27 @@ int seq_buf_putmem_hex(struct seq_buf *s, const void *mem, WARN_ON(s->size == 0); - start_len = min(len, MAX_MEMHEX_BYTES); + while (len) { + 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]); - } + 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; + /* j increments twice per loop */ + len -= j / 2; + hex[j++] = ' '; + + seq_buf_putmem(s, hex, j); + if (seq_buf_has_overflowed(s)) + return -1; + + data += start_len; + } return 0; } -- 2.26.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] seq_buf: fix overflow when length is bigger than 8 2021-06-25 15:53 [PATCH 1/2] seq_buf: fix overflow when length is bigger than 8 Yun Zhou 2021-06-25 15:53 ` [PATCH 2/2] seq_buf: Make trace_seq_putmem_hex() support data longer " Yun Zhou @ 2021-06-25 16:24 ` Steven Rostedt 2021-06-26 0:57 ` 回复: " Zhou, Yun 1 sibling, 1 reply; 4+ messages in thread From: Steven Rostedt @ 2021-06-25 16:24 UTC (permalink / raw) To: Yun Zhou; +Cc: linux-kernel, kernel-hardening, ying.xue, zhiquan.li 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 -- 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* 回复: [PATCH 1/2] seq_buf: fix overflow when length is bigger than 8 2021-06-25 16:24 ` [PATCH 1/2] seq_buf: fix overflow when length is bigger " Steven Rostedt @ 2021-06-26 0:57 ` Zhou, Yun 0 siblings, 0 replies; 4+ messages in thread From: Zhou, Yun @ 2021-06-26 0:57 UTC (permalink / raw) To: Steven Rostedt; +Cc: linux-kernel, kernel-hardening, Xue, Ying, Li, Zhiquan [-- Attachment #1: Type: text/plain, Size: 6541 bytes --] 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@goodmis.org> 发送时间: 2021年6月26日 0:24 收件人: Zhou, Yun <Yun.Zhou@windriver.com> 抄送: linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; kernel-hardening@lists.openwall.com <kernel-hardening@lists.openwall.com>; Xue, Ying <Ying.Xue@windriver.com>; Li, Zhiquan <Zhiquan.Li@windriver.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@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 -- 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 [-- Attachment #2: Type: text/html, Size: 13626 bytes --] ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-06-26 9:37 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-25 15:53 [PATCH 1/2] seq_buf: fix overflow when length is bigger than 8 Yun Zhou 2021-06-25 15:53 ` [PATCH 2/2] seq_buf: Make trace_seq_putmem_hex() support data longer " Yun Zhou 2021-06-25 16:24 ` [PATCH 1/2] seq_buf: fix overflow when length is bigger " Steven Rostedt 2021-06-26 0:57 ` 回复: " Zhou, Yun
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.