| Message ID | 20221023163945.39920-4-yin31149@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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
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
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 > > >
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);
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(+)