|
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@xxxxxxxxxxx>
发送时间: 2021年6月26日 0:24 收件人: Zhou, Yun <Yun.Zhou@xxxxxxxxxxxxx> 抄送: linux-kernel@xxxxxxxxxxxxxxx <linux-kernel@xxxxxxxxxxxxxxx>; kernel-hardening@xxxxxxxxxxxxxxxxxx <kernel-hardening@xxxxxxxxxxxxxxxxxx>; Xue, Ying <Ying.Xue@xxxxxxxxxxxxx>; Li, Zhiquan <Zhiquan.Li@xxxxxxxxxxxxx> 主题: 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@xxxxxxxxxxxxx> 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 = ""> > > #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@xxxxxxxxxxxxx> > --- > 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 |
