All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ocfs2: mount fails with buffer overflow in strlen
@ 2021-09-27 15:44 ` Valentin Vidic
  0 siblings, 0 replies; 17+ messages in thread
From: Valentin Vidic @ 2021-09-27 15:44 UTC (permalink / raw)
  To: Mark Fasheh, Joel Becker, Joseph Qi, ocfs2-devel, linux-kernel
  Cc: Valentin Vidic

Starting with kernel v5.11 mouting an ocfs2 filesystem with either o2cb
or pcmk cluster stack fails with the trace below. Problem seems to be
that strings for cluster stack and cluster name are not guaranteed to be
null terminated in the disk representation, while strlcpy assumes that
the source string is always null terminated. This causes a read outside
of the source string triggering the buffer overflow detection.

detected buffer overflow in strlen
------------[ cut here ]------------
kernel BUG at lib/string.c:1149!
invalid opcode: 0000 [#1] SMP PTI
CPU: 1 PID: 910 Comm: mount.ocfs2 Not tainted 5.14.0-1-amd64 #1
  Debian 5.14.6-2
RIP: 0010:fortify_panic+0xf/0x11
...
Call Trace:
 ocfs2_initialize_super.isra.0.cold+0xc/0x18 [ocfs2]
 ocfs2_fill_super+0x359/0x19b0 [ocfs2]
 mount_bdev+0x185/0x1b0
 ? ocfs2_remount+0x440/0x440 [ocfs2]
 legacy_get_tree+0x27/0x40
 vfs_get_tree+0x25/0xb0
 path_mount+0x454/0xa20
 __x64_sys_mount+0x103/0x140
 do_syscall_64+0x3b/0xc0
 entry_SYSCALL_64_after_hwframe+0x44/0xae

Signed-off-by: Valentin Vidic <vvidic@valentin-vidic.from.hr>
---
 fs/ocfs2/super.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
index c86bd4e60e20..1dea535224df 100644
--- a/fs/ocfs2/super.c
+++ b/fs/ocfs2/super.c
@@ -2169,9 +2169,10 @@ static int ocfs2_initialize_super(struct super_block *sb,
 	if (ocfs2_clusterinfo_valid(osb)) {
 		osb->osb_stackflags =
 			OCFS2_RAW_SB(di)->s_cluster_info.ci_stackflags;
-		strlcpy(osb->osb_cluster_stack,
+		memcpy(osb->osb_cluster_stack,
 		       OCFS2_RAW_SB(di)->s_cluster_info.ci_stack,
-		       OCFS2_STACK_LABEL_LEN + 1);
+		       OCFS2_STACK_LABEL_LEN);
+		osb->osb_cluster_stack[OCFS2_STACK_LABEL_LEN] = '\0';
 		if (strlen(osb->osb_cluster_stack) != OCFS2_STACK_LABEL_LEN) {
 			mlog(ML_ERROR,
 			     "couldn't mount because of an invalid "
@@ -2180,9 +2181,10 @@ static int ocfs2_initialize_super(struct super_block *sb,
 			status = -EINVAL;
 			goto bail;
 		}
-		strlcpy(osb->osb_cluster_name,
+		memcpy(osb->osb_cluster_name,
 			OCFS2_RAW_SB(di)->s_cluster_info.ci_cluster,
-			OCFS2_CLUSTER_NAME_LEN + 1);
+			OCFS2_CLUSTER_NAME_LEN);
+		osb->osb_cluster_name[OCFS2_CLUSTER_NAME_LEN] = '\0';
 	} else {
 		/* The empty string is identical with classic tools that
 		 * don't know about s_cluster_info. */
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Ocfs2-devel] [PATCH] ocfs2: mount fails with buffer overflow in strlen
@ 2021-09-27 15:44 ` Valentin Vidic
  0 siblings, 0 replies; 17+ messages in thread
From: Valentin Vidic @ 2021-09-27 15:44 UTC (permalink / raw)
  To: Mark Fasheh, Joel Becker, Joseph Qi, ocfs2-devel, linux-kernel
  Cc: Valentin Vidic

Starting with kernel v5.11 mouting an ocfs2 filesystem with either o2cb
or pcmk cluster stack fails with the trace below. Problem seems to be
that strings for cluster stack and cluster name are not guaranteed to be
null terminated in the disk representation, while strlcpy assumes that
the source string is always null terminated. This causes a read outside
of the source string triggering the buffer overflow detection.

detected buffer overflow in strlen
------------[ cut here ]------------
kernel BUG at lib/string.c:1149!
invalid opcode: 0000 [#1] SMP PTI
CPU: 1 PID: 910 Comm: mount.ocfs2 Not tainted 5.14.0-1-amd64 #1
  Debian 5.14.6-2
RIP: 0010:fortify_panic+0xf/0x11
...
Call Trace:
 ocfs2_initialize_super.isra.0.cold+0xc/0x18 [ocfs2]
 ocfs2_fill_super+0x359/0x19b0 [ocfs2]
 mount_bdev+0x185/0x1b0
 ? ocfs2_remount+0x440/0x440 [ocfs2]
 legacy_get_tree+0x27/0x40
 vfs_get_tree+0x25/0xb0
 path_mount+0x454/0xa20
 __x64_sys_mount+0x103/0x140
 do_syscall_64+0x3b/0xc0
 entry_SYSCALL_64_after_hwframe+0x44/0xae

Signed-off-by: Valentin Vidic <vvidic@valentin-vidic.from.hr>
---
 fs/ocfs2/super.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
index c86bd4e60e20..1dea535224df 100644
--- a/fs/ocfs2/super.c
+++ b/fs/ocfs2/super.c
@@ -2169,9 +2169,10 @@ static int ocfs2_initialize_super(struct super_block *sb,
 	if (ocfs2_clusterinfo_valid(osb)) {
 		osb->osb_stackflags =
 			OCFS2_RAW_SB(di)->s_cluster_info.ci_stackflags;
-		strlcpy(osb->osb_cluster_stack,
+		memcpy(osb->osb_cluster_stack,
 		       OCFS2_RAW_SB(di)->s_cluster_info.ci_stack,
-		       OCFS2_STACK_LABEL_LEN + 1);
+		       OCFS2_STACK_LABEL_LEN);
+		osb->osb_cluster_stack[OCFS2_STACK_LABEL_LEN] = '\0';
 		if (strlen(osb->osb_cluster_stack) != OCFS2_STACK_LABEL_LEN) {
 			mlog(ML_ERROR,
 			     "couldn't mount because of an invalid "
@@ -2180,9 +2181,10 @@ static int ocfs2_initialize_super(struct super_block *sb,
 			status = -EINVAL;
 			goto bail;
 		}
-		strlcpy(osb->osb_cluster_name,
+		memcpy(osb->osb_cluster_name,
 			OCFS2_RAW_SB(di)->s_cluster_info.ci_cluster,
-			OCFS2_CLUSTER_NAME_LEN + 1);
+			OCFS2_CLUSTER_NAME_LEN);
+		osb->osb_cluster_name[OCFS2_CLUSTER_NAME_LEN] = '\0';
 	} else {
 		/* The empty string is identical with classic tools that
 		 * don't know about s_cluster_info. */
-- 
2.30.2


_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH] ocfs2: mount fails with buffer overflow in strlen
  2021-09-27 15:44 ` [Ocfs2-devel] " Valentin Vidic
@ 2021-09-28 12:05   ` Joseph Qi
  -1 siblings, 0 replies; 17+ messages in thread
From: Joseph Qi @ 2021-09-28 12:05 UTC (permalink / raw)
  To: Valentin Vidic, Mark Fasheh, Joel Becker, ocfs2-devel, linux-kernel



On 9/27/21 11:44 PM, Valentin Vidic wrote:
> Starting with kernel v5.11 mouting an ocfs2 filesystem with either o2cb
> or pcmk cluster stack fails with the trace below. Problem seems to be
> that strings for cluster stack and cluster name are not guaranteed to be
> null terminated in the disk representation, while strlcpy assumes that
> the source string is always null terminated. This causes a read outside
> of the source string triggering the buffer overflow detection.
> 

strlcpy in ocfs2_initialize_super() is introduced 8 years ago, so I
don't understand why you've mentioned that the issues starts from
v5.11.

osb->osb_cluster_stack and osb->osb_cluster_name is always larger by
1 than which in ocfs2_cluster_info, and the input size of strlcpy does
the same, so I don't see how it overflows.

Thanks,
Joseph

> detected buffer overflow in strlen
> ------------[ cut here ]------------
> kernel BUG at lib/string.c:1149!
> invalid opcode: 0000 [#1] SMP PTI
> CPU: 1 PID: 910 Comm: mount.ocfs2 Not tainted 5.14.0-1-amd64 #1
>   Debian 5.14.6-2
> RIP: 0010:fortify_panic+0xf/0x11
> ...
> Call Trace:
>  ocfs2_initialize_super.isra.0.cold+0xc/0x18 [ocfs2]
>  ocfs2_fill_super+0x359/0x19b0 [ocfs2]
>  mount_bdev+0x185/0x1b0
>  ? ocfs2_remount+0x440/0x440 [ocfs2]
>  legacy_get_tree+0x27/0x40
>  vfs_get_tree+0x25/0xb0
>  path_mount+0x454/0xa20
>  __x64_sys_mount+0x103/0x140
>  do_syscall_64+0x3b/0xc0
>  entry_SYSCALL_64_after_hwframe+0x44/0xae
> 
> Signed-off-by: Valentin Vidic <vvidic@valentin-vidic.from.hr>
> ---
>  fs/ocfs2/super.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
> index c86bd4e60e20..1dea535224df 100644
> --- a/fs/ocfs2/super.c
> +++ b/fs/ocfs2/super.c
> @@ -2169,9 +2169,10 @@ static int ocfs2_initialize_super(struct super_block *sb,
>  	if (ocfs2_clusterinfo_valid(osb)) {
>  		osb->osb_stackflags =
>  			OCFS2_RAW_SB(di)->s_cluster_info.ci_stackflags;
> -		strlcpy(osb->osb_cluster_stack,
> +		memcpy(osb->osb_cluster_stack,
>  		       OCFS2_RAW_SB(di)->s_cluster_info.ci_stack,
> -		       OCFS2_STACK_LABEL_LEN + 1);
> +		       OCFS2_STACK_LABEL_LEN);
> +		osb->osb_cluster_stack[OCFS2_STACK_LABEL_LEN] = '\0';
>  		if (strlen(osb->osb_cluster_stack) != OCFS2_STACK_LABEL_LEN) {
>  			mlog(ML_ERROR,
>  			     "couldn't mount because of an invalid "
> @@ -2180,9 +2181,10 @@ static int ocfs2_initialize_super(struct super_block *sb,
>  			status = -EINVAL;
>  			goto bail;
>  		}
> -		strlcpy(osb->osb_cluster_name,
> +		memcpy(osb->osb_cluster_name,
>  			OCFS2_RAW_SB(di)->s_cluster_info.ci_cluster,
> -			OCFS2_CLUSTER_NAME_LEN + 1);
> +			OCFS2_CLUSTER_NAME_LEN);
> +		osb->osb_cluster_name[OCFS2_CLUSTER_NAME_LEN] = '\0';
>  	} else {
>  		/* The empty string is identical with classic tools that
>  		 * don't know about s_cluster_info. */
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Ocfs2-devel] [PATCH] ocfs2: mount fails with buffer overflow in strlen
@ 2021-09-28 12:05   ` Joseph Qi
  0 siblings, 0 replies; 17+ messages in thread
From: Joseph Qi @ 2021-09-28 12:05 UTC (permalink / raw)
  To: Valentin Vidic, Mark Fasheh, Joel Becker, ocfs2-devel, linux-kernel



On 9/27/21 11:44 PM, Valentin Vidic wrote:
> Starting with kernel v5.11 mouting an ocfs2 filesystem with either o2cb
> or pcmk cluster stack fails with the trace below. Problem seems to be
> that strings for cluster stack and cluster name are not guaranteed to be
> null terminated in the disk representation, while strlcpy assumes that
> the source string is always null terminated. This causes a read outside
> of the source string triggering the buffer overflow detection.
> 

strlcpy in ocfs2_initialize_super() is introduced 8 years ago, so I
don't understand why you've mentioned that the issues starts from
v5.11.

osb->osb_cluster_stack and osb->osb_cluster_name is always larger by
1 than which in ocfs2_cluster_info, and the input size of strlcpy does
the same, so I don't see how it overflows.

Thanks,
Joseph

> detected buffer overflow in strlen
> ------------[ cut here ]------------
> kernel BUG at lib/string.c:1149!
> invalid opcode: 0000 [#1] SMP PTI
> CPU: 1 PID: 910 Comm: mount.ocfs2 Not tainted 5.14.0-1-amd64 #1
>   Debian 5.14.6-2
> RIP: 0010:fortify_panic+0xf/0x11
> ...
> Call Trace:
>  ocfs2_initialize_super.isra.0.cold+0xc/0x18 [ocfs2]
>  ocfs2_fill_super+0x359/0x19b0 [ocfs2]
>  mount_bdev+0x185/0x1b0
>  ? ocfs2_remount+0x440/0x440 [ocfs2]
>  legacy_get_tree+0x27/0x40
>  vfs_get_tree+0x25/0xb0
>  path_mount+0x454/0xa20
>  __x64_sys_mount+0x103/0x140
>  do_syscall_64+0x3b/0xc0
>  entry_SYSCALL_64_after_hwframe+0x44/0xae
> 
> Signed-off-by: Valentin Vidic <vvidic@valentin-vidic.from.hr>
> ---
>  fs/ocfs2/super.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
> index c86bd4e60e20..1dea535224df 100644
> --- a/fs/ocfs2/super.c
> +++ b/fs/ocfs2/super.c
> @@ -2169,9 +2169,10 @@ static int ocfs2_initialize_super(struct super_block *sb,
>  	if (ocfs2_clusterinfo_valid(osb)) {
>  		osb->osb_stackflags =
>  			OCFS2_RAW_SB(di)->s_cluster_info.ci_stackflags;
> -		strlcpy(osb->osb_cluster_stack,
> +		memcpy(osb->osb_cluster_stack,
>  		       OCFS2_RAW_SB(di)->s_cluster_info.ci_stack,
> -		       OCFS2_STACK_LABEL_LEN + 1);
> +		       OCFS2_STACK_LABEL_LEN);
> +		osb->osb_cluster_stack[OCFS2_STACK_LABEL_LEN] = '\0';
>  		if (strlen(osb->osb_cluster_stack) != OCFS2_STACK_LABEL_LEN) {
>  			mlog(ML_ERROR,
>  			     "couldn't mount because of an invalid "
> @@ -2180,9 +2181,10 @@ static int ocfs2_initialize_super(struct super_block *sb,
>  			status = -EINVAL;
>  			goto bail;
>  		}
> -		strlcpy(osb->osb_cluster_name,
> +		memcpy(osb->osb_cluster_name,
>  			OCFS2_RAW_SB(di)->s_cluster_info.ci_cluster,
> -			OCFS2_CLUSTER_NAME_LEN + 1);
> +			OCFS2_CLUSTER_NAME_LEN);
> +		osb->osb_cluster_name[OCFS2_CLUSTER_NAME_LEN] = '\0';
>  	} else {
>  		/* The empty string is identical with classic tools that
>  		 * don't know about s_cluster_info. */
> 

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] ocfs2: mount fails with buffer overflow in strlen
  2021-09-28 12:05   ` [Ocfs2-devel] " Joseph Qi
@ 2021-09-28 13:14     ` Valentin Vidić
  -1 siblings, 0 replies; 17+ messages in thread
From: Valentin Vidić @ 2021-09-28 13:14 UTC (permalink / raw)
  To: Joseph Qi; +Cc: Mark Fasheh, Joel Becker, ocfs2-devel, linux-kernel

On Tue, Sep 28, 2021 at 08:05:22PM +0800, Joseph Qi wrote:
> strlcpy in ocfs2_initialize_super() is introduced 8 years ago, so I
> don't understand why you've mentioned that the issues starts from
> v5.11.

v5.11 introduced the overflow checks to string functions so that is
when the mount started to fail.

> osb->osb_cluster_stack and osb->osb_cluster_name is always larger by
> 1 than which in ocfs2_cluster_info, and the input size of strlcpy does
> the same, so I don't see how it overflows.

strlcpy internally calls strlen on the source argument, in this case
that is ci_stack array with size of 4. That array stores the value
"o2cb" so the strlen continues reading into the union until it reaches
a zero byte somewhere. The same would happen with ci_cluster if the
cluster name is long enough.

struct ocfs2_cluster_info {
/*00*/	__u8   ci_stack[OCFS2_STACK_LABEL_LEN];
	union {
		__le32 ci_reserved;
		struct {
			__u8 ci_stackflags;
			__u8 ci_reserved1;
			__u8 ci_reserved2;
			__u8 ci_reserved3;
		};
	};
/*08*/	__u8   ci_cluster[OCFS2_CLUSTER_NAME_LEN];
/*18*/
};

-- 
Valentin

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Ocfs2-devel] [PATCH] ocfs2: mount fails with buffer overflow in strlen
@ 2021-09-28 13:14     ` Valentin Vidić
  0 siblings, 0 replies; 17+ messages in thread
From: Valentin Vidić @ 2021-09-28 13:14 UTC (permalink / raw)
  To: Joseph Qi; +Cc: ocfs2-devel, linux-kernel

On Tue, Sep 28, 2021 at 08:05:22PM +0800, Joseph Qi wrote:
> strlcpy in ocfs2_initialize_super() is introduced 8 years ago, so I
> don't understand why you've mentioned that the issues starts from
> v5.11.

v5.11 introduced the overflow checks to string functions so that is
when the mount started to fail.

> osb->osb_cluster_stack and osb->osb_cluster_name is always larger by
> 1 than which in ocfs2_cluster_info, and the input size of strlcpy does
> the same, so I don't see how it overflows.

strlcpy internally calls strlen on the source argument, in this case
that is ci_stack array with size of 4. That array stores the value
"o2cb" so the strlen continues reading into the union until it reaches
a zero byte somewhere. The same would happen with ci_cluster if the
cluster name is long enough.

struct ocfs2_cluster_info {
/*00*/	__u8   ci_stack[OCFS2_STACK_LABEL_LEN];
	union {
		__le32 ci_reserved;
		struct {
			__u8 ci_stackflags;
			__u8 ci_reserved1;
			__u8 ci_reserved2;
			__u8 ci_reserved3;
		};
	};
/*08*/	__u8   ci_cluster[OCFS2_CLUSTER_NAME_LEN];
/*18*/
};

-- 
Valentin

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] ocfs2: mount fails with buffer overflow in strlen
  2021-09-28 13:14     ` [Ocfs2-devel] " Valentin Vidić
@ 2021-09-29  2:38       ` Joseph Qi
  -1 siblings, 0 replies; 17+ messages in thread
From: Joseph Qi @ 2021-09-29  2:38 UTC (permalink / raw)
  To: Valentin Vidić; +Cc: Mark Fasheh, Joel Becker, ocfs2-devel, linux-kernel

Okay, you are right, strlen(src) is indeed wrong here.

But please note that in strlcpy():
if (size) {
	size_t len = (ret >= size) ? size - 1 : ret;
	memcpy(dest, src, len);
	dest[len] = '\0';
}

Take ci_stack "o2cb" for example, strlen("o2cb") may return wrong if the
coming byte is not null, say it is 10.
The input size is 5, so len will finally be 4.
So dest is still correct ending with null byte. No overflow happens.
So the problem here is the wrong return value, but it is discarded in
ocfs2_initialize_super().

Thanks,
Joseph

On 9/28/21 9:14 PM, Valentin Vidić wrote:
> On Tue, Sep 28, 2021 at 08:05:22PM +0800, Joseph Qi wrote:
>> strlcpy in ocfs2_initialize_super() is introduced 8 years ago, so I
>> don't understand why you've mentioned that the issues starts from
>> v5.11.
> 
> v5.11 introduced the overflow checks to string functions so that is
> when the mount started to fail.
> 
>> osb->osb_cluster_stack and osb->osb_cluster_name is always larger by
>> 1 than which in ocfs2_cluster_info, and the input size of strlcpy does
>> the same, so I don't see how it overflows.
> 
> strlcpy internally calls strlen on the source argument, in this case
> that is ci_stack array with size of 4. That array stores the value
> "o2cb" so the strlen continues reading into the union until it reaches
> a zero byte somewhere. The same would happen with ci_cluster if the
> cluster name is long enough.
> 
> struct ocfs2_cluster_info {
> /*00*/	__u8   ci_stack[OCFS2_STACK_LABEL_LEN];
> 	union {
> 		__le32 ci_reserved;
> 		struct {
> 			__u8 ci_stackflags;
> 			__u8 ci_reserved1;
> 			__u8 ci_reserved2;
> 			__u8 ci_reserved3;
> 		};
> 	};
> /*08*/	__u8   ci_cluster[OCFS2_CLUSTER_NAME_LEN];
> /*18*/
> };
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Ocfs2-devel] [PATCH] ocfs2: mount fails with buffer overflow in strlen
@ 2021-09-29  2:38       ` Joseph Qi
  0 siblings, 0 replies; 17+ messages in thread
From: Joseph Qi @ 2021-09-29  2:38 UTC (permalink / raw)
  To: Valentin Vidić; +Cc: ocfs2-devel, linux-kernel

Okay, you are right, strlen(src) is indeed wrong here.

But please note that in strlcpy():
if (size) {
	size_t len = (ret >= size) ? size - 1 : ret;
	memcpy(dest, src, len);
	dest[len] = '\0';
}

Take ci_stack "o2cb" for example, strlen("o2cb") may return wrong if the
coming byte is not null, say it is 10.
The input size is 5, so len will finally be 4.
So dest is still correct ending with null byte. No overflow happens.
So the problem here is the wrong return value, but it is discarded in
ocfs2_initialize_super().

Thanks,
Joseph

On 9/28/21 9:14 PM, Valentin Vidić wrote:
> On Tue, Sep 28, 2021 at 08:05:22PM +0800, Joseph Qi wrote:
>> strlcpy in ocfs2_initialize_super() is introduced 8 years ago, so I
>> don't understand why you've mentioned that the issues starts from
>> v5.11.
> 
> v5.11 introduced the overflow checks to string functions so that is
> when the mount started to fail.
> 
>> osb->osb_cluster_stack and osb->osb_cluster_name is always larger by
>> 1 than which in ocfs2_cluster_info, and the input size of strlcpy does
>> the same, so I don't see how it overflows.
> 
> strlcpy internally calls strlen on the source argument, in this case
> that is ci_stack array with size of 4. That array stores the value
> "o2cb" so the strlen continues reading into the union until it reaches
> a zero byte somewhere. The same would happen with ci_cluster if the
> cluster name is long enough.
> 
> struct ocfs2_cluster_info {
> /*00*/	__u8   ci_stack[OCFS2_STACK_LABEL_LEN];
> 	union {
> 		__le32 ci_reserved;
> 		struct {
> 			__u8 ci_stackflags;
> 			__u8 ci_reserved1;
> 			__u8 ci_reserved2;
> 			__u8 ci_reserved3;
> 		};
> 	};
> /*08*/	__u8   ci_cluster[OCFS2_CLUSTER_NAME_LEN];
> /*18*/
> };
> 

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] ocfs2: mount fails with buffer overflow in strlen
  2021-09-29  2:38       ` [Ocfs2-devel] " Joseph Qi
@ 2021-09-29  6:24         ` Valentin Vidić
  -1 siblings, 0 replies; 17+ messages in thread
From: Valentin Vidić @ 2021-09-29  6:24 UTC (permalink / raw)
  To: Joseph Qi; +Cc: Mark Fasheh, Joel Becker, ocfs2-devel, linux-kernel

On Wed, Sep 29, 2021 at 10:38:59AM +0800, Joseph Qi wrote:
> Okay, you are right, strlen(src) is indeed wrong here.
> 
> But please note that in strlcpy():
> size_t ret = strlen(src);
> if (size) {
> 	size_t len = (ret >= size) ? size - 1 : ret;
> 	memcpy(dest, src, len);
> 	dest[len] = '\0';
> }
> 
> Take ci_stack "o2cb" for example, strlen("o2cb") may return wrong if the
> coming byte is not null, say it is 10.
> The input size is 5, so len will finally be 4.
> So dest is still correct ending with null byte. No overflow happens.
> So the problem here is the wrong return value, but it is discarded in
> ocfs2_initialize_super().

strlcpy starts with a call to strlen(src) and this is where the read overflow
happens. If the kernel is compiled with CONFIG_FORTIFY_SOURCE this gets
executed instead (include/linux/fortify-string.h):

__FORTIFY_INLINE __kernel_size_t strlen(const char *p)
{
        __kernel_size_t ret;
        size_t p_size = __builtin_object_size(p, 1);

        /* Work around gcc excess stack consumption issue */
        if (p_size == (size_t)-1 ||
                (__builtin_constant_p(p[p_size - 1]) && p[p_size - 1] == '\0'))
                return __underlying_strlen(p);
        ret = strnlen(p, p_size);
        if (p_size <= ret)
                fortify_panic(__func__);
        return ret;
}

So while strlcpy did work before this fortify check, it is probably not the
best option anymore due to the missing null terminator in the source.

-- 
Valentin

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Ocfs2-devel] [PATCH] ocfs2: mount fails with buffer overflow in strlen
@ 2021-09-29  6:24         ` Valentin Vidić
  0 siblings, 0 replies; 17+ messages in thread
From: Valentin Vidić @ 2021-09-29  6:24 UTC (permalink / raw)
  To: Joseph Qi; +Cc: ocfs2-devel, linux-kernel

On Wed, Sep 29, 2021 at 10:38:59AM +0800, Joseph Qi wrote:
> Okay, you are right, strlen(src) is indeed wrong here.
> 
> But please note that in strlcpy():
> size_t ret = strlen(src);
> if (size) {
> 	size_t len = (ret >= size) ? size - 1 : ret;
> 	memcpy(dest, src, len);
> 	dest[len] = '\0';
> }
> 
> Take ci_stack "o2cb" for example, strlen("o2cb") may return wrong if the
> coming byte is not null, say it is 10.
> The input size is 5, so len will finally be 4.
> So dest is still correct ending with null byte. No overflow happens.
> So the problem here is the wrong return value, but it is discarded in
> ocfs2_initialize_super().

strlcpy starts with a call to strlen(src) and this is where the read overflow
happens. If the kernel is compiled with CONFIG_FORTIFY_SOURCE this gets
executed instead (include/linux/fortify-string.h):

__FORTIFY_INLINE __kernel_size_t strlen(const char *p)
{
        __kernel_size_t ret;
        size_t p_size = __builtin_object_size(p, 1);

        /* Work around gcc excess stack consumption issue */
        if (p_size == (size_t)-1 ||
                (__builtin_constant_p(p[p_size - 1]) && p[p_size - 1] == '\0'))
                return __underlying_strlen(p);
        ret = strnlen(p, p_size);
        if (p_size <= ret)
                fortify_panic(__func__);
        return ret;
}

So while strlcpy did work before this fortify check, it is probably not the
best option anymore due to the missing null terminator in the source.

-- 
Valentin

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] ocfs2: mount fails with buffer overflow in strlen
  2021-09-29  6:24         ` [Ocfs2-devel] " Valentin Vidić
@ 2021-09-29  9:12           ` Joseph Qi
  -1 siblings, 0 replies; 17+ messages in thread
From: Joseph Qi @ 2021-09-29  9:12 UTC (permalink / raw)
  To: Valentin Vidić; +Cc: Mark Fasheh, Joel Becker, ocfs2-devel, linux-kernel



On 9/29/21 2:24 PM, Valentin Vidić wrote:
> On Wed, Sep 29, 2021 at 10:38:59AM +0800, Joseph Qi wrote:
>> Okay, you are right, strlen(src) is indeed wrong here.
>>
>> But please note that in strlcpy():
>> size_t ret = strlen(src);
>> if (size) {
>> 	size_t len = (ret >= size) ? size - 1 : ret;
>> 	memcpy(dest, src, len);
>> 	dest[len] = '\0';
>> }
>>
>> Take ci_stack "o2cb" for example, strlen("o2cb") may return wrong if the
>> coming byte is not null, say it is 10.
>> The input size is 5, so len will finally be 4.
>> So dest is still correct ending with null byte. No overflow happens.
>> So the problem here is the wrong return value, but it is discarded in
>> ocfs2_initialize_super().
> 
> strlcpy starts with a call to strlen(src) and this is where the read overflow
> happens. If the kernel is compiled with CONFIG_FORTIFY_SOURCE this gets
> executed instead (include/linux/fortify-string.h):
> 
> __FORTIFY_INLINE __kernel_size_t strlen(const char *p)
> {
>         __kernel_size_t ret;
>         size_t p_size = __builtin_object_size(p, 1);
> 
>         /* Work around gcc excess stack consumption issue */
>         if (p_size == (size_t)-1 ||
>                 (__builtin_constant_p(p[p_size - 1]) && p[p_size - 1] == '\0'))
>                 return __underlying_strlen(p);
>         ret = strnlen(p, p_size);
>         if (p_size <= ret)
>                 fortify_panic(__func__);
>         return ret;
> }
> 
> So while strlcpy did work before this fortify check, it is probably not the
> best option anymore due to the missing null terminator in the source.
> 
Got it, it really triggers panic in strlen().
So could you please update the commit log? I think CONFIG_FORTIFY_SOURCE
is necessary information since it is not default enabled.
And add comments with your changes, e.g.

/*
 * ci_stack and ci_cluster in ocfs2_cluster_info may not null
 * terminated, make sure no overflow happens here.
 */

BTW, since we use kzalloc to alloc osb, so we don't have to manually
set the last null byte.

Thanks,
Joseph

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Ocfs2-devel] [PATCH] ocfs2: mount fails with buffer overflow in strlen
@ 2021-09-29  9:12           ` Joseph Qi
  0 siblings, 0 replies; 17+ messages in thread
From: Joseph Qi @ 2021-09-29  9:12 UTC (permalink / raw)
  To: Valentin Vidić; +Cc: ocfs2-devel, linux-kernel



On 9/29/21 2:24 PM, Valentin Vidić wrote:
> On Wed, Sep 29, 2021 at 10:38:59AM +0800, Joseph Qi wrote:
>> Okay, you are right, strlen(src) is indeed wrong here.
>>
>> But please note that in strlcpy():
>> size_t ret = strlen(src);
>> if (size) {
>> 	size_t len = (ret >= size) ? size - 1 : ret;
>> 	memcpy(dest, src, len);
>> 	dest[len] = '\0';
>> }
>>
>> Take ci_stack "o2cb" for example, strlen("o2cb") may return wrong if the
>> coming byte is not null, say it is 10.
>> The input size is 5, so len will finally be 4.
>> So dest is still correct ending with null byte. No overflow happens.
>> So the problem here is the wrong return value, but it is discarded in
>> ocfs2_initialize_super().
> 
> strlcpy starts with a call to strlen(src) and this is where the read overflow
> happens. If the kernel is compiled with CONFIG_FORTIFY_SOURCE this gets
> executed instead (include/linux/fortify-string.h):
> 
> __FORTIFY_INLINE __kernel_size_t strlen(const char *p)
> {
>         __kernel_size_t ret;
>         size_t p_size = __builtin_object_size(p, 1);
> 
>         /* Work around gcc excess stack consumption issue */
>         if (p_size == (size_t)-1 ||
>                 (__builtin_constant_p(p[p_size - 1]) && p[p_size - 1] == '\0'))
>                 return __underlying_strlen(p);
>         ret = strnlen(p, p_size);
>         if (p_size <= ret)
>                 fortify_panic(__func__);
>         return ret;
> }
> 
> So while strlcpy did work before this fortify check, it is probably not the
> best option anymore due to the missing null terminator in the source.
> 
Got it, it really triggers panic in strlen().
So could you please update the commit log? I think CONFIG_FORTIFY_SOURCE
is necessary information since it is not default enabled.
And add comments with your changes, e.g.

/*
 * ci_stack and ci_cluster in ocfs2_cluster_info may not null
 * terminated, make sure no overflow happens here.
 */

BTW, since we use kzalloc to alloc osb, so we don't have to manually
set the last null byte.

Thanks,
Joseph

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2] ocfs2: mount fails with buffer overflow in strlen
  2021-09-29  9:12           ` [Ocfs2-devel] " Joseph Qi
@ 2021-09-29 18:06             ` Valentin Vidic
  -1 siblings, 0 replies; 17+ messages in thread
From: Valentin Vidic @ 2021-09-29 18:06 UTC (permalink / raw)
  To: Joseph Qi
  Cc: Mark Fasheh, Joel Becker, ocfs2-devel, linux-kernel, Valentin Vidic

Starting with kernel 5.11 built with CONFIG_FORTIFY_SOURCE mouting an
ocfs2 filesystem with either o2cb or pcmk cluster stack fails with the
trace below. Problem seems to be that strings for cluster stack and
cluster name are not guaranteed to be null terminated in the disk
representation, while strlcpy assumes that the source string is always
null terminated. This causes a read outside of the source string
triggering the buffer overflow detection.

detected buffer overflow in strlen
------------[ cut here ]------------
kernel BUG at lib/string.c:1149!
invalid opcode: 0000 [#1] SMP PTI
CPU: 1 PID: 910 Comm: mount.ocfs2 Not tainted 5.14.0-1-amd64 #1
  Debian 5.14.6-2
RIP: 0010:fortify_panic+0xf/0x11
...
Call Trace:
 ocfs2_initialize_super.isra.0.cold+0xc/0x18 [ocfs2]
 ocfs2_fill_super+0x359/0x19b0 [ocfs2]
 mount_bdev+0x185/0x1b0
 ? ocfs2_remount+0x440/0x440 [ocfs2]
 legacy_get_tree+0x27/0x40
 vfs_get_tree+0x25/0xb0
 path_mount+0x454/0xa20
 __x64_sys_mount+0x103/0x140
 do_syscall_64+0x3b/0xc0
 entry_SYSCALL_64_after_hwframe+0x44/0xae

Signed-off-by: Valentin Vidic <vvidic@valentin-vidic.from.hr>
---
v2: update description, add comment, drop null termination

 fs/ocfs2/super.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
index c86bd4e60e20..5c914ce9b3ac 100644
--- a/fs/ocfs2/super.c
+++ b/fs/ocfs2/super.c
@@ -2167,11 +2167,17 @@ static int ocfs2_initialize_super(struct super_block *sb,
 	}
 
 	if (ocfs2_clusterinfo_valid(osb)) {
+		/*
+		 * ci_stack and ci_cluster in ocfs2_cluster_info may not be null
+		 * terminated, so make sure no overflow happens here by using
+		 * memcpy. Destination strings will always be null terminated
+		 * because osb is allocated using kzalloc.
+		 */
 		osb->osb_stackflags =
 			OCFS2_RAW_SB(di)->s_cluster_info.ci_stackflags;
-		strlcpy(osb->osb_cluster_stack,
+		memcpy(osb->osb_cluster_stack,
 		       OCFS2_RAW_SB(di)->s_cluster_info.ci_stack,
-		       OCFS2_STACK_LABEL_LEN + 1);
+		       OCFS2_STACK_LABEL_LEN);
 		if (strlen(osb->osb_cluster_stack) != OCFS2_STACK_LABEL_LEN) {
 			mlog(ML_ERROR,
 			     "couldn't mount because of an invalid "
@@ -2180,9 +2186,9 @@ static int ocfs2_initialize_super(struct super_block *sb,
 			status = -EINVAL;
 			goto bail;
 		}
-		strlcpy(osb->osb_cluster_name,
+		memcpy(osb->osb_cluster_name,
 			OCFS2_RAW_SB(di)->s_cluster_info.ci_cluster,
-			OCFS2_CLUSTER_NAME_LEN + 1);
+			OCFS2_CLUSTER_NAME_LEN);
 	} else {
 		/* The empty string is identical with classic tools that
 		 * don't know about s_cluster_info. */
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Ocfs2-devel] [PATCH v2] ocfs2: mount fails with buffer overflow in strlen
@ 2021-09-29 18:06             ` Valentin Vidic
  0 siblings, 0 replies; 17+ messages in thread
From: Valentin Vidic @ 2021-09-29 18:06 UTC (permalink / raw)
  To: Joseph Qi; +Cc: Valentin Vidic, ocfs2-devel, linux-kernel

Starting with kernel 5.11 built with CONFIG_FORTIFY_SOURCE mouting an
ocfs2 filesystem with either o2cb or pcmk cluster stack fails with the
trace below. Problem seems to be that strings for cluster stack and
cluster name are not guaranteed to be null terminated in the disk
representation, while strlcpy assumes that the source string is always
null terminated. This causes a read outside of the source string
triggering the buffer overflow detection.

detected buffer overflow in strlen
------------[ cut here ]------------
kernel BUG at lib/string.c:1149!
invalid opcode: 0000 [#1] SMP PTI
CPU: 1 PID: 910 Comm: mount.ocfs2 Not tainted 5.14.0-1-amd64 #1
  Debian 5.14.6-2
RIP: 0010:fortify_panic+0xf/0x11
...
Call Trace:
 ocfs2_initialize_super.isra.0.cold+0xc/0x18 [ocfs2]
 ocfs2_fill_super+0x359/0x19b0 [ocfs2]
 mount_bdev+0x185/0x1b0
 ? ocfs2_remount+0x440/0x440 [ocfs2]
 legacy_get_tree+0x27/0x40
 vfs_get_tree+0x25/0xb0
 path_mount+0x454/0xa20
 __x64_sys_mount+0x103/0x140
 do_syscall_64+0x3b/0xc0
 entry_SYSCALL_64_after_hwframe+0x44/0xae

Signed-off-by: Valentin Vidic <vvidic@valentin-vidic.from.hr>
---
v2: update description, add comment, drop null termination

 fs/ocfs2/super.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
index c86bd4e60e20..5c914ce9b3ac 100644
--- a/fs/ocfs2/super.c
+++ b/fs/ocfs2/super.c
@@ -2167,11 +2167,17 @@ static int ocfs2_initialize_super(struct super_block *sb,
 	}
 
 	if (ocfs2_clusterinfo_valid(osb)) {
+		/*
+		 * ci_stack and ci_cluster in ocfs2_cluster_info may not be null
+		 * terminated, so make sure no overflow happens here by using
+		 * memcpy. Destination strings will always be null terminated
+		 * because osb is allocated using kzalloc.
+		 */
 		osb->osb_stackflags =
 			OCFS2_RAW_SB(di)->s_cluster_info.ci_stackflags;
-		strlcpy(osb->osb_cluster_stack,
+		memcpy(osb->osb_cluster_stack,
 		       OCFS2_RAW_SB(di)->s_cluster_info.ci_stack,
-		       OCFS2_STACK_LABEL_LEN + 1);
+		       OCFS2_STACK_LABEL_LEN);
 		if (strlen(osb->osb_cluster_stack) != OCFS2_STACK_LABEL_LEN) {
 			mlog(ML_ERROR,
 			     "couldn't mount because of an invalid "
@@ -2180,9 +2186,9 @@ static int ocfs2_initialize_super(struct super_block *sb,
 			status = -EINVAL;
 			goto bail;
 		}
-		strlcpy(osb->osb_cluster_name,
+		memcpy(osb->osb_cluster_name,
 			OCFS2_RAW_SB(di)->s_cluster_info.ci_cluster,
-			OCFS2_CLUSTER_NAME_LEN + 1);
+			OCFS2_CLUSTER_NAME_LEN);
 	} else {
 		/* The empty string is identical with classic tools that
 		 * don't know about s_cluster_info. */
-- 
2.30.2


_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] ocfs2: mount fails with buffer overflow in strlen
  2021-09-29 18:06             ` [Ocfs2-devel] " Valentin Vidic
@ 2021-09-30  1:54               ` Joseph Qi
  -1 siblings, 0 replies; 17+ messages in thread
From: Joseph Qi @ 2021-09-30  1:54 UTC (permalink / raw)
  To: Valentin Vidic, akpm; +Cc: Mark Fasheh, Joel Becker, ocfs2-devel, linux-kernel



On 9/30/21 2:06 AM, Valentin Vidic wrote:
> Starting with kernel 5.11 built with CONFIG_FORTIFY_SOURCE mouting an
> ocfs2 filesystem with either o2cb or pcmk cluster stack fails with the
> trace below. Problem seems to be that strings for cluster stack and
> cluster name are not guaranteed to be null terminated in the disk
> representation, while strlcpy assumes that the source string is always
> null terminated. This causes a read outside of the source string
> triggering the buffer overflow detection.
> 
> detected buffer overflow in strlen
> ------------[ cut here ]------------
> kernel BUG at lib/string.c:1149!
> invalid opcode: 0000 [#1] SMP PTI
> CPU: 1 PID: 910 Comm: mount.ocfs2 Not tainted 5.14.0-1-amd64 #1
>   Debian 5.14.6-2
> RIP: 0010:fortify_panic+0xf/0x11
> ...
> Call Trace:
>  ocfs2_initialize_super.isra.0.cold+0xc/0x18 [ocfs2]
>  ocfs2_fill_super+0x359/0x19b0 [ocfs2]
>  mount_bdev+0x185/0x1b0
>  ? ocfs2_remount+0x440/0x440 [ocfs2]
>  legacy_get_tree+0x27/0x40
>  vfs_get_tree+0x25/0xb0
>  path_mount+0x454/0xa20
>  __x64_sys_mount+0x103/0x140
>  do_syscall_64+0x3b/0xc0
>  entry_SYSCALL_64_after_hwframe+0x44/0xae
> 
> Signed-off-by: Valentin Vidic <vvidic@valentin-vidic.from.hr>

Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com>
> ---
> v2: update description, add comment, drop null termination
> 
>  fs/ocfs2/super.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
> index c86bd4e60e20..5c914ce9b3ac 100644
> --- a/fs/ocfs2/super.c
> +++ b/fs/ocfs2/super.c
> @@ -2167,11 +2167,17 @@ static int ocfs2_initialize_super(struct super_block *sb,
>  	}
>  
>  	if (ocfs2_clusterinfo_valid(osb)) {
> +		/*
> +		 * ci_stack and ci_cluster in ocfs2_cluster_info may not be null
> +		 * terminated, so make sure no overflow happens here by using
> +		 * memcpy. Destination strings will always be null terminated
> +		 * because osb is allocated using kzalloc.
> +		 */
>  		osb->osb_stackflags =
>  			OCFS2_RAW_SB(di)->s_cluster_info.ci_stackflags;
> -		strlcpy(osb->osb_cluster_stack,
> +		memcpy(osb->osb_cluster_stack,
>  		       OCFS2_RAW_SB(di)->s_cluster_info.ci_stack,
> -		       OCFS2_STACK_LABEL_LEN + 1);
> +		       OCFS2_STACK_LABEL_LEN);
>  		if (strlen(osb->osb_cluster_stack) != OCFS2_STACK_LABEL_LEN) {
>  			mlog(ML_ERROR,
>  			     "couldn't mount because of an invalid "
> @@ -2180,9 +2186,9 @@ static int ocfs2_initialize_super(struct super_block *sb,
>  			status = -EINVAL;
>  			goto bail;
>  		}
> -		strlcpy(osb->osb_cluster_name,
> +		memcpy(osb->osb_cluster_name,
>  			OCFS2_RAW_SB(di)->s_cluster_info.ci_cluster,
> -			OCFS2_CLUSTER_NAME_LEN + 1);
> +			OCFS2_CLUSTER_NAME_LEN);
>  	} else {
>  		/* The empty string is identical with classic tools that
>  		 * don't know about s_cluster_info. */
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Ocfs2-devel] [PATCH v2] ocfs2: mount fails with buffer overflow in strlen
@ 2021-09-30  1:54               ` Joseph Qi
  0 siblings, 0 replies; 17+ messages in thread
From: Joseph Qi @ 2021-09-30  1:54 UTC (permalink / raw)
  To: Valentin Vidic, akpm; +Cc: ocfs2-devel, linux-kernel



On 9/30/21 2:06 AM, Valentin Vidic wrote:
> Starting with kernel 5.11 built with CONFIG_FORTIFY_SOURCE mouting an
> ocfs2 filesystem with either o2cb or pcmk cluster stack fails with the
> trace below. Problem seems to be that strings for cluster stack and
> cluster name are not guaranteed to be null terminated in the disk
> representation, while strlcpy assumes that the source string is always
> null terminated. This causes a read outside of the source string
> triggering the buffer overflow detection.
> 
> detected buffer overflow in strlen
> ------------[ cut here ]------------
> kernel BUG at lib/string.c:1149!
> invalid opcode: 0000 [#1] SMP PTI
> CPU: 1 PID: 910 Comm: mount.ocfs2 Not tainted 5.14.0-1-amd64 #1
>   Debian 5.14.6-2
> RIP: 0010:fortify_panic+0xf/0x11
> ...
> Call Trace:
>  ocfs2_initialize_super.isra.0.cold+0xc/0x18 [ocfs2]
>  ocfs2_fill_super+0x359/0x19b0 [ocfs2]
>  mount_bdev+0x185/0x1b0
>  ? ocfs2_remount+0x440/0x440 [ocfs2]
>  legacy_get_tree+0x27/0x40
>  vfs_get_tree+0x25/0xb0
>  path_mount+0x454/0xa20
>  __x64_sys_mount+0x103/0x140
>  do_syscall_64+0x3b/0xc0
>  entry_SYSCALL_64_after_hwframe+0x44/0xae
> 
> Signed-off-by: Valentin Vidic <vvidic@valentin-vidic.from.hr>

Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com>
> ---
> v2: update description, add comment, drop null termination
> 
>  fs/ocfs2/super.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
> index c86bd4e60e20..5c914ce9b3ac 100644
> --- a/fs/ocfs2/super.c
> +++ b/fs/ocfs2/super.c
> @@ -2167,11 +2167,17 @@ static int ocfs2_initialize_super(struct super_block *sb,
>  	}
>  
>  	if (ocfs2_clusterinfo_valid(osb)) {
> +		/*
> +		 * ci_stack and ci_cluster in ocfs2_cluster_info may not be null
> +		 * terminated, so make sure no overflow happens here by using
> +		 * memcpy. Destination strings will always be null terminated
> +		 * because osb is allocated using kzalloc.
> +		 */
>  		osb->osb_stackflags =
>  			OCFS2_RAW_SB(di)->s_cluster_info.ci_stackflags;
> -		strlcpy(osb->osb_cluster_stack,
> +		memcpy(osb->osb_cluster_stack,
>  		       OCFS2_RAW_SB(di)->s_cluster_info.ci_stack,
> -		       OCFS2_STACK_LABEL_LEN + 1);
> +		       OCFS2_STACK_LABEL_LEN);
>  		if (strlen(osb->osb_cluster_stack) != OCFS2_STACK_LABEL_LEN) {
>  			mlog(ML_ERROR,
>  			     "couldn't mount because of an invalid "
> @@ -2180,9 +2186,9 @@ static int ocfs2_initialize_super(struct super_block *sb,
>  			status = -EINVAL;
>  			goto bail;
>  		}
> -		strlcpy(osb->osb_cluster_name,
> +		memcpy(osb->osb_cluster_name,
>  			OCFS2_RAW_SB(di)->s_cluster_info.ci_cluster,
> -			OCFS2_CLUSTER_NAME_LEN + 1);
> +			OCFS2_CLUSTER_NAME_LEN);
>  	} else {
>  		/* The empty string is identical with classic tools that
>  		 * don't know about s_cluster_info. */
> 

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Ocfs2-devel] [PATCH v2] ocfs2: mount fails with buffer overflow in strlen
  2021-09-30  1:54               ` [Ocfs2-devel] " Joseph Qi
  (?)
@ 2021-10-08 10:46               ` Gang He
  -1 siblings, 0 replies; 17+ messages in thread
From: Gang He @ 2021-10-08 10:46 UTC (permalink / raw)
  To: ocfs2-devel

Hi Andrew,

This panic looks urgent, which is blocking our CI testing for new SLE 
releases.
If possible, please help to merge it to Linus git tree.

Thanks
Gang

On 2021/9/30 9:54, Joseph Qi wrote:
> 
> 
> On 9/30/21 2:06 AM, Valentin Vidic wrote:
>> Starting with kernel 5.11 built with CONFIG_FORTIFY_SOURCE mouting an
>> ocfs2 filesystem with either o2cb or pcmk cluster stack fails with the
>> trace below. Problem seems to be that strings for cluster stack and
>> cluster name are not guaranteed to be null terminated in the disk
>> representation, while strlcpy assumes that the source string is always
>> null terminated. This causes a read outside of the source string
>> triggering the buffer overflow detection.
>>
>> detected buffer overflow in strlen
>> ------------[ cut here ]------------
>> kernel BUG at lib/string.c:1149!
>> invalid opcode: 0000 [#1] SMP PTI
>> CPU: 1 PID: 910 Comm: mount.ocfs2 Not tainted 5.14.0-1-amd64 #1
>>    Debian 5.14.6-2
>> RIP: 0010:fortify_panic+0xf/0x11
>> ...
>> Call Trace:
>>   ocfs2_initialize_super.isra.0.cold+0xc/0x18 [ocfs2]
>>   ocfs2_fill_super+0x359/0x19b0 [ocfs2]
>>   mount_bdev+0x185/0x1b0
>>   ? ocfs2_remount+0x440/0x440 [ocfs2]
>>   legacy_get_tree+0x27/0x40
>>   vfs_get_tree+0x25/0xb0
>>   path_mount+0x454/0xa20
>>   __x64_sys_mount+0x103/0x140
>>   do_syscall_64+0x3b/0xc0
>>   entry_SYSCALL_64_after_hwframe+0x44/0xae
>>
>> Signed-off-by: Valentin Vidic <vvidic@valentin-vidic.from.hr>
> 
> Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com>
>> ---
>> v2: update description, add comment, drop null termination
>>
>>   fs/ocfs2/super.c | 14 ++++++++++----
>>   1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
>> index c86bd4e60e20..5c914ce9b3ac 100644
>> --- a/fs/ocfs2/super.c
>> +++ b/fs/ocfs2/super.c
>> @@ -2167,11 +2167,17 @@ static int ocfs2_initialize_super(struct super_block *sb,
>>   	}
>>   
>>   	if (ocfs2_clusterinfo_valid(osb)) {
>> +		/*
>> +		 * ci_stack and ci_cluster in ocfs2_cluster_info may not be null
>> +		 * terminated, so make sure no overflow happens here by using
>> +		 * memcpy. Destination strings will always be null terminated
>> +		 * because osb is allocated using kzalloc.
>> +		 */
>>   		osb->osb_stackflags =
>>   			OCFS2_RAW_SB(di)->s_cluster_info.ci_stackflags;
>> -		strlcpy(osb->osb_cluster_stack,
>> +		memcpy(osb->osb_cluster_stack,
>>   		       OCFS2_RAW_SB(di)->s_cluster_info.ci_stack,
>> -		       OCFS2_STACK_LABEL_LEN + 1);
>> +		       OCFS2_STACK_LABEL_LEN);
>>   		if (strlen(osb->osb_cluster_stack) != OCFS2_STACK_LABEL_LEN) {
>>   			mlog(ML_ERROR,
>>   			     "couldn't mount because of an invalid "
>> @@ -2180,9 +2186,9 @@ static int ocfs2_initialize_super(struct super_block *sb,
>>   			status = -EINVAL;
>>   			goto bail;
>>   		}
>> -		strlcpy(osb->osb_cluster_name,
>> +		memcpy(osb->osb_cluster_name,
>>   			OCFS2_RAW_SB(di)->s_cluster_info.ci_cluster,
>> -			OCFS2_CLUSTER_NAME_LEN + 1);
>> +			OCFS2_CLUSTER_NAME_LEN);
>>   	} else {
>>   		/* The empty string is identical with classic tools that
>>   		 * don't know about s_cluster_info. */
>>
> 
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
> 


_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2021-10-08 10:50 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-27 15:44 [PATCH] ocfs2: mount fails with buffer overflow in strlen Valentin Vidic
2021-09-27 15:44 ` [Ocfs2-devel] " Valentin Vidic
2021-09-28 12:05 ` Joseph Qi
2021-09-28 12:05   ` [Ocfs2-devel] " Joseph Qi
2021-09-28 13:14   ` Valentin Vidić
2021-09-28 13:14     ` [Ocfs2-devel] " Valentin Vidić
2021-09-29  2:38     ` Joseph Qi
2021-09-29  2:38       ` [Ocfs2-devel] " Joseph Qi
2021-09-29  6:24       ` Valentin Vidić
2021-09-29  6:24         ` [Ocfs2-devel] " Valentin Vidić
2021-09-29  9:12         ` Joseph Qi
2021-09-29  9:12           ` [Ocfs2-devel] " Joseph Qi
2021-09-29 18:06           ` [PATCH v2] " Valentin Vidic
2021-09-29 18:06             ` [Ocfs2-devel] " Valentin Vidic
2021-09-30  1:54             ` Joseph Qi
2021-09-30  1:54               ` [Ocfs2-devel] " Joseph Qi
2021-10-08 10:46               ` Gang He

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.