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

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

[Yun Zhou]

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

[PATCH 2/2] seq_buf: Make trace_seq_putmem_hex() support data longer than 8

[Yun Zhou]

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

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

[Steven Rostedt]

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

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

[Zhou, Yun]

[-- 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 --]

[PATCH 2/2] seq_buf: Make trace_seq_putmem_hex() support data longer than 8

[Yun Zhou]

Since the raw memory 'data' does not go forward, it will dump repeated
data if the data length is more than 8. 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 improve the commit 6d2289f3faa7 ("tracing: Make
trace_seq_putmem_hex() more robust").

Signed-off-by: Yun Zhou <yun.zhou@windriver.com>
---
 lib/seq_buf.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/seq_buf.c b/lib/seq_buf.c
index 223fbc3bb958..562e53c93b7b 100644
--- a/lib/seq_buf.c
+++ b/lib/seq_buf.c
@@ -244,12 +244,14 @@ int seq_buf_putmem_hex(struct seq_buf *s, const void *mem,
 			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;
 }
-- 
2.26.1