kernel-hardening.lists.openwall.com archive mirror
 help / color / mirror / Atom feed
* [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	[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	[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	[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	[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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).