LinuxLists.cc - [PATCH -next 0/5] fs: fix possible null-ptr-deref when parsing param

2022-10-23 16:43:10

by Hawkins Jiawei

[permalink] [raw]

Subject: [PATCH -next 0/5] fs: fix possible null-ptr-deref when parsing param

2022-10-23 17:00:26

by Hawkins Jiawei

[permalink] [raw]

Subject: [PATCH -next 4/5] gfs2: fix possible null-ptr-deref when parsing param

2022-10-23 17:03:50

by Hawkins Jiawei

[permalink] [raw]

Subject: [PATCH -next 1/5] smb3: fix possible null-ptr-deref when parsing param

According to commit "vfs: parse: deal with zero length string value",
kernel will set the param->string to null pointer in vfs_parse_fs_string()
if fs string has zero length.

Yet the problem is that, smb3_fs_context_parse_param() will dereferences
the param->string, without checking whether it is a null pointer, which
may trigger a null-ptr-deref bug.

This patch solves it by adding sanity check on param->string
in smb3_fs_context_parse_param().

Signed-off-by: Hawkins Jiawei <[email protected]>
---
fs/cifs/fs_context.c | 58 +++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 57 insertions(+), 1 deletion(-)

diff --git a/fs/cifs/fs_context.c b/fs/cifs/fs_context.c
index 45119597c765..7832e5d6bbb0 100644
--- a/fs/cifs/fs_context.c
+++ b/fs/cifs/fs_context.c
@@ -858,7 +858,8 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
* fs_parse can not handle string options with an empty value so
* we will need special handling of them.
*/
- if (param->type == fs_value_is_string && param->string[0] == 0) {
+ if ((param->type == fs_value_is_string && param->string[0] == 0) ||
+ param->type == fs_value_is_empty) {
if (!strcmp("pass", param->key) || !strcmp("password", param->key)) {
skip_parsing = true;
opt = Opt_pass;
@@ -1124,6 +1125,11 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
case Opt_source:
kfree(ctx->UNC);
ctx->UNC = NULL;
+ if (!param->string) {
+ cifs_errorf(fc, "Bad value '(null)' for mount option '%s'\n",
+ param->key);
+ goto cifs_parse_mount_err;
+ }
switch (smb3_parse_devname(param->string, ctx)) {
case 0:
break;
@@ -1181,6 +1187,11 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
}
break;
case Opt_ip:
+ if (!param->string) {
+ cifs_errorf(fc, "Bad value '(null)' for mount option '%s'\n",
+ param->key);
+ goto cifs_parse_mount_err;
+ }
if (strlen(param->string) == 0) {
ctx->got_ip = false;
break;
@@ -1194,6 +1205,11 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
ctx->got_ip = true;
break;
case Opt_domain:
+ if (!param->string) {
+ cifs_errorf(fc, "Bad value '(null)' for mount option '%s'\n",
+ param->key);
+ goto cifs_parse_mount_err;
+ }
if (strnlen(param->string, CIFS_MAX_DOMAINNAME_LEN)
== CIFS_MAX_DOMAINNAME_LEN) {
pr_warn("domain name too long\n");
@@ -1209,6 +1225,11 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
cifs_dbg(FYI, "Domain name set\n");
break;
case Opt_srcaddr:
+ if (!param->string) {
+ cifs_errorf(fc, "Bad value '(null)' for mount option '%s'\n",
+ param->key);
+ goto cifs_parse_mount_err;
+ }
if (!cifs_convert_address(
(struct sockaddr *)&ctx->srcaddr,
param->string, strlen(param->string))) {
@@ -1218,6 +1239,11 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
}
break;
case Opt_iocharset:
+ if (!param->string) {
+ cifs_errorf(fc, "Bad value '(null)' for mount option '%s'\n",
+ param->key);
+ goto cifs_parse_mount_err;
+ }
if (strnlen(param->string, 1024) >= 65) {
pr_warn("iocharset name too long\n");
goto cifs_parse_mount_err;
@@ -1237,6 +1263,11 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
cifs_dbg(FYI, "iocharset set to %s\n", ctx->iocharset);
break;
case Opt_netbiosname:
+ if (!param->string) {
+ cifs_errorf(fc, "Bad value '(null)' for mount option '%s'\n",
+ param->key);
+ goto cifs_parse_mount_err;
+ }
memset(ctx->source_rfc1001_name, 0x20,
RFC1001_NAME_LEN);
/*
@@ -1257,6 +1288,11 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
pr_warn("netbiosname longer than 15 truncated\n");
break;
case Opt_servern:
+ if (!param->string) {
+ cifs_errorf(fc, "Bad value '(null)' for mount option '%s'\n",
+ param->key);
+ goto cifs_parse_mount_err;
+ }
/* last byte, type, is 0x20 for servr type */
memset(ctx->target_rfc1001_name, 0x20,
RFC1001_NAME_LEN_WITH_NULL);
@@ -1277,6 +1313,11 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
pr_warn("server netbiosname longer than 15 truncated\n");
break;
case Opt_ver:
+ if (!param->string) {
+ cifs_errorf(fc, "Bad value '(null)' for mount option '%s'\n",
+ param->key);
+ goto cifs_parse_mount_err;
+ }
/* version of mount userspace tools, not dialect */
/* If interface changes in mount.cifs bump to new ver */
if (strncasecmp(param->string, "1", 1) == 0) {
@@ -1292,16 +1333,31 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
pr_warn("Invalid mount helper version specified\n");
goto cifs_parse_mount_err;
case Opt_vers:
+ if (!param->string) {
+ cifs_errorf(fc, "Bad value '(null)' for mount option '%s'\n",
+ param->key);
+ goto cifs_parse_mount_err;
+ }
/* protocol version (dialect) */
if (cifs_parse_smb_version(fc, param->string, ctx, is_smb3) != 0)
goto cifs_parse_mount_err;
ctx->got_version = true;
break;
case Opt_sec:
+ if (!param->string) {
+ cifs_errorf(fc, "Bad value '(null)' for mount option '%s'\n",
+ param->key);
+ goto cifs_parse_mount_err;
+ }
if (cifs_parse_security_flavors(fc, param->string, ctx) != 0)
goto cifs_parse_mount_err;
break;
case Opt_cache:
+ if (!param->string) {
+ cifs_errorf(fc, "Bad value '(null)' for mount option '%s'\n",
+ param->key);
+ goto cifs_parse_mount_err;
+ }
if (cifs_parse_cache_flavor(fc, param->string, ctx) != 0)
goto cifs_parse_mount_err;
break;
--
2.25.1

2022-10-23 17:07:26

by Al Viro

[permalink] [raw]

Subject: Re: [PATCH -next 0/5] fs: fix possible null-ptr-deref when parsing param

On Mon, Oct 24, 2022 at 12:39:41AM +0800, Hawkins Jiawei wrote:
> According to commit "vfs: parse: deal with zero length string value",
> kernel will set the param->string to null pointer in vfs_parse_fs_string()
> if fs string has zero length.
>
> Yet the problem is that, when fs parses its mount parameters, it will
> dereferences the param->string, without checking whether it is a
> null pointer, which may trigger a null-ptr-deref bug.
>
> So this patchset reviews all functions for fs to parse parameters,
> by using `git grep -n "\.parse_param" fs/*`, and adds sanity check
> on param->string if its function will dereference param->string
> without check.

How about reverting the commit in question instead? Or dropping it
from patch series, depending upon the way akpm handles the pile
these days...

2022-10-23 17:08:30

by Hawkins Jiawei

[permalink] [raw]

Subject: [PATCH -next 5/5] proc: fix possible null-ptr-deref when parsing param

2022-10-23 17:09:50

by Hawkins Jiawei

[permalink] [raw]

Subject: [PATCH -next 3/5] ceph: fix possible null-ptr-deref when parsing param

2022-10-24 00:47:29

by Hawkins Jiawei

[permalink] [raw]

Subject: Re: [PATCH -next 0/5] fs: fix possible null-ptr-deref when parsing param

On Mon, 24 Oct 2022 at 00:48, Al Viro <[email protected]> wrote:
>
> On Mon, Oct 24, 2022 at 12:39:41AM +0800, Hawkins Jiawei wrote:
> > According to commit "vfs: parse: deal with zero length string value",
> > kernel will set the param->string to null pointer in vfs_parse_fs_string()
> > if fs string has zero length.
> >
> > Yet the problem is that, when fs parses its mount parameters, it will
> > dereferences the param->string, without checking whether it is a
> > null pointer, which may trigger a null-ptr-deref bug.
> >
> > So this patchset reviews all functions for fs to parse parameters,
> > by using `git grep -n "\.parse_param" fs/*`, and adds sanity check
> > on param->string if its function will dereference param->string
> > without check.
>
> How about reverting the commit in question instead? Or dropping it
> from patch series, depending upon the way akpm handles the pile
> these days...

I think both are OK.

On one hand, commit "vfs: parse: deal with zero length string value"
seems just want to make output more informattive, which probably is not
the one which must be applied immediately to fix the
panic.

On the other hand, commit "vfs: parse: deal with zero length string value"
affects so many file systems, so there are probably some deeper
null-ptr-deref bugs I ignore, which may take time to review.

2022-10-24 01:06:08

by Xiubo Li

[permalink] [raw]

Subject: Re: [PATCH -next 3/5] ceph: fix possible null-ptr-deref when parsing param

On 24/10/2022 00:39, Hawkins Jiawei wrote:
> According to commit "vfs: parse: deal with zero length string value",
> kernel will set the param->string to null pointer in vfs_parse_fs_string()
> if fs string has zero length.
>
> Yet the problem is that, ceph_parse_mount_param() will dereferences the
> param->string, without checking whether it is a null pointer, which may
> trigger a null-ptr-deref bug.
>
> This patch solves it by adding sanity check on param->string
> in ceph_parse_mount_param().
>
> Signed-off-by: Hawkins Jiawei <[email protected]>
> ---
> fs/ceph/super.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index 3fc48b43cab0..341e23fe29eb 100644
> --- a/fs/ceph/super.c
> +++ b/fs/ceph/super.c
> @@ -417,6 +417,9 @@ static int ceph_parse_mount_param(struct fs_context *fc,
> param->string = NULL;
> break;
> case Opt_mds_namespace:
> + if (!param->string)
> + return invalfc(fc, "Bad value '%s' for mount option '%s'\n",
> + param->string, param->key);
> if (!namespace_equals(fsopt, param->string, strlen(param->string)))
> return invalfc(fc, "Mismatching mds_namespace");
> kfree(fsopt->mds_namespace);

BTW, did you hit any crash issue when testing this ?

$ ./bin/mount.ceph :/ /mnt/kcephfs -o mds_namespace=

<5>[ 375.535442] ceph: module verification failed: signature and/or
required key missing - tainting kernel
<6>[ 375.698145] ceph: loaded (mds proto 32)
<3>[ 375.801621] ceph: Bad value for 'mds_namespace'

From my test, the 'fsparam_string()' has already make sure it won't
trigger the null-ptr-deref bug.

But it will always make sense to fix it in ceph code with your patch.

- Xiubo

2022-10-24 01:35:19

by Xiubo Li

[permalink] [raw]

Subject: Re: [PATCH -next 3/5] ceph: fix possible null-ptr-deref when parsing param

On 24/10/2022 00:39, Hawkins Jiawei wrote:
> According to commit "vfs: parse: deal with zero length string value",
> kernel will set the param->string to null pointer in vfs_parse_fs_string()
> if fs string has zero length.
>
> Yet the problem is that, ceph_parse_mount_param() will dereferences the
> param->string, without checking whether it is a null pointer, which may
> trigger a null-ptr-deref bug.
>
> This patch solves it by adding sanity check on param->string
> in ceph_parse_mount_param().
>
> Signed-off-by: Hawkins Jiawei <[email protected]>
> ---
> fs/ceph/super.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index 3fc48b43cab0..341e23fe29eb 100644
> --- a/fs/ceph/super.c
> +++ b/fs/ceph/super.c
> @@ -417,6 +417,9 @@ static int ceph_parse_mount_param(struct fs_context *fc,
> param->string = NULL;
> break;
> case Opt_mds_namespace:
> + if (!param->string)
> + return invalfc(fc, "Bad value '%s' for mount option '%s'\n",
> + param->string, param->key);
> if (!namespace_equals(fsopt, param->string, strlen(param->string)))
> return invalfc(fc, "Mismatching mds_namespace");
> kfree(fsopt->mds_namespace);

Good catch!

Will merge it to testing branch.

Thanks!

- Xiubo

2022-10-24 02:30:15

by Hawkins Jiawei

[permalink] [raw]

Subject: Re: [PATCH -next 3/5] ceph: fix possible null-ptr-deref when parsing param

Hi Xiubo,
On Mon, 24 Oct 2022 at 08:55, Xiubo Li <[email protected]> wrote:
>
>
> On 24/10/2022 00:39, Hawkins Jiawei wrote:
> > According to commit "vfs: parse: deal with zero length string value",
> > kernel will set the param->string to null pointer in vfs_parse_fs_string()
> > if fs string has zero length.
> >
> > Yet the problem is that, ceph_parse_mount_param() will dereferences the
> > param->string, without checking whether it is a null pointer, which may
> > trigger a null-ptr-deref bug.
> >
> > This patch solves it by adding sanity check on param->string
> > in ceph_parse_mount_param().
> >
> > Signed-off-by: Hawkins Jiawei <[email protected]>
> > ---
> > fs/ceph/super.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> > index 3fc48b43cab0..341e23fe29eb 100644
> > --- a/fs/ceph/super.c
> > +++ b/fs/ceph/super.c
> > @@ -417,6 +417,9 @@ static int ceph_parse_mount_param(struct fs_context *fc,
> > param->string = NULL;
> > break;
> > case Opt_mds_namespace:
> > + if (!param->string)
> > + return invalfc(fc, "Bad value '%s' for mount option '%s'\n",
> > + param->string, param->key);
> > if (!namespace_equals(fsopt, param->string, strlen(param->string)))
> > return invalfc(fc, "Mismatching mds_namespace");
> > kfree(fsopt->mds_namespace);
>
> BTW, did you hit any crash issue when testing this ?
>
> $ ./bin/mount.ceph :/ /mnt/kcephfs -o mds_namespace=
>
> <5>[ 375.535442] ceph: module verification failed: signature and/or
> required key missing - tainting kernel
> <6>[ 375.698145] ceph: loaded (mds proto 32)
> <3>[ 375.801621] ceph: Bad value for 'mds_namespace'
>
> From my test, the 'fsparam_string()' has already make sure it won't
> trigger the null-ptr-deref bug.
Did you test on linux-next tree?

I just write a reproducer based on syzkaller's template(So please
forgive me if it is too ugly to read)

===========================================================
// https://syzkaller.appspot.com/bug?id=76bbdfd28722f0160325e4350b57e33aa95b0bbe
// autogenerated by syzkaller (https://github.com/google/syzkaller)

#define _GNU_SOURCE

#include <dirent.h>
#include <endian.h>
#include <errno.h>
#include <fcntl.h>
#include <signal.h>
#include <stdarg.h>
#include <stdbool.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/prctl.h>
#include <sys/stat.h>
#include <sys/syscall.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <time.h>
#include <unistd.h>

unsigned long long procid;

static void sleep_ms(uint64_t ms)
{
usleep(ms * 1000);
}

static uint64_t current_time_ms(void)
{
struct timespec ts;
if (clock_gettime(CLOCK_MONOTONIC, &ts))
exit(1);
return (uint64_t)ts.tv_sec * 1000 + (uint64_t)ts.tv_nsec / 1000000;
}

static bool write_file(const char* file, const char* what, ...)
{
char buf[1024];
va_list args;
va_start(args, what);
vsnprintf(buf, sizeof(buf), what, args);
va_end(args);
buf[sizeof(buf) - 1] = 0;
int len = strlen(buf);
int fd = open(file, O_WRONLY | O_CLOEXEC);
if (fd == -1)
return false;
if (write(fd, buf, len) != len) {
int err = errno;
close(fd);
errno = err;
return false;
}
close(fd);
return true;
}

static void kill_and_wait(int pid, int* status)
{
kill(-pid, SIGKILL);
kill(pid, SIGKILL);
int i;
for (i = 0; i < 100; i++) {
if (waitpid(-1, status, WNOHANG | __WALL) == pid)
return;
usleep(1000);
}
DIR* dir = opendir("/sys/fs/fuse/connections");
if (dir) {
for (;;) {
struct dirent* ent = readdir(dir);
if (!ent)
break;
if (strcmp(ent->d_name, ".") == 0 || strcmp(ent->d_name, "..") == 0)
continue;
char abort[300];
snprintf(abort, sizeof(abort), "/sys/fs/fuse/connections/%s/abort",
ent->d_name);
int fd = open(abort, O_WRONLY);
if (fd == -1) {
continue;
}
if (write(fd, abort, 1) < 0) {
}
close(fd);
}
closedir(dir);
} else {
}
while (waitpid(-1, status, __WALL) != pid) {
}
}

static void setup_test()
{
prctl(PR_SET_PDEATHSIG, SIGKILL, 0, 0, 0);
setpgrp();
write_file("/proc/self/oom_score_adj", "1000");
}

static void execute_one(void);

#define WAIT_FLAGS __WALL

static void loop(void)
{
int iter;
for (iter = 0;; iter++) {
int pid = fork();
if (pid < 0)
exit(1);
if (pid == 0) {
setup_test();
execute_one();
exit(0);
}
int status = 0;
uint64_t start = current_time_ms();
for (;;) {
if (waitpid(-1, &status, WNOHANG | WAIT_FLAGS) == pid)
break;
sleep_ms(1);
if (current_time_ms() - start < 5 * 1000)
continue;
kill_and_wait(pid, &status);
break;
}
}
}

void execute_one(void)
{
char opt[] = "mds_namespace=,\x00";
memcpy((void*)0x20000080, "./file0\000", 8);
syscall(__NR_mknod, 0x20000080ul, 0ul, 0x700ul + procid * 2);
memcpy((void*)0x20000040, "[d::]:/8:", 9);
memcpy((void*)0x200000c0, "./file0\000", 8);
memcpy((void*)0x20000140, "ceph\000", 5);
memcpy((void*)0x20000150, opt, sizeof(opt));
syscall(__NR_mount, 0x20000040ul, 0x200000c0ul, 0x20000140ul, 0ul, 0x20000150);
}
int main(void)
{
syscall(__NR_mmap, 0x20000000ul, 0x1000000ul, 3ul, 0x32ul, -1, 0);
for (procid = 0; procid < 6; procid++) {
if (fork() == 0) {
loop();
}
}
sleep(1000000);
return 0;
}
===========================================================

And it triggers the null-ptr-deref bug described above,
its log is shown as below:
===========================================================
[ 90.779695][ T6513] KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
[ 90.782502][ T6513] RIP: 0010:strlen+0x1a/0x90
[ ... ]
[ 90.782502][ T6513] Call Trace:
[ 90.782502][ T6513] <TASK>
[ 90.782502][ T6513] ceph_parse_mount_param+0x89a/0x21e0
[ 90.782502][ T6513] ? __kasan_unpoison_range-0xf/0x10
[ 90.782502][ T6513] ? kasan_addr_to_slab-0xf/0x90
[ 90.782502][ T6513] ? __sanitizer_cov_trace_pc+0x1a/0x40
[ 90.782502][ T6513] ? ceph_parse_mount_param+0x0/0x21e0
[ 90.782502][ T6513] ? audit_kill_trees+0x2b0/0x300
[ 90.782502][ T6513] ? lock_release+0x0/0x760
[ 90.782502][ T6513] ? __sanitizer_cov_trace_pc+0x1a/0x40
[ 90.782502][ T6513] ? security_fs_context_parse_param+0x99/0xd0
[ 90.782502][ T6513] ? ceph_parse_mount_param+0x0/0x21e0
[ 90.782502][ T6513] vfs_parse_fs_param+0x20f/0x3d0
[ 90.782502][ T6513] vfs_parse_fs_string+0xe4/0x180
[ 90.782502][ T6513] ? vfs_parse_fs_string+0x0/0x180
[ 90.782502][ T6513] ? rcu_read_lock_sched_held+0x0/0xd0
[ 90.782502][ T6513] ? __sanitizer_cov_trace_pc+0x1a/0x40
[ 90.782502][ T6513] ? kfree+0x129/0x1a0
[ 90.782502][ T6513] ? __sanitizer_cov_trace_pc+0x1a/0x40
[ 90.782502][ T6513] ? __sanitizer_cov_trace_pc+0x1a/0x40
[ 90.782502][ T6513] generic_parse_monolithic+0x16f/0x1f0
[ 90.782502][ T6513] ? generic_parse_monolithic+0x0/0x1f0
[ 90.782502][ T6513] ? __sanitizer_cov_trace_pc+0x1a/0x40
[ 90.782502][ T6513] ? alloc_fs_context+0x5cb/0xa00
[ 90.782502][ T6513] path_mount+0x11d3/0x1cb0
[ 90.782502][ T6513] ? path_mount+0x0/0x1cb0
[ 90.782502][ T6513] ? putname+0xfe/0x140
[ 90.782502][ T6513] do_mount+0xf3/0x110
[ 90.782502][ T6513] ? do_mount+0x0/0x110
[ 90.782502][ T6513] ? _copy_from_user+0xf7/0x170
[ 90.782502][ T6513] ? __sanitizer_cov_trace_pc+0x1a/0x40
[ 90.782502][ T6513] __x64_sys_mount+0x18f/0x230
[ 90.782502][ T6513] do_syscall_64+0x35/0xb0
[ 90.782502][ T6513] entry_SYSCALL_64_after_hwframe+0x63/0xcd
[ ... ]
[ 90.782502][ T6513] </TASK>
===========================================================

By the way, commit "vfs: parse: deal with zero length string value"
is still in discussion as below, so maybe this patchset is not
needed.
https://lore.kernel.org/all/[email protected]/
>
> But it will always make sense to fix it in ceph code with your patch.
>
> - Xiubo
>
>
>

2022-10-24 03:07:37

by Xiubo Li

[permalink] [raw]

Subject: Re: [PATCH -next 3/5] ceph: fix possible null-ptr-deref when parsing param

On 24/10/2022 10:04, Hawkins Jiawei wrote:
> Hi Xiubo,
> On Mon, 24 Oct 2022 at 08:55, Xiubo Li <[email protected]> wrote:
>>
>> On 24/10/2022 00:39, Hawkins Jiawei wrote:
>>> According to commit "vfs: parse: deal with zero length string value",
>>> kernel will set the param->string to null pointer in vfs_parse_fs_string()
>>> if fs string has zero length.
>>>
>>> Yet the problem is that, ceph_parse_mount_param() will dereferences the
>>> param->string, without checking whether it is a null pointer, which may
>>> trigger a null-ptr-deref bug.
>>>
>>> This patch solves it by adding sanity check on param->string
>>> in ceph_parse_mount_param().
>>>
>>> Signed-off-by: Hawkins Jiawei <[email protected]>
>>> ---
>>> fs/ceph/super.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
>>> index 3fc48b43cab0..341e23fe29eb 100644
>>> --- a/fs/ceph/super.c
>>> +++ b/fs/ceph/super.c
>>> @@ -417,6 +417,9 @@ static int ceph_parse_mount_param(struct fs_context *fc,
>>> param->string = NULL;
>>> break;
>>> case Opt_mds_namespace:
>>> + if (!param->string)
>>> + return invalfc(fc, "Bad value '%s' for mount option '%s'\n",
>>> + param->string, param->key);
>>> if (!namespace_equals(fsopt, param->string, strlen(param->string)))
>>> return invalfc(fc, "Mismatching mds_namespace");
>>> kfree(fsopt->mds_namespace);
>> BTW, did you hit any crash issue when testing this ?
>>
>> $ ./bin/mount.ceph :/ /mnt/kcephfs -o mds_namespace=
>>
>> <5>[ 375.535442] ceph: module verification failed: signature and/or
>> required key missing - tainting kernel
>> <6>[ 375.698145] ceph: loaded (mds proto 32)
>> <3>[ 375.801621] ceph: Bad value for 'mds_namespace'
>>
>> From my test, the 'fsparam_string()' has already make sure it won't
>> trigger the null-ptr-deref bug.
> Did you test on linux-next tree?

No, I am using the ceph-client repo for ceph code developing.

>
> I just write a reproducer based on syzkaller's template(So please
> forgive me if it is too ugly to read)
>
> ===========================================================
> // https://syzkaller.appspot.com/bug?id=76bbdfd28722f0160325e4350b57e33aa95b0bbe
> // autogenerated by syzkaller (https://github.com/google/syzkaller)
>
> #define _GNU_SOURCE
>
> #include <dirent.h>
> #include <endian.h>
> #include <errno.h>
> #include <fcntl.h>
> #include <signal.h>
> #include <stdarg.h>
> #include <stdbool.h>
> #include <stdint.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <sys/prctl.h>
> #include <sys/stat.h>
> #include <sys/syscall.h>
> #include <sys/types.h>
> #include <sys/wait.h>
> #include <time.h>
> #include <unistd.h>
>
> unsigned long long procid;
>
> static void sleep_ms(uint64_t ms)
> {
> usleep(ms * 1000);
> }
>
> static uint64_t current_time_ms(void)
> {
> struct timespec ts;
> if (clock_gettime(CLOCK_MONOTONIC, &ts))
> exit(1);
> return (uint64_t)ts.tv_sec * 1000 + (uint64_t)ts.tv_nsec / 1000000;
> }
>
> static bool write_file(const char* file, const char* what, ...)
> {
> char buf[1024];
> va_list args;
> va_start(args, what);
> vsnprintf(buf, sizeof(buf), what, args);
> va_end(args);
> buf[sizeof(buf) - 1] = 0;
> int len = strlen(buf);
> int fd = open(file, O_WRONLY | O_CLOEXEC);
> if (fd == -1)
> return false;
> if (write(fd, buf, len) != len) {
> int err = errno;
> close(fd);
> errno = err;
> return false;
> }
> close(fd);
> return true;
> }
>
> static void kill_and_wait(int pid, int* status)
> {
> kill(-pid, SIGKILL);
> kill(pid, SIGKILL);
> int i;
> for (i = 0; i < 100; i++) {
> if (waitpid(-1, status, WNOHANG | __WALL) == pid)
> return;
> usleep(1000);
> }
> DIR* dir = opendir("/sys/fs/fuse/connections");
> if (dir) {
> for (;;) {
> struct dirent* ent = readdir(dir);
> if (!ent)
> break;
> if (strcmp(ent->d_name, ".") == 0 || strcmp(ent->d_name, "..") == 0)
> continue;
> char abort[300];
> snprintf(abort, sizeof(abort), "/sys/fs/fuse/connections/%s/abort",
> ent->d_name);
> int fd = open(abort, O_WRONLY);
> if (fd == -1) {
> continue;
> }
> if (write(fd, abort, 1) < 0) {
> }
> close(fd);
> }
> closedir(dir);
> } else {
> }
> while (waitpid(-1, status, __WALL) != pid) {
> }
> }
>
> static void setup_test()
> {
> prctl(PR_SET_PDEATHSIG, SIGKILL, 0, 0, 0);
> setpgrp();
> write_file("/proc/self/oom_score_adj", "1000");
> }
>
> static void execute_one(void);
>
> #define WAIT_FLAGS __WALL
>
> static void loop(void)
> {
> int iter;
> for (iter = 0;; iter++) {
> int pid = fork();
> if (pid < 0)
> exit(1);
> if (pid == 0) {
> setup_test();
> execute_one();
> exit(0);
> }
> int status = 0;
> uint64_t start = current_time_ms();
> for (;;) {
> if (waitpid(-1, &status, WNOHANG | WAIT_FLAGS) == pid)
> break;
> sleep_ms(1);
> if (current_time_ms() - start < 5 * 1000)
> continue;
> kill_and_wait(pid, &status);
> break;
> }
> }
> }
>
> void execute_one(void)
> {
> char opt[] = "mds_namespace=,\x00";
> memcpy((void*)0x20000080, "./file0\000", 8);
> syscall(__NR_mknod, 0x20000080ul, 0ul, 0x700ul + procid * 2);
> memcpy((void*)0x20000040, "[d::]:/8:", 9);
> memcpy((void*)0x200000c0, "./file0\000", 8);
> memcpy((void*)0x20000140, "ceph\000", 5);
> memcpy((void*)0x20000150, opt, sizeof(opt));
> syscall(__NR_mount, 0x20000040ul, 0x200000c0ul, 0x20000140ul, 0ul, 0x20000150);
> }
> int main(void)
> {
> syscall(__NR_mmap, 0x20000000ul, 0x1000000ul, 3ul, 0x32ul, -1, 0);
> for (procid = 0; procid < 6; procid++) {
> if (fork() == 0) {
> loop();
> }
> }
> sleep(1000000);
> return 0;
> }
> ===========================================================
>
> And it triggers the null-ptr-deref bug described above,
> its log is shown as below:
> ===========================================================
> [ 90.779695][ T6513] KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
> [ 90.782502][ T6513] RIP: 0010:strlen+0x1a/0x90
> [ ... ]
> [ 90.782502][ T6513] Call Trace:
> [ 90.782502][ T6513] <TASK>
> [ 90.782502][ T6513] ceph_parse_mount_param+0x89a/0x21e0
> [ 90.782502][ T6513] ? __kasan_unpoison_range-0xf/0x10
> [ 90.782502][ T6513] ? kasan_addr_to_slab-0xf/0x90
> [ 90.782502][ T6513] ? __sanitizer_cov_trace_pc+0x1a/0x40
> [ 90.782502][ T6513] ? ceph_parse_mount_param+0x0/0x21e0
> [ 90.782502][ T6513] ? audit_kill_trees+0x2b0/0x300
> [ 90.782502][ T6513] ? lock_release+0x0/0x760
> [ 90.782502][ T6513] ? __sanitizer_cov_trace_pc+0x1a/0x40
> [ 90.782502][ T6513] ? security_fs_context_parse_param+0x99/0xd0
> [ 90.782502][ T6513] ? ceph_parse_mount_param+0x0/0x21e0
> [ 90.782502][ T6513] vfs_parse_fs_param+0x20f/0x3d0
> [ 90.782502][ T6513] vfs_parse_fs_string+0xe4/0x180
> [ 90.782502][ T6513] ? vfs_parse_fs_string+0x0/0x180
> [ 90.782502][ T6513] ? rcu_read_lock_sched_held+0x0/0xd0
> [ 90.782502][ T6513] ? __sanitizer_cov_trace_pc+0x1a/0x40
> [ 90.782502][ T6513] ? kfree+0x129/0x1a0
> [ 90.782502][ T6513] ? __sanitizer_cov_trace_pc+0x1a/0x40
> [ 90.782502][ T6513] ? __sanitizer_cov_trace_pc+0x1a/0x40
> [ 90.782502][ T6513] generic_parse_monolithic+0x16f/0x1f0
> [ 90.782502][ T6513] ? generic_parse_monolithic+0x0/0x1f0
> [ 90.782502][ T6513] ? __sanitizer_cov_trace_pc+0x1a/0x40
> [ 90.782502][ T6513] ? alloc_fs_context+0x5cb/0xa00
> [ 90.782502][ T6513] path_mount+0x11d3/0x1cb0
> [ 90.782502][ T6513] ? path_mount+0x0/0x1cb0
> [ 90.782502][ T6513] ? putname+0xfe/0x140
> [ 90.782502][ T6513] do_mount+0xf3/0x110
> [ 90.782502][ T6513] ? do_mount+0x0/0x110
> [ 90.782502][ T6513] ? _copy_from_user+0xf7/0x170
> [ 90.782502][ T6513] ? __sanitizer_cov_trace_pc+0x1a/0x40
> [ 90.782502][ T6513] __x64_sys_mount+0x18f/0x230
> [ 90.782502][ T6513] do_syscall_64+0x35/0xb0
> [ 90.782502][ T6513] entry_SYSCALL_64_after_hwframe+0x63/0xcd
> [ ... ]
> [ 90.782502][ T6513] </TASK>
> ===========================================================
>
> By the way, commit "vfs: parse: deal with zero length string value"
> is still in discussion as below, so maybe this patchset is not
> needed.
> https://lore.kernel.org/all/[email protected]/

Okay, It's said that breaking commit will be reverted. Let's wait for a
while to see what will it be.

Thanks!

- Xiubo

>> But it will always make sense to fix it in ceph code with your patch.
>>
>> - Xiubo
>>
>>
>>

2022-10-24 04:07:23

by Ian Kent

[permalink] [raw]

Subject: Re: [PATCH -next 0/5] fs: fix possible null-ptr-deref when parsing param

On 24/10/22 08:42, Hawkins Jiawei wrote:
> On Mon, 24 Oct 2022 at 00:48, Al Viro <[email protected]> wrote:
>> On Mon, Oct 24, 2022 at 12:39:41AM +0800, Hawkins Jiawei wrote:
>>> According to commit "vfs: parse: deal with zero length string value",
>>> kernel will set the param->string to null pointer in vfs_parse_fs_string()
>>> if fs string has zero length.
>>>
>>> Yet the problem is that, when fs parses its mount parameters, it will
>>> dereferences the param->string, without checking whether it is a
>>> null pointer, which may trigger a null-ptr-deref bug.
>>>
>>> So this patchset reviews all functions for fs to parse parameters,
>>> by using `git grep -n "\.parse_param" fs/*`, and adds sanity check
>>> on param->string if its function will dereference param->string
>>> without check.
>> How about reverting the commit in question instead? Or dropping it
>> from patch series, depending upon the way akpm handles the pile
>> these days...
> I think both are OK.
>
> On one hand, commit "vfs: parse: deal with zero length string value"
> seems just want to make output more informattive, which probably is not
> the one which must be applied immediately to fix the
> panic.
>
> On the other hand, commit "vfs: parse: deal with zero length string value"
> affects so many file systems, so there are probably some deeper
> null-ptr-deref bugs I ignore, which may take time to review.

Yeah, it would be good to make the file system handling consistent

but I think there's been a bit too much breakage and it appears not

everyone thinks the approach is the right way to do it.

I'm thinking of abandoning this and restricting it to the "source"

parameter only to solve the user space mount table parser problem but

still doing it in the mount context code to keep it general (at least

for this case).

Ian

2022-10-24 10:00:25

by Andreas Grünbacher

[permalink] [raw]

Subject: Re: [PATCH -next 4/5] gfs2: fix possible null-ptr-deref when parsing param

Am So., 23. Okt. 2022 um 18:46 Uhr schrieb Hawkins Jiawei <[email protected]>:
> According to commit "vfs: parse: deal with zero length string value",
> kernel will set the param->string to null pointer in vfs_parse_fs_string()
> if fs string has zero length.
>
> Yet the problem is that, gfs2_parse_param() will dereferences the
> param->string, without checking whether it is a null pointer, which may
> trigger a null-ptr-deref bug.
>
> This patch solves it by adding sanity check on param->string
> in gfs2_parse_param().

Yes, but then it dereferences param->string in the error message. That
won't help.

> Reported-by: [email protected]
> Tested-by: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Hawkins Jiawei <[email protected]>
> ---
> fs/gfs2/ops_fstype.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> index c0cf1d2d0ef5..934746f18c25 100644
> --- a/fs/gfs2/ops_fstype.c
> +++ b/fs/gfs2/ops_fstype.c
> @@ -1446,12 +1446,18 @@ static int gfs2_parse_param(struct fs_context *fc, struct fs_parameter *param)
>
> switch (o) {
> case Opt_lockproto:
> + if (!param->string)
> + goto bad_val;
> strscpy(args->ar_lockproto, param->string, GFS2_LOCKNAME_LEN);
> break;
> case Opt_locktable:
> + if (!param->string)
> + goto bad_val;
> strscpy(args->ar_locktable, param->string, GFS2_LOCKNAME_LEN);
> break;
> case Opt_hostdata:
> + if (!param->string)
> + goto bad_val;
> strscpy(args->ar_hostdata, param->string, GFS2_LOCKNAME_LEN);
> break;
> case Opt_spectator:
> @@ -1535,6 +1541,10 @@ static int gfs2_parse_param(struct fs_context *fc, struct fs_parameter *param)
> return invalfc(fc, "invalid mount option: %s", param->key);
> }
> return 0;
> +
> +bad_val:
> + return invalfc(fc, "Bad value '%s' for mount option '%s'\n",
> + param->string, param->key);
> }
>
> static int gfs2_reconfigure(struct fs_context *fc)
> --
> 2.25.1
>

Thanks,
Andreas

2022-10-31 11:58:06

by Tetsuo Handa

[permalink] [raw]

Subject: Re: [PATCH -next 0/5] fs: fix possible null-ptr-deref when parsing param

On 2022/10/24 12:34, Ian Kent wrote:
>
> On 24/10/22 08:42, Hawkins Jiawei wrote:
>> On Mon, 24 Oct 2022 at 00:48, Al Viro <[email protected]> wrote:
>>> On Mon, Oct 24, 2022 at 12:39:41AM +0800, Hawkins Jiawei wrote:
>>>> According to commit "vfs: parse: deal with zero length string value",
>>>> kernel will set the param->string to null pointer in vfs_parse_fs_string()
>>>> if fs string has zero length.
>>>>
>>>> Yet the problem is that, when fs parses its mount parameters, it will
>>>> dereferences the param->string, without checking whether it is a
>>>> null pointer, which may trigger a null-ptr-deref bug.
>>>>
>>>> So this patchset reviews all functions for fs to parse parameters,
>>>> by using `git grep -n "\.parse_param" fs/*`, and adds sanity check
>>>> on param->string if its function will dereference param->string
>>>> without check.
>>> How about reverting the commit in question instead? Or dropping it
>>> from patch series, depending upon the way akpm handles the pile
>>> these days...
>> I think both are OK.
>>
>> On one hand, commit "vfs: parse: deal with zero length string value"
>> seems just want to make output more informattive, which probably is not
>> the one which must be applied immediately to fix the
>> panic.
>>
>> On the other hand, commit "vfs: parse: deal with zero length string value"
>> affects so many file systems, so there are probably some deeper
>> null-ptr-deref bugs I ignore, which may take time to review.
>
> Yeah, it would be good to make the file system handling consistent
> but I think there's been a bit too much breakage and it appears not
> everyone thinks the approach is the right way to do it.
>
> I'm thinking of abandoning this and restricting it to the "source"
> parameter only to solve the user space mount table parser problem but
> still doing it in the mount context code to keep it general (at least
> for this case).

No progress on this problem, and syzbot is reporting one after the other...

I think that reverting is the better choice.

2022-11-01 00:37:14

by Ian Kent

[permalink] [raw]

Subject: Re: [PATCH -next 0/5] fs: fix possible null-ptr-deref when parsing param

On 31/10/22 19:28, Tetsuo Handa wrote:
> On 2022/10/24 12:34, Ian Kent wrote:
>> On 24/10/22 08:42, Hawkins Jiawei wrote:
>>> On Mon, 24 Oct 2022 at 00:48, Al Viro <[email protected]> wrote:
>>>> On Mon, Oct 24, 2022 at 12:39:41AM +0800, Hawkins Jiawei wrote:
>>>>> According to commit "vfs: parse: deal with zero length string value",
>>>>> kernel will set the param->string to null pointer in vfs_parse_fs_string()
>>>>> if fs string has zero length.
>>>>>
>>>>> Yet the problem is that, when fs parses its mount parameters, it will
>>>>> dereferences the param->string, without checking whether it is a
>>>>> null pointer, which may trigger a null-ptr-deref bug.
>>>>>
>>>>> So this patchset reviews all functions for fs to parse parameters,
>>>>> by using `git grep -n "\.parse_param" fs/*`, and adds sanity check
>>>>> on param->string if its function will dereference param->string
>>>>> without check.
>>>> How about reverting the commit in question instead? Or dropping it
>>>> from patch series, depending upon the way akpm handles the pile
>>>> these days...
>>> I think both are OK.
>>>
>>> On one hand, commit "vfs: parse: deal with zero length string value"
>>> seems just want to make output more informattive, which probably is not
>>> the one which must be applied immediately to fix the
>>> panic.
>>>
>>> On the other hand, commit "vfs: parse: deal with zero length string value"
>>> affects so many file systems, so there are probably some deeper
>>> null-ptr-deref bugs I ignore, which may take time to review.
>> Yeah, it would be good to make the file system handling consistent
>> but I think there's been a bit too much breakage and it appears not
>> everyone thinks the approach is the right way to do it.
>>
>> I'm thinking of abandoning this and restricting it to the "source"
>> parameter only to solve the user space mount table parser problem but
>> still doing it in the mount context code to keep it general (at least
>> for this case).
> No progress on this problem, and syzbot is reporting one after the other...
>
> I think that reverting is the better choice.

Yes, I agree/

Ian