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

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

[Hawkins Jiawei]

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.

Hawkins Jiawei (5):
  smb3: fix possible null-ptr-deref when parsing param
  nfs: fix possible null-ptr-deref when parsing param
  ceph: fix possible null-ptr-deref when parsing param
  gfs2: fix possible null-ptr-deref when parsing param
  proc: fix possible null-ptr-deref when parsing param

 fs/ceph/super.c      |  3 +++
 fs/cifs/fs_context.c | 58 +++++++++++++++++++++++++++++++++++++++++++-
 fs/gfs2/ops_fstype.c | 10 ++++++++
 fs/nfs/fs_context.c  |  6 +++++
 fs/proc/root.c       |  3 +++
 5 files changed, 79 insertions(+), 1 deletion(-)

-- 
2.25.1

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

[Hawkins Jiawei]

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 <yin31149@gmail.com>
---
 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

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

[Hawkins Jiawei]

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, nfs_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 nfs_fs_context_parse_param().

Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
---
 fs/nfs/fs_context.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
index 4da701fd1424..0c330bc13ef2 100644
--- a/fs/nfs/fs_context.c
+++ b/fs/nfs/fs_context.c
@@ -684,6 +684,8 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
 			return ret;
 		break;
 	case Opt_vers:
+		if (!param->string)
+			goto out_invalid_value;
 		trace_nfs_mount_assign(param->key, param->string);
 		ret = nfs_parse_version_string(fc, param->string);
 		if (ret < 0)
@@ -696,6 +698,8 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
 		break;
 
 	case Opt_proto:
+		if (!param->string)
+			goto out_invalid_value;
 		trace_nfs_mount_assign(param->key, param->string);
 		protofamily = AF_INET;
 		switch (lookup_constant(nfs_xprt_protocol_tokens, param->string, -1)) {
@@ -732,6 +736,8 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
 		break;
 
 	case Opt_mountproto:
+		if (!param->string)
+			goto out_invalid_value;
 		trace_nfs_mount_assign(param->key, param->string);
 		mountfamily = AF_INET;
 		switch (lookup_constant(nfs_xprt_protocol_tokens, param->string, -1)) {
-- 
2.25.1

Re: [PATCH -next 2/5] nfs: fix possible null-ptr-deref when parsing param

[Jeff Layton]

On Mon, 2022-10-24 at 00:39 +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, nfs_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 nfs_fs_context_parse_param().
> 
> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
> ---
>  fs/nfs/fs_context.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
> index 4da701fd1424..0c330bc13ef2 100644
> --- a/fs/nfs/fs_context.c
> +++ b/fs/nfs/fs_context.c
> @@ -684,6 +684,8 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
>  			return ret;
>  		break;
>  	case Opt_vers:
> +		if (!param->string)
> +			goto out_invalid_value;
>  		trace_nfs_mount_assign(param->key, param->string);
>  		ret = nfs_parse_version_string(fc, param->string);
>  		if (ret < 0)
> @@ -696,6 +698,8 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
>  		break;
>  
>  	case Opt_proto:
> +		if (!param->string)
> +			goto out_invalid_value;
>  		trace_nfs_mount_assign(param->key, param->string);
>  		protofamily = AF_INET;
>  		switch (lookup_constant(nfs_xprt_protocol_tokens, param->string, -1)) {
> @@ -732,6 +736,8 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
>  		break;
>  
>  	case Opt_mountproto:
> +		if (!param->string)
> +			goto out_invalid_value;
>  		trace_nfs_mount_assign(param->key, param->string);
>  		mountfamily = AF_INET;
>  		switch (lookup_constant(nfs_xprt_protocol_tokens, param->string, -1)) {


Looks reasonable. I took a quick look for other fsparam_string values
that might not handle a NULL pointer correctly, but I didn't see any.

Reviewed-by: Jeff Layton <jlayton@kernel.org>

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

[Hawkins Jiawei]

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 <yin31149@gmail.com>
---
 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);
-- 
2.25.1

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

[Xiubo Li]

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 <yin31149@gmail.com>
> ---
>   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

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

[Xiubo Li]

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 <yin31149@gmail.com>
> ---
>   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

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

[Hawkins Jiawei]

Hi Xiubo,
On Mon, 24 Oct 2022 at 08:55, Xiubo Li <xiubli@redhat.com> 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 <yin31149@gmail.com>
> > ---
> >   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/17a1fdc-14a0-cf3c-784f-baa939895aef@google.com/
>
> But it will always make sense to fix it in ceph code with your patch.
>
> - Xiubo
>
>
>

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

[Xiubo Li]

On 24/10/2022 10:04, Hawkins Jiawei wrote:
> Hi Xiubo,
> On Mon, 24 Oct 2022 at 08:55, Xiubo Li <xiubli@redhat.com> 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 <yin31149@gmail.com>
>>> ---
>>>    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/17a1fdc-14a0-cf3c-784f-baa939895aef@google.com/

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

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

[Hawkins Jiawei]

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().

Reported-by: syzbot+da97a57c5b742d05db51@syzkaller.appspotmail.com
Tested-by: syzbot+da97a57c5b742d05db51@syzkaller.appspotmail.com
Cc: agruenba@redhat.com
Cc: cluster-devel@redhat.com
Cc: linux-kernel@vger.kernel.org
Cc: rpeterso@redhat.com
Cc: syzkaller-bugs@googlegroups.com
Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
---
 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

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

[Andreas Grünbacher]

Am So., 23. Okt. 2022 um 18:46 Uhr schrieb Hawkins Jiawei <yin31149@gmail.com>:
> 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: syzbot+da97a57c5b742d05db51@syzkaller.appspotmail.com
> Tested-by: syzbot+da97a57c5b742d05db51@syzkaller.appspotmail.com
> Cc: agruenba@redhat.com
> Cc: cluster-devel@redhat.com
> Cc: linux-kernel@vger.kernel.org
> Cc: rpeterso@redhat.com
> Cc: syzkaller-bugs@googlegroups.com
> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
> ---
>  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

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

[Hawkins Jiawei]

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, proc_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 proc_parse_param().

Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
---
 fs/proc/root.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/proc/root.c b/fs/proc/root.c
index 3c2ee3eb1138..5346809dc3c3 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -130,6 +130,9 @@ static int proc_parse_param(struct fs_context *fc, struct fs_parameter *param)
 		break;
 
 	case Opt_subset:
+		if (!param->string)
+			return invalfc(fc, "Bad value '%s' for mount option '%s'\n",
+				       param->string, param->key);
 		if (proc_parse_subset_param(fc, param->string) < 0)
 			return -EINVAL;
 		break;
-- 
2.25.1

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

[Al Viro]

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...

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

[Hawkins Jiawei]

On Mon, 24 Oct 2022 at 00:48, Al Viro <viro@zeniv.linux.org.uk> 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.

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

[Ian Kent]

On 24/10/22 08:42, Hawkins Jiawei wrote:
> On Mon, 24 Oct 2022 at 00:48, Al Viro <viro@zeniv.linux.org.uk> 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

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

[Tetsuo Handa]

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 <viro@zeniv.linux.org.uk> 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.

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

[Ian Kent]

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 <viro@zeniv.linux.org.uk> 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