Message-ID: <CAEkJfYOTNdu2rK-7F58sg7XuYk+trdGDBjYo-y0SusxmSeCkFA@mail.gmail.com>
Date: Tue, 5 Mar 2024 10:32:31 +0800
From: Sam Sun <samsun1006219@...il.com>
To: Paul Moore <paul@...l-moore.com>
Cc: Christian Göttsche <cgzones@...glemail.com>, 
	linux-kernel@...r.kernel.org, "xrivendell7@...il.com" <xrivendell7@...il.com>, 
	syzkaller@...glegroups.com, selinux@...r.kernel.org, omosnace@...hat.com
Subject: Re: [Bug] WARNING: zero-size vmalloc in sel_write_load

On Tue, Mar 5, 2024 at 4:45 AM Paul Moore <paul@...l-moore.com> wrote:
>
> On Mon, Mar 4, 2024 at 3:11 PM Christian Göttsche
> <cgzones@...glemail.com> wrote:
> > On Mon, 4 Mar 2024 at 20:19, Paul Moore <paul@...l-moore.com> wrote:
> > > On Mon, Mar 4, 2024 at 9:08 AM Sam Sun <samsun1006219@...il.com> wrote:
> > > >
> > > > Dear developers and maintainers,
> > > >
> > > > We encountered a warning in function sel_write_load(). It is tested on
> > > > kernel 6.8.0-rc7. Bug report is listed below.
> > > >
> > > > ```
> > > > WARNING: CPU: 1 PID: 8109 at mm/vmalloc.c:3247
> > > > __vmalloc_node_range+0x10a0/0x1490 mm/vmalloc.c:3247
> > > > Modules linked in:
> > > > CPU: 1 PID: 8109 Comm: syz-executor370 Not tainted 6.7.0-rc7 #1
> > > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
> > > > RIP: 0010:__vmalloc_node_range+0x10a0/0x1490 mm/vmalloc.c:3247
> > > > Code: 65 48 2b 04 25 28 00 00 00 0f 85 98 02 00 00 48 81 c4 70 01 00
> > > > 00 4c 89 e0 5b 5d 41 5c 41 5d 41 5e 41 5f c3 e8 71 43 b7 ff 90 <0f> 0b
> > > > 90 45 31 e4 eb a1 e8 63 43 b7 ff 48 8b 4c 24 40 31 f6 45 31
> > > > RSP: 0018:ffffc90002adf9c0 EFLAGS: 00010293
> > > > RAX: 0000000000000000 RBX: dffffc0000000000 RCX: ffffffff81cdc194
> > > > RDX: ffff888022124ec0 RSI: ffffffff81cdd16f RDI: 0000000000000007
> > > > RBP: 0000000000000000 R08: 0000000000000007 R09: 0000000000000000
> > > > R10: 0000000000000000 R11: 0000000000000001 R12: ffff888107373a48
> > > > R13: dffffc0000000000 R14: 0000000000000000 R15: ffffc90002adfec8
> > > > FS:  00005555560953c0(0000) GS:ffff888135c00000(0000) knlGS:0000000000000000
> > > > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > CR2: 0000000020000010 CR3: 000000010503d000 CR4: 0000000000750ef0
> > > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > > > PKRU: 55555554
> > > > Call Trace:
> > > >  <TASK>
> > > >  __vmalloc_node mm/vmalloc.c:3385 [inline]
> > > >  vmalloc+0x6b/0x80 mm/vmalloc.c:3418
> > > >  sel_write_load+0x27f/0x19d0 security/selinux/selinuxfs.c:603
> > > >  vfs_write+0x2a9/0xd80 fs/read_write.c:582
> > > >  ksys_pwrite64 fs/read_write.c:699 [inline]
> > > >  __do_sys_pwrite64 fs/read_write.c:709 [inline]
> > > >  __se_sys_pwrite64 fs/read_write.c:706 [inline]
> > > >  __x64_sys_pwrite64+0x1f3/0x250 fs/read_write.c:706
> > > >  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
> > > >  do_syscall_64+0x40/0x110 arch/x86/entry/common.c:83
> > > >  entry_SYSCALL_64_after_hwframe+0x63/0x6b
> > > > RIP: 0033:0x7f40e7728f8d
> > > > Code: 28 c3 e8 46 1e 00 00 66 0f 1f 44 00 00 f3 0f 1e fa 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 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
> > > > RSP: 002b:00007fff5bf39508 EFLAGS: 00000246 ORIG_RAX: 0000000000000012
> > > > RAX: ffffffffffffffda RBX: 00007fff5bf39708 RCX: 00007f40e7728f8d
> > > > RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000003
> > > > RBP: 0000000000000001 R08: 0000000000000000 R09: 00007fff5bf39708
> > > > R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000001
> > > > R13: 00007fff5bf396f8 R14: 00007f40e77a6530 R15: 0000000000000001
> > > >  </TASK>
> > > > ```
> > > >
> > > > The cause of this bug is that in sel_write_load(), parameter "count"
> > > > is controlled by user, which could be zero. It is passed to vmalloc()
> > > > as an argument.
> > > >
> > > > If you have any questions, please contact us.
> > > > Reported by: Yue Sun <samsun1006219@...il.com>
> > > > Reported by: xingwei lee <xrivendell7@...il.com>
> > >
> > > Everything appears to be working as expected, vmalloc() caught the
> > > zero-length allocation request, emitted the warning, returned NULL to
> > > sel_write_load(), and sel_write_load() handled the error condition
> > > triggered by vmalloc(0) returning NULL.  Did you see any unexpected
> > > behavior beyond this warning message above?
> >
> > Probably because kernel warnings should not be reachable from
> > userspace ...
>
> My question was asking if the reporter was seeing any unexpected
> behavior *beyond* the warning message.  I wanted to make sure they
> weren't seeing anything else on their system that we should also take
> into account.
>

I didn't see any unexpected behavior beyond this warning message. You
are right, everything appears to be working as expected. Like
Christian said, I enabled kernel panic_on_warn config. I thought
kernel warning was something worthy to report, but I was wrong. In
future reports, I will check carefully to eliminate kernel warnings
like "vmalloc zero" and "page size alloc too large" if they don't have
unexpected behaviors. Sorry for wasting your time analyzing it, and
thanks for pointing out my mistake!

> > ... although in this case loading a policy is a highly
> > privileged operation - , because they mostly signal incorrect internal
> > state and can lead with the enabled option of panic_on_warn to system
> > halts.
> >
> > I have two suggestions:
> >
> > I. Can the documentation of vmalloc() mention that passing a size of 0
> > is discouraged?
>
> One could always submit a patch and see what happens, that's almost
> always the best way to get feedback.
>
> > II. Can the global SELinux state mutex in sel_write_load() be acquired
> > after the avc permission check, so that rouge processes with write
> > access to /load but not granted security { load_policy } can not pound
> > on that mutex?
>
> We need to make sure that there is not a window between the
> avc_has_perm() permission check and the actual policy load in
> security_load_policy() where another task could race and cause some
> unexpected behavior.  For that reason I think we need to take the
> mutex before the avc_has_perm() call, and we likely want to keep the
> vmalloc()/copy_from_user() after the permission check for the same
> reason you wanted to delay taking the mutex.
>
> We probably could consider moving the @ppos and @count sanity checks
> before the mutex.
>
> --
> paul-moore.com

Best Regards,
Yue

Powered by blists - more mailing lists