回复: [PATCH 1/2] seq_buf: fix overflow when length is bigger than 8

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux