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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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. */
>
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
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*/
> };
>
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
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
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 <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Joseph Qi <[email protected]>
> ---
> 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. */
>