Re: [kernfs] e19dfdc83b: BUG:KASAN:global-out-of-bounds_in_s

Re: [kernfs] e19dfdc83b: BUG:KASAN:global-out-of-bounds_in_s

[Paul Moore]

Ondrej, please look into this.

You've looked at this code more recently than I have, but it looks
like there might be an issue with __kernfs_iattrs() returning a
pointer to a kernfs_iattrs object without taking a kernfs reference
(kernfs_get(kn)).  Although I would be a little surprised if this was
the problem as I think it would cause a number of issues beyond just
this one ... ?

On Mon, Mar 25, 2019 at 10:50 AM kernel test robot
<rong.a.chen@intel.com> wrote:
>
> FYI, we noticed the following commit (built with gcc-7):
>
> commit: e19dfdc83b60f196e0653d683499f7bc5548128f ("kernfs: initialize security of newly created nodes")
> https://git.kernel.org/cgit/linux/kernel/git/pcmoore/selinux.git next
>
> in testcase: locktorture
> with following parameters:
>
>         runtime: 300s
>         test: default
>
> test-description: This torture test consists of creating a number of kernel threads which acquire the lock and hold it for specific amount of time, thus simulating different critical region behaviors.
> test-url: https://www.kernel.org/doc/Documentation/locking/locktorture.txt
>
>
> on test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 2G
>
> caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
>
>
> +-------------------------------------------------+------------+------------+
> |                                                 | ec882da5cd | e19dfdc83b |
> +-------------------------------------------------+------------+------------+
> | boot_successes                                  | 0          | 0          |
> | boot_failures                                   | 8          | 8          |
> | BUG:kernel_reboot-without-warning_in_test_stage | 8          |            |
> | BUG:KASAN:global-out-of-bounds_in_s             | 0          | 8          |
> +-------------------------------------------------+------------+------------+
>
>
>
> [   27.938038] BUG: KASAN: global-out-of-bounds in strcmp+0x97/0xa0
> [   27.940755] Read of size 1 at addr ffffffff946a83d7 by task systemd/1
> [   27.943554]
> [   27.944603] CPU: 0 PID: 1 Comm: systemd Not tainted 5.1.0-rc1-00010-ge19dfdc #1
> [   27.948091] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
> [   27.951946] Call Trace:
> [   27.953353]  ? strcmp+0x97/0xa0
> [   27.955026]  print_address_description+0x22/0x270
> [   27.957203]  ? strcmp+0x97/0xa0
> [   27.958841]  kasan_report+0x13b/0x1d0
> [   27.960759]  ? strcmp+0x97/0xa0
> [   27.962378]  ? strcmp+0x97/0xa0
> [   27.963976]  strcmp+0x97/0xa0
> [   27.965846]  simple_xattr_get+0x7b/0x120
> [   27.967473]  selinux_kernfs_init_security+0x108/0x440
> [   27.969360]  ? __radix_tree_replace+0x9a/0x230
> [   27.971200]  ? selinux_secctx_to_secid+0x20/0x20
> [   27.973011]  ? __fprop_inc_percpu_max+0x190/0x190
> [   27.975563]  ? kvm_sched_clock_read+0x12/0x20
> [   27.977907]  ? sched_clock+0x5/0x10
> [   27.979867]  ? sched_clock_cpu+0x24/0xb0
> [   27.982048]  ? idr_alloc_cyclic+0xcb/0x190
> [   27.984229]  ? lock_downgrade+0x620/0x620
> [   27.986388]  security_kernfs_init_security+0x3c/0x70
> [   27.989012]  __kernfs_new_node+0x403/0x5e0
> [   27.991195]  ? kernfs_dop_revalidate+0x330/0x330
> [   27.993589]  ? css_next_child+0xec/0x260
> [   27.995685]  ? css_next_descendant_pre+0x36/0x110
> [   27.998115]  ? cgroup_propagate_control+0x2d6/0x460
> [   28.000662]  kernfs_new_node+0x72/0x140
> [   28.002818]  ? lockdep_hardirqs_on+0x379/0x560
> [   28.005171]  ? cgroup_idr_replace+0x35/0x40
> [   28.007417]  kernfs_create_dir_ns+0x26/0x130
> [   28.009690]  cgroup_mkdir+0x3b9/0xef0
> [   28.011764]  ? cgroup_destroy_locked+0x5e0/0x5e0
> [   28.014196]  kernfs_iop_mkdir+0x12f/0x1b0
> [   28.016396]  vfs_mkdir+0x2e6/0x510
> [   28.018317]  do_mkdirat+0x19b/0x1f0
> [   28.020284]  ? __x64_sys_mknod+0xb0/0xb0
> [   28.022437]  do_syscall_64+0xe5/0x10d0
> [   28.024408]  ? syscall_return_slowpath+0x790/0x790
> [   28.026874]  ? entry_SYSCALL_64_after_hwframe+0x3e/0xbe
> [   28.029504]  ? trace_hardirqs_off_caller+0x58/0x200
> [   28.031993]  ? trace_hardirqs_off_thunk+0x1a/0x1c
> [   28.034438]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [   28.036748] RIP: 0033:0x7f38cab6f447
> [   28.038825] Code: 00 b8 ff ff ff ff c3 0f 1f 40 00 48 8b 05 49 da 2b 00 64 c7 00 5f 00 00 00 b8 ff ff ff ff c3 0f 1f 40 00 b8 53 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 21 da 2b 00 f7 d8 64 89 01 48
> [   28.047736] RSP: 002b:00007ffeef143d88 EFLAGS: 00000246 ORIG_RAX: 0000000000000053
> [   28.051776] RAX: ffffffffffffffda RBX: 0000000000000001 RCX: 00007f38cab6f447
> [   28.055117] RDX: 00007ffeef143c30 RSI: 00000000000001ed RDI: 000055a7b0458560
> [   28.058533] RBP: 0000000000000040 R08: 0000000000000000 R09: 2f73662f7379732f
> [   28.062031] R10: 732f70756f726763 R11: 0000000000000246 R12: 000055a7b04b30a0
> [   28.065528] R13: 0000000000000000 R14: 000055a7b046bb88 R15: 000055a7b046b540
> [   28.068977]
> [   28.070240] The buggy address belongs to the variable:
> [   28.072491]  securityfs_super_operations+0x4917/0x6220
> [   28.075171]
> [   28.076286] Memory state around the buggy address:
> [   28.078861]  ffffffff946a8280: fa fa fa fa 00 01 fa fa fa fa fa fa 00 02 fa fa
> [   28.082610]  ffffffff946a8300: fa fa fa fa 00 02 fa fa fa fa fa fa 00 01 fa fa
> [   28.086669] >ffffffff946a8380: fa fa fa fa 00 03 fa fa fa fa fa fa 00 fa fa fa
> [   28.090587]                                                  ^
> [   28.093576]  ffffffff946a8400: fa fa fa fa 00 00 00 00 00 00 05 fa fa fa fa fa
> [   28.097599]  ffffffff946a8480: 00 00 01 fa fa fa fa fa 00 00 00 00 00 00 00 00
> [   28.101453] ==================================================================
> [   28.105478] Disabling lock debugging due to kernel taint
>          Starting Load Kernel Modules...
>          Mounting Debug File System...
> ] Listening on RPCbind Server Activation Socket.
>          Starting Remount Root and Kernel File Systems...
>          Starting Journal Service...
>          Mounting RPC Pipe File System...
> [   28.508319] _warn_unseeded_randomness: 131 callbacks suppressed
> [   28.508335] random: get_random_u64 called from copy_process+0x596/0x6450 with crng_init=1
>          Starting Create Static Device Nodes in /dev...
> [   28.552988] random: get_random_u64 called from arch_pick_mmap_layout+0x4a1/0x600 with crng_init=1
> [   28.556785] random: get_random_u64 called from arch_pick_mmap_layout+0x446/0x600 with crng_init=1
>          Starting Load/Save Random Seed...
>          Starting udev Coldplug all Devices...
>          Mounting FUSE Control File System...
>          Starting Apply Kernel Variables...
>          Mounting Configuration File System...
>          Starting Raise network interfaces...
>          Starting Preprocess NFS configuration...
>          Starting udev Kernel Device Manager...
>          Starting Flush Journal to Persistent Storage...
>          Starting Create Volatile Files and Directories...
> [   29.523554] random: get_random_u64 called from arch_pick_mmap_layout+0x446/0x600 with crng_init=1
> [   29.527262] random: get_random_u64 called from load_elf_binary+0x1281/0x2f30 with crng_init=1
>
>          Starting RPC bind portmap service...
>          Starting Network Time Synchronization...
>          Starting Update UTMP about System Boot/Shutdown...
> [   30.574449] _warn_unseeded_randomness: 154 callbacks suppressed
> [   30.574479] random: get_random_u32 called from bucket_table_alloc+0x149/0x370 with crng_init=1
> [   32.628754] random: get_random_u64 called from arch_pick_mmap_layout+0x4a1/0x600 with crng_init=1
> [   32.632973] random: get_random_u64 called from arch_pick_mmap_layout+0x446/0x600 with crng_init=1
> [   32.637364] random: get_random_u64 called from load_elf_binary+0x1281/0x2f30 with crng_init=1
>          Starting Login Service...
>          Starting LSB: Start and stop bmc-watchdog...
>          Starting LSB: Execute the kexec -e command to reboot system...
>
>
> To reproduce:
>
>         # build kernel
>         cd linux
>         cp config-5.1.0-rc1-00010-ge19dfdc .config
>         make HOSTCC=gcc-7 CC=gcc-7 ARCH=x86_64 olddefconfig
>         make HOSTCC=gcc-7 CC=gcc-7 ARCH=x86_64 prepare
>         make HOSTCC=gcc-7 CC=gcc-7 ARCH=x86_64 modules_prepare
>         make HOSTCC=gcc-7 CC=gcc-7 ARCH=x86_64 SHELL=/bin/bash
>         make HOSTCC=gcc-7 CC=gcc-7 ARCH=x86_64 bzImage
>
>
>         git clone https://github.com/intel/lkp-tests.git
>         cd lkp-tests
>         find lib/ | cpio -o -H newc --quiet | gzip > modules.cgz
>         bin/lkp qemu -k <bzImage> -m modules.cgz job-script # job-script is attached in this email
>
>
>
>
> Thanks,
> Rong Chen
>


-- 
paul moore
www.paul-moore.com

Re: [kernfs] e19dfdc83b: BUG:KASAN:global-out-of-bounds_in_s

[Ondrej Mosnacek]

On Mon, Mar 25, 2019 at 4:17 PM Paul Moore <paul@paul-moore.com> wrote:
> Ondrej, please look into this.
>
> You've looked at this code more recently than I have, but it looks
> like there might be an issue with __kernfs_iattrs() returning a
> pointer to a kernfs_iattrs object without taking a kernfs reference
> (kernfs_get(kn)).  Although I would be a little surprised if this was
> the problem as I think it would cause a number of issues beyond just
> this one ... ?

I think this is actually because of how xattr_full_name() reconstructs
the full name from the xattr suffix. It assumes that the suffix was
obtained from the full name by just taking a pointer inside it, but in
kernfs_security_xattr_get/set() I pass the suffix directly... I'm
surprised that this didn't fail spectacularly earlier during testing.
Maybe the newer GCC does some clever merging of the string constants,
so that XATTR_SELINUX_SUFFIX actually ends up as a substring of
XATTR_NAME_SELINUX? (That would be one hell of a "lucky" coincidence
:)

I'll post a patch that converts kernfs_security_xattr_get/set() to
take the full name and hopefully that will fix the problem. I'll see
if I can run the reproducer locally tomorrow...

>
> On Mon, Mar 25, 2019 at 10:50 AM kernel test robot
> <rong.a.chen@intel.com> wrote:
> >
> > FYI, we noticed the following commit (built with gcc-7):
> >
> > commit: e19dfdc83b60f196e0653d683499f7bc5548128f ("kernfs: initialize security of newly created nodes")
> > https://git.kernel.org/cgit/linux/kernel/git/pcmoore/selinux.git next
> >
> > in testcase: locktorture
> > with following parameters:
> >
> >         runtime: 300s
> >         test: default
> >
> > test-description: This torture test consists of creating a number of kernel threads which acquire the lock and hold it for specific amount of time, thus simulating different critical region behaviors.
> > test-url: https://www.kernel.org/doc/Documentation/locking/locktorture.txt
> >
> >
> > on test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 2G
> >
> > caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
> >
> >
> > +-------------------------------------------------+------------+------------+
> > |                                                 | ec882da5cd | e19dfdc83b |
> > +-------------------------------------------------+------------+------------+
> > | boot_successes                                  | 0          | 0          |
> > | boot_failures                                   | 8          | 8          |
> > | BUG:kernel_reboot-without-warning_in_test_stage | 8          |            |
> > | BUG:KASAN:global-out-of-bounds_in_s             | 0          | 8          |
> > +-------------------------------------------------+------------+------------+
> >
> >
> >
> > [   27.938038] BUG: KASAN: global-out-of-bounds in strcmp+0x97/0xa0
> > [   27.940755] Read of size 1 at addr ffffffff946a83d7 by task systemd/1
> > [   27.943554]
> > [   27.944603] CPU: 0 PID: 1 Comm: systemd Not tainted 5.1.0-rc1-00010-ge19dfdc #1
> > [   27.948091] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
> > [   27.951946] Call Trace:
> > [   27.953353]  ? strcmp+0x97/0xa0
> > [   27.955026]  print_address_description+0x22/0x270
> > [   27.957203]  ? strcmp+0x97/0xa0
> > [   27.958841]  kasan_report+0x13b/0x1d0
> > [   27.960759]  ? strcmp+0x97/0xa0
> > [   27.962378]  ? strcmp+0x97/0xa0
> > [   27.963976]  strcmp+0x97/0xa0
> > [   27.965846]  simple_xattr_get+0x7b/0x120
> > [   27.967473]  selinux_kernfs_init_security+0x108/0x440
> > [   27.969360]  ? __radix_tree_replace+0x9a/0x230
> > [   27.971200]  ? selinux_secctx_to_secid+0x20/0x20
> > [   27.973011]  ? __fprop_inc_percpu_max+0x190/0x190
> > [   27.975563]  ? kvm_sched_clock_read+0x12/0x20
> > [   27.977907]  ? sched_clock+0x5/0x10
> > [   27.979867]  ? sched_clock_cpu+0x24/0xb0
> > [   27.982048]  ? idr_alloc_cyclic+0xcb/0x190
> > [   27.984229]  ? lock_downgrade+0x620/0x620
> > [   27.986388]  security_kernfs_init_security+0x3c/0x70
> > [   27.989012]  __kernfs_new_node+0x403/0x5e0
> > [   27.991195]  ? kernfs_dop_revalidate+0x330/0x330
> > [   27.993589]  ? css_next_child+0xec/0x260
> > [   27.995685]  ? css_next_descendant_pre+0x36/0x110
> > [   27.998115]  ? cgroup_propagate_control+0x2d6/0x460
> > [   28.000662]  kernfs_new_node+0x72/0x140
> > [   28.002818]  ? lockdep_hardirqs_on+0x379/0x560
> > [   28.005171]  ? cgroup_idr_replace+0x35/0x40
> > [   28.007417]  kernfs_create_dir_ns+0x26/0x130
> > [   28.009690]  cgroup_mkdir+0x3b9/0xef0
> > [   28.011764]  ? cgroup_destroy_locked+0x5e0/0x5e0
> > [   28.014196]  kernfs_iop_mkdir+0x12f/0x1b0
> > [   28.016396]  vfs_mkdir+0x2e6/0x510
> > [   28.018317]  do_mkdirat+0x19b/0x1f0
> > [   28.020284]  ? __x64_sys_mknod+0xb0/0xb0
> > [   28.022437]  do_syscall_64+0xe5/0x10d0
> > [   28.024408]  ? syscall_return_slowpath+0x790/0x790
> > [   28.026874]  ? entry_SYSCALL_64_after_hwframe+0x3e/0xbe
> > [   28.029504]  ? trace_hardirqs_off_caller+0x58/0x200
> > [   28.031993]  ? trace_hardirqs_off_thunk+0x1a/0x1c
> > [   28.034438]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > [   28.036748] RIP: 0033:0x7f38cab6f447
> > [   28.038825] Code: 00 b8 ff ff ff ff c3 0f 1f 40 00 48 8b 05 49 da 2b 00 64 c7 00 5f 00 00 00 b8 ff ff ff ff c3 0f 1f 40 00 b8 53 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 21 da 2b 00 f7 d8 64 89 01 48
> > [   28.047736] RSP: 002b:00007ffeef143d88 EFLAGS: 00000246 ORIG_RAX: 0000000000000053
> > [   28.051776] RAX: ffffffffffffffda RBX: 0000000000000001 RCX: 00007f38cab6f447
> > [   28.055117] RDX: 00007ffeef143c30 RSI: 00000000000001ed RDI: 000055a7b0458560
> > [   28.058533] RBP: 0000000000000040 R08: 0000000000000000 R09: 2f73662f7379732f
> > [   28.062031] R10: 732f70756f726763 R11: 0000000000000246 R12: 000055a7b04b30a0
> > [   28.065528] R13: 0000000000000000 R14: 000055a7b046bb88 R15: 000055a7b046b540
> > [   28.068977]
> > [   28.070240] The buggy address belongs to the variable:
> > [   28.072491]  securityfs_super_operations+0x4917/0x6220
> > [   28.075171]
> > [   28.076286] Memory state around the buggy address:
> > [   28.078861]  ffffffff946a8280: fa fa fa fa 00 01 fa fa fa fa fa fa 00 02 fa fa
> > [   28.082610]  ffffffff946a8300: fa fa fa fa 00 02 fa fa fa fa fa fa 00 01 fa fa
> > [   28.086669] >ffffffff946a8380: fa fa fa fa 00 03 fa fa fa fa fa fa 00 fa fa fa
> > [   28.090587]                                                  ^
> > [   28.093576]  ffffffff946a8400: fa fa fa fa 00 00 00 00 00 00 05 fa fa fa fa fa
> > [   28.097599]  ffffffff946a8480: 00 00 01 fa fa fa fa fa 00 00 00 00 00 00 00 00
> > [   28.101453] ==================================================================
> > [   28.105478] Disabling lock debugging due to kernel taint
> >          Starting Load Kernel Modules...
> >          Mounting Debug File System...
> > ] Listening on RPCbind Server Activation Socket.
> >          Starting Remount Root and Kernel File Systems...
> >          Starting Journal Service...
> >          Mounting RPC Pipe File System...
> > [   28.508319] _warn_unseeded_randomness: 131 callbacks suppressed
> > [   28.508335] random: get_random_u64 called from copy_process+0x596/0x6450 with crng_init=1
> >          Starting Create Static Device Nodes in /dev...
> > [   28.552988] random: get_random_u64 called from arch_pick_mmap_layout+0x4a1/0x600 with crng_init=1
> > [   28.556785] random: get_random_u64 called from arch_pick_mmap_layout+0x446/0x600 with crng_init=1
> >          Starting Load/Save Random Seed...
> >          Starting udev Coldplug all Devices...
> >          Mounting FUSE Control File System...
> >          Starting Apply Kernel Variables...
> >          Mounting Configuration File System...
> >          Starting Raise network interfaces...
> >          Starting Preprocess NFS configuration...
> >          Starting udev Kernel Device Manager...
> >          Starting Flush Journal to Persistent Storage...
> >          Starting Create Volatile Files and Directories...
> > [   29.523554] random: get_random_u64 called from arch_pick_mmap_layout+0x446/0x600 with crng_init=1
> > [   29.527262] random: get_random_u64 called from load_elf_binary+0x1281/0x2f30 with crng_init=1
> >
> >          Starting RPC bind portmap service...
> >          Starting Network Time Synchronization...
> >          Starting Update UTMP about System Boot/Shutdown...
> > [   30.574449] _warn_unseeded_randomness: 154 callbacks suppressed
> > [   30.574479] random: get_random_u32 called from bucket_table_alloc+0x149/0x370 with crng_init=1
> > [   32.628754] random: get_random_u64 called from arch_pick_mmap_layout+0x4a1/0x600 with crng_init=1
> > [   32.632973] random: get_random_u64 called from arch_pick_mmap_layout+0x446/0x600 with crng_init=1
> > [   32.637364] random: get_random_u64 called from load_elf_binary+0x1281/0x2f30 with crng_init=1
> >          Starting Login Service...
> >          Starting LSB: Start and stop bmc-watchdog...
> >          Starting LSB: Execute the kexec -e command to reboot system...
> >
> >
> > To reproduce:
> >
> >         # build kernel
> >         cd linux
> >         cp config-5.1.0-rc1-00010-ge19dfdc .config
> >         make HOSTCC=gcc-7 CC=gcc-7 ARCH=x86_64 olddefconfig
> >         make HOSTCC=gcc-7 CC=gcc-7 ARCH=x86_64 prepare
> >         make HOSTCC=gcc-7 CC=gcc-7 ARCH=x86_64 modules_prepare
> >         make HOSTCC=gcc-7 CC=gcc-7 ARCH=x86_64 SHELL=/bin/bash
> >         make HOSTCC=gcc-7 CC=gcc-7 ARCH=x86_64 bzImage
> >
> >
> >         git clone https://github.com/intel/lkp-tests.git
> >         cd lkp-tests
> >         find lib/ | cpio -o -H newc --quiet | gzip > modules.cgz
> >         bin/lkp qemu -k <bzImage> -m modules.cgz job-script # job-script is attached in this email
> >
> >
> >
> >
> > Thanks,
> > Rong Chen
> >
>
>
> --
> paul moore
> www.paul-moore.com

--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.

[PATCH] kernfs: fix xattr name handling in LSM helpers

[Ondrej Mosnacek]

The implementation of kernfs_security_xattr_*() helpers reuses the
kernfs_node_xattr_*() functions, which take the suffix of the xattr name
and extract full xattr name from it using xattr_full_name(). However,
this function relies on the fact that the suffix passed to xattr
handlers from VFS is always constructed from the full name by just
incerementing the pointer. This doesn't necessarily hold for the callers
of kernfs_security_xattr_*(), so their usage will easily lead to
out-of-bounds access.

Fix this by converting the helpers to take the full xattr name instead
of just the suffix and moving the reconstruction to the xattr handlers.
We now need to check if the prefix is correct in the helpers, but it
saves us the difficulty of reconstructing the full name from just the
plain suffix.

Reported-by: kernel test robot <rong.a.chen@intel.com>
Fixes: b230d5aba2d1 ("LSM: add new hook for kernfs node initialization")
Fixes: ec882da5cda9 ("selinux: implement the kernfs_init_security hook")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 fs/kernfs/inode.c        | 38 ++++++++++++++++++--------------------
 include/linux/kernfs.h   |  8 ++++----
 security/selinux/hooks.c |  6 +++---
 3 files changed, 25 insertions(+), 27 deletions(-)

diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
index 673ef598d97d..1daa8aa9ec96 100644
--- a/fs/kernfs/inode.c
+++ b/fs/kernfs/inode.c
@@ -288,28 +288,20 @@ int kernfs_iop_permission(struct inode *inode, int mask)
 	return generic_permission(inode, mask);
 }
 
-static int kernfs_node_xattr_get(const struct xattr_handler *handler,
-				 struct kernfs_node *kn, const char *suffix,
+static int kernfs_node_xattr_get(struct kernfs_node *kn, const char *name,
 				 void *value, size_t size)
 {
-	const char *name = xattr_full_name(handler, suffix);
-	struct kernfs_iattrs *attrs;
-
-	attrs = kernfs_iattrs_noalloc(kn);
+	struct kernfs_iattrs *attrs = kernfs_iattrs_noalloc(kn);
 	if (!attrs)
 		return -ENODATA;
 
 	return simple_xattr_get(&attrs->xattrs, name, value, size);
 }
 
-static int kernfs_node_xattr_set(const struct xattr_handler *handler,
-				 struct kernfs_node *kn, const char *suffix,
+static int kernfs_node_xattr_set(struct kernfs_node *kn, const char *name,
 				 const void *value, size_t size, int flags)
 {
-	const char *name = xattr_full_name(handler, suffix);
-	struct kernfs_iattrs *attrs;
-
-	attrs = kernfs_iattrs(kn);
+	struct kernfs_iattrs *attrs = kernfs_iattrs(kn);
 	if (!attrs)
 		return -ENOMEM;
 
@@ -320,9 +312,10 @@ static int kernfs_xattr_get(const struct xattr_handler *handler,
 			    struct dentry *unused, struct inode *inode,
 			    const char *suffix, void *value, size_t size)
 {
+	const char *name = xattr_full_name(handler, suffix);
 	struct kernfs_node *kn = inode->i_private;
 
-	return kernfs_node_xattr_get(handler, kn, suffix, value, size);
+	return kernfs_node_xattr_get(kn, name, value, size);
 }
 
 static int kernfs_xattr_set(const struct xattr_handler *handler,
@@ -330,9 +323,10 @@ static int kernfs_xattr_set(const struct xattr_handler *handler,
 			    const char *suffix, const void *value,
 			    size_t size, int flags)
 {
+	const char *name = xattr_full_name(handler, suffix);
 	struct kernfs_node *kn = inode->i_private;
 
-	return kernfs_node_xattr_set(handler, kn, suffix, value, size, flags);
+	return kernfs_node_xattr_set(kn, name, value, size, flags);
 }
 
 static const struct xattr_handler kernfs_trusted_xattr_handler = {
@@ -353,16 +347,20 @@ const struct xattr_handler *kernfs_xattr_handlers[] = {
 	NULL
 };
 
-int kernfs_security_xattr_get(struct kernfs_node *kn, const char *suffix,
+int kernfs_security_xattr_get(struct kernfs_node *kn, const char *name,
 			      void *value, size_t size)
 {
-	return kernfs_node_xattr_get(&kernfs_security_xattr_handler,
-				     kn, suffix, value, size);
+	if (WARN_ON_ONCE(!strstarts(name, XATTR_SECURITY_PREFIX)))
+		return -EINVAL;
+
+	return kernfs_node_xattr_get(kn, name, value, size);
 }
 
-int kernfs_security_xattr_set(struct kernfs_node *kn, const char *suffix,
+int kernfs_security_xattr_set(struct kernfs_node *kn, const char *name,
 			      void *value, size_t size, int flags)
 {
-	return kernfs_node_xattr_set(&kernfs_security_xattr_handler,
-				     kn, suffix, value, size, flags);
+	if (WARN_ON_ONCE(!strstarts(name, XATTR_SECURITY_PREFIX)))
+		return -EINVAL;
+
+	return kernfs_node_xattr_set(kn, name, value, size, flags);
 }
diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h
index 39eea07c2900..196a98cf39ed 100644
--- a/include/linux/kernfs.h
+++ b/include/linux/kernfs.h
@@ -371,9 +371,9 @@ __poll_t kernfs_generic_poll(struct kernfs_open_file *of,
 			     struct poll_table_struct *pt);
 void kernfs_notify(struct kernfs_node *kn);
 
-int kernfs_security_xattr_get(struct kernfs_node *kn, const char *suffix,
+int kernfs_security_xattr_get(struct kernfs_node *kn, const char *name,
 			      void *value, size_t size);
-int kernfs_security_xattr_set(struct kernfs_node *kn, const char *suffix,
+int kernfs_security_xattr_set(struct kernfs_node *kn, const char *name,
 			      void *value, size_t size, int flags);
 
 const void *kernfs_super_ns(struct super_block *sb);
@@ -479,12 +479,12 @@ static inline int kernfs_setattr(struct kernfs_node *kn,
 static inline void kernfs_notify(struct kernfs_node *kn) { }
 
 static inline int kernfs_security_xattr_get(struct kernfs_node *kn,
-					    const char *suffix, void *value,
+					    const char *name, void *value,
 					    size_t size)
 { return -ENOSYS; }
 
 static inline int kernfs_security_xattr_set(struct kernfs_node *kn,
-					    const char *suffix, void *value,
+					    const char *name, void *value,
 					    size_t size, int flags)
 { return -ENOSYS; }
 
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index ab4b049daf17..716ae37d4834 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3394,7 +3394,7 @@ int selinux_kernfs_init_security(struct kernfs_node *kn_dir,
 	int rc;
 	char *context;
 
-	rc = kernfs_security_xattr_get(kn_dir, XATTR_SELINUX_SUFFIX, NULL, 0);
+	rc = kernfs_security_xattr_get(kn_dir, XATTR_NAME_SELINUX, NULL, 0);
 	if (rc == -ENODATA)
 		return 0;
 	else if (rc < 0)
@@ -3405,7 +3405,7 @@ int selinux_kernfs_init_security(struct kernfs_node *kn_dir,
 	if (!context)
 		return -ENOMEM;
 
-	rc = kernfs_security_xattr_get(kn_dir, XATTR_SELINUX_SUFFIX, context,
+	rc = kernfs_security_xattr_get(kn_dir, XATTR_NAME_SELINUX, context,
 				       clen);
 	if (rc < 0) {
 		kfree(context);
@@ -3439,7 +3439,7 @@ int selinux_kernfs_init_security(struct kernfs_node *kn_dir,
 	if (rc)
 		return rc;
 
-	rc = kernfs_security_xattr_set(kn, XATTR_SELINUX_SUFFIX, context, clen,
+	rc = kernfs_security_xattr_set(kn, XATTR_NAME_SELINUX, context, clen,
 				       XATTR_CREATE);
 	kfree(context);
 	return rc;
-- 
2.20.1

[PATCH v2] kernfs: fix xattr name handling in LSM helpers

[Ondrej Mosnacek]

The implementation of kernfs_security_xattr_*() helpers reuses the
kernfs_node_xattr_*() functions, which take the suffix of the xattr name
and extract full xattr name from it using xattr_full_name(). However,
this function relies on the fact that the suffix passed to xattr
handlers from VFS is always constructed from the full name by just
incerementing the pointer. This doesn't necessarily hold for the callers
of kernfs_security_xattr_*(), so their usage will easily lead to
out-of-bounds access.

Fix this by converting the helpers to take the full xattr name instead
of just the suffix and moving the reconstruction to the xattr handlers.
We now need to check if the prefix is correct in the helpers, but it
saves us the difficulty of reconstructing the full name from just the
plain suffix.

Reported-by: kernel test robot <rong.a.chen@intel.com>
Fixes: b230d5aba2d1 ("LSM: add new hook for kernfs node initialization")
Fixes: ec882da5cda9 ("selinux: implement the kernfs_init_security hook")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---

v2: Rebase on current selinux/next.

 fs/kernfs/inode.c        | 38 ++++++++++++++++++--------------------
 include/linux/kernfs.h   |  8 ++++----
 security/selinux/hooks.c |  6 +++---
 3 files changed, 25 insertions(+), 27 deletions(-)

diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
index 673ef598d97d..1daa8aa9ec96 100644
--- a/fs/kernfs/inode.c
+++ b/fs/kernfs/inode.c
@@ -288,28 +288,20 @@ int kernfs_iop_permission(struct inode *inode, int mask)
 	return generic_permission(inode, mask);
 }
 
-static int kernfs_node_xattr_get(const struct xattr_handler *handler,
-				 struct kernfs_node *kn, const char *suffix,
+static int kernfs_node_xattr_get(struct kernfs_node *kn, const char *name,
 				 void *value, size_t size)
 {
-	const char *name = xattr_full_name(handler, suffix);
-	struct kernfs_iattrs *attrs;
-
-	attrs = kernfs_iattrs_noalloc(kn);
+	struct kernfs_iattrs *attrs = kernfs_iattrs_noalloc(kn);
 	if (!attrs)
 		return -ENODATA;
 
 	return simple_xattr_get(&attrs->xattrs, name, value, size);
 }
 
-static int kernfs_node_xattr_set(const struct xattr_handler *handler,
-				 struct kernfs_node *kn, const char *suffix,
+static int kernfs_node_xattr_set(struct kernfs_node *kn, const char *name,
 				 const void *value, size_t size, int flags)
 {
-	const char *name = xattr_full_name(handler, suffix);
-	struct kernfs_iattrs *attrs;
-
-	attrs = kernfs_iattrs(kn);
+	struct kernfs_iattrs *attrs = kernfs_iattrs(kn);
 	if (!attrs)
 		return -ENOMEM;
 
@@ -320,9 +312,10 @@ static int kernfs_xattr_get(const struct xattr_handler *handler,
 			    struct dentry *unused, struct inode *inode,
 			    const char *suffix, void *value, size_t size)
 {
+	const char *name = xattr_full_name(handler, suffix);
 	struct kernfs_node *kn = inode->i_private;
 
-	return kernfs_node_xattr_get(handler, kn, suffix, value, size);
+	return kernfs_node_xattr_get(kn, name, value, size);
 }
 
 static int kernfs_xattr_set(const struct xattr_handler *handler,
@@ -330,9 +323,10 @@ static int kernfs_xattr_set(const struct xattr_handler *handler,
 			    const char *suffix, const void *value,
 			    size_t size, int flags)
 {
+	const char *name = xattr_full_name(handler, suffix);
 	struct kernfs_node *kn = inode->i_private;
 
-	return kernfs_node_xattr_set(handler, kn, suffix, value, size, flags);
+	return kernfs_node_xattr_set(kn, name, value, size, flags);
 }
 
 static const struct xattr_handler kernfs_trusted_xattr_handler = {
@@ -353,16 +347,20 @@ const struct xattr_handler *kernfs_xattr_handlers[] = {
 	NULL
 };
 
-int kernfs_security_xattr_get(struct kernfs_node *kn, const char *suffix,
+int kernfs_security_xattr_get(struct kernfs_node *kn, const char *name,
 			      void *value, size_t size)
 {
-	return kernfs_node_xattr_get(&kernfs_security_xattr_handler,
-				     kn, suffix, value, size);
+	if (WARN_ON_ONCE(!strstarts(name, XATTR_SECURITY_PREFIX)))
+		return -EINVAL;
+
+	return kernfs_node_xattr_get(kn, name, value, size);
 }
 
-int kernfs_security_xattr_set(struct kernfs_node *kn, const char *suffix,
+int kernfs_security_xattr_set(struct kernfs_node *kn, const char *name,
 			      void *value, size_t size, int flags)
 {
-	return kernfs_node_xattr_set(&kernfs_security_xattr_handler,
-				     kn, suffix, value, size, flags);
+	if (WARN_ON_ONCE(!strstarts(name, XATTR_SECURITY_PREFIX)))
+		return -EINVAL;
+
+	return kernfs_node_xattr_set(kn, name, value, size, flags);
 }
diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h
index 39eea07c2900..196a98cf39ed 100644
--- a/include/linux/kernfs.h
+++ b/include/linux/kernfs.h
@@ -371,9 +371,9 @@ __poll_t kernfs_generic_poll(struct kernfs_open_file *of,
 			     struct poll_table_struct *pt);
 void kernfs_notify(struct kernfs_node *kn);
 
-int kernfs_security_xattr_get(struct kernfs_node *kn, const char *suffix,
+int kernfs_security_xattr_get(struct kernfs_node *kn, const char *name,
 			      void *value, size_t size);
-int kernfs_security_xattr_set(struct kernfs_node *kn, const char *suffix,
+int kernfs_security_xattr_set(struct kernfs_node *kn, const char *name,
 			      void *value, size_t size, int flags);
 
 const void *kernfs_super_ns(struct super_block *sb);
@@ -479,12 +479,12 @@ static inline int kernfs_setattr(struct kernfs_node *kn,
 static inline void kernfs_notify(struct kernfs_node *kn) { }
 
 static inline int kernfs_security_xattr_get(struct kernfs_node *kn,
-					    const char *suffix, void *value,
+					    const char *name, void *value,
 					    size_t size)
 { return -ENOSYS; }
 
 static inline int kernfs_security_xattr_set(struct kernfs_node *kn,
-					    const char *suffix, void *value,
+					    const char *name, void *value,
 					    size_t size, int flags)
 { return -ENOSYS; }
 
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index b6e61524d68d..43f1f244b7de 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3394,7 +3394,7 @@ static int selinux_kernfs_init_security(struct kernfs_node *kn_dir,
 	int rc;
 	char *context;
 
-	rc = kernfs_security_xattr_get(kn_dir, XATTR_SELINUX_SUFFIX, NULL, 0);
+	rc = kernfs_security_xattr_get(kn_dir, XATTR_NAME_SELINUX, NULL, 0);
 	if (rc == -ENODATA)
 		return 0;
 	else if (rc < 0)
@@ -3405,7 +3405,7 @@ static int selinux_kernfs_init_security(struct kernfs_node *kn_dir,
 	if (!context)
 		return -ENOMEM;
 
-	rc = kernfs_security_xattr_get(kn_dir, XATTR_SELINUX_SUFFIX, context,
+	rc = kernfs_security_xattr_get(kn_dir, XATTR_NAME_SELINUX, context,
 				       clen);
 	if (rc < 0) {
 		kfree(context);
@@ -3439,7 +3439,7 @@ static int selinux_kernfs_init_security(struct kernfs_node *kn_dir,
 	if (rc)
 		return rc;
 
-	rc = kernfs_security_xattr_set(kn, XATTR_SELINUX_SUFFIX, context, clen,
+	rc = kernfs_security_xattr_set(kn, XATTR_NAME_SELINUX, context, clen,
 				       XATTR_CREATE);
 	kfree(context);
 	return rc;
-- 
2.20.1

Re: [kernfs] e19dfdc83b: BUG:KASAN:global-out-of-bounds_in_s

[Ondrej Mosnacek]

On Mon, Mar 25, 2019 at 6:06 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Mon, Mar 25, 2019 at 4:17 PM Paul Moore <paul@paul-moore.com> wrote:
> > Ondrej, please look into this.
> >
> > You've looked at this code more recently than I have, but it looks
> > like there might be an issue with __kernfs_iattrs() returning a
> > pointer to a kernfs_iattrs object without taking a kernfs reference
> > (kernfs_get(kn)).  Although I would be a little surprised if this was
> > the problem as I think it would cause a number of issues beyond just
> > this one ... ?
>
> I think this is actually because of how xattr_full_name() reconstructs
> the full name from the xattr suffix. It assumes that the suffix was
> obtained from the full name by just taking a pointer inside it, but in
> kernfs_security_xattr_get/set() I pass the suffix directly... I'm
> surprised that this didn't fail spectacularly earlier during testing.
> Maybe the newer GCC does some clever merging of the string constants,
> so that XATTR_SELINUX_SUFFIX actually ends up as a substring of
> XATTR_NAME_SELINUX? (That would be one hell of a "lucky" coincidence
> :)
>
> I'll post a patch that converts kernfs_security_xattr_get/set() to
> take the full name and hopefully that will fix the problem. I'll see
> if I can run the reproducer locally tomorrow...

I managed to reproduce the KASAN warning in my kernel testing
environment by simply enabling CONFIG_KASAN and running the cgroupfs
issue reproducer from the original patchset. With the patch I posted I
no longer get the warning, so I believe it really fixes the problem.

--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.

Re: [PATCH v2] kernfs: fix xattr name handling in LSM helpers

[Paul Moore]

On Tue, Mar 26, 2019 at 8:12 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> The implementation of kernfs_security_xattr_*() helpers reuses the
> kernfs_node_xattr_*() functions, which take the suffix of the xattr name
> and extract full xattr name from it using xattr_full_name(). However,
> this function relies on the fact that the suffix passed to xattr
> handlers from VFS is always constructed from the full name by just
> incerementing the pointer. This doesn't necessarily hold for the callers
> of kernfs_security_xattr_*(), so their usage will easily lead to
> out-of-bounds access.
>
> Fix this by converting the helpers to take the full xattr name instead
> of just the suffix and moving the reconstruction to the xattr handlers.
> We now need to check if the prefix is correct in the helpers, but it
> saves us the difficulty of reconstructing the full name from just the
> plain suffix.
>
> Reported-by: kernel test robot <rong.a.chen@intel.com>
> Fixes: b230d5aba2d1 ("LSM: add new hook for kernfs node initialization")
> Fixes: ec882da5cda9 ("selinux: implement the kernfs_init_security hook")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>
> v2: Rebase on current selinux/next.
>
>  fs/kernfs/inode.c        | 38 ++++++++++++++++++--------------------
>  include/linux/kernfs.h   |  8 ++++----
>  security/selinux/hooks.c |  6 +++---
>  3 files changed, 25 insertions(+), 27 deletions(-)

Thanks for diagnosing this and providing a patch.  I haven't seen any
objections, but I do have some questions (below).

> diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
> index 673ef598d97d..1daa8aa9ec96 100644
> --- a/fs/kernfs/inode.c
> +++ b/fs/kernfs/inode.c
> @@ -288,28 +288,20 @@ int kernfs_iop_permission(struct inode *inode, int mask)
>         return generic_permission(inode, mask);
>  }
>
> -static int kernfs_node_xattr_get(const struct xattr_handler *handler,
> -                                struct kernfs_node *kn, const char *suffix,
> +static int kernfs_node_xattr_get(struct kernfs_node *kn, const char *name,
>                                  void *value, size_t size)
>  {
> -       const char *name = xattr_full_name(handler, suffix);
> -       struct kernfs_iattrs *attrs;
> -
> -       attrs = kernfs_iattrs_noalloc(kn);
> +       struct kernfs_iattrs *attrs = kernfs_iattrs_noalloc(kn);
>         if (!attrs)
>                 return -ENODATA;
>
>         return simple_xattr_get(&attrs->xattrs, name, value, size);
>  }
>
> -static int kernfs_node_xattr_set(const struct xattr_handler *handler,
> -                                struct kernfs_node *kn, const char *suffix,
> +static int kernfs_node_xattr_set(struct kernfs_node *kn, const char *name,
>                                  const void *value, size_t size, int flags)
>  {
> -       const char *name = xattr_full_name(handler, suffix);
> -       struct kernfs_iattrs *attrs;
> -
> -       attrs = kernfs_iattrs(kn);
> +       struct kernfs_iattrs *attrs = kernfs_iattrs(kn);
>         if (!attrs)
>                 return -ENOMEM;
>

...

> -int kernfs_security_xattr_get(struct kernfs_node *kn, const char *suffix,
> +int kernfs_security_xattr_get(struct kernfs_node *kn, const char *name,
>                               void *value, size_t size)
>  {
> -       return kernfs_node_xattr_get(&kernfs_security_xattr_handler,
> -                                    kn, suffix, value, size);
> +       if (WARN_ON_ONCE(!strstarts(name, XATTR_SECURITY_PREFIX)))
> +               return -EINVAL;
> +
> +       return kernfs_node_xattr_get(kn, name, value, size);
>  }
>
> -int kernfs_security_xattr_set(struct kernfs_node *kn, const char *suffix,
> +int kernfs_security_xattr_set(struct kernfs_node *kn, const char *name,
>                               void *value, size_t size, int flags)
>  {
> -       return kernfs_node_xattr_set(&kernfs_security_xattr_handler,
> -                                    kn, suffix, value, size, flags);
> +       if (WARN_ON_ONCE(!strstarts(name, XATTR_SECURITY_PREFIX)))
> +               return -EINVAL;
> +
> +       return kernfs_node_xattr_set(kn, name, value, size, flags);
>  }

I think it is reasonable to ask if we even need
kernfs_security_xattr_{set|get}()?  Can we just call the respective
kernfs_node_xattr*() functions instead?  I can't imagine the
WARN_ON_ONCE check being that important.

-- 
paul moore
www.paul-moore.com

Re: [PATCH v2] kernfs: fix xattr name handling in LSM helpers

[Ondrej Mosnacek]

On Fri, Mar 29, 2019 at 2:31 PM Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Mar 26, 2019 at 8:12 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > The implementation of kernfs_security_xattr_*() helpers reuses the
> > kernfs_node_xattr_*() functions, which take the suffix of the xattr name
> > and extract full xattr name from it using xattr_full_name(). However,
> > this function relies on the fact that the suffix passed to xattr
> > handlers from VFS is always constructed from the full name by just
> > incerementing the pointer. This doesn't necessarily hold for the callers
> > of kernfs_security_xattr_*(), so their usage will easily lead to
> > out-of-bounds access.
> >
> > Fix this by converting the helpers to take the full xattr name instead
> > of just the suffix and moving the reconstruction to the xattr handlers.
> > We now need to check if the prefix is correct in the helpers, but it
> > saves us the difficulty of reconstructing the full name from just the
> > plain suffix.
> >
> > Reported-by: kernel test robot <rong.a.chen@intel.com>
> > Fixes: b230d5aba2d1 ("LSM: add new hook for kernfs node initialization")
> > Fixes: ec882da5cda9 ("selinux: implement the kernfs_init_security hook")
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >
> > v2: Rebase on current selinux/next.
> >
> >  fs/kernfs/inode.c        | 38 ++++++++++++++++++--------------------
> >  include/linux/kernfs.h   |  8 ++++----
> >  security/selinux/hooks.c |  6 +++---
> >  3 files changed, 25 insertions(+), 27 deletions(-)
>
> Thanks for diagnosing this and providing a patch.  I haven't seen any
> objections, but I do have some questions (below).
>
> > diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
> > index 673ef598d97d..1daa8aa9ec96 100644
> > --- a/fs/kernfs/inode.c
> > +++ b/fs/kernfs/inode.c
> > @@ -288,28 +288,20 @@ int kernfs_iop_permission(struct inode *inode, int mask)
> >         return generic_permission(inode, mask);
> >  }
> >
> > -static int kernfs_node_xattr_get(const struct xattr_handler *handler,
> > -                                struct kernfs_node *kn, const char *suffix,
> > +static int kernfs_node_xattr_get(struct kernfs_node *kn, const char *name,
> >                                  void *value, size_t size)
> >  {
> > -       const char *name = xattr_full_name(handler, suffix);
> > -       struct kernfs_iattrs *attrs;
> > -
> > -       attrs = kernfs_iattrs_noalloc(kn);
> > +       struct kernfs_iattrs *attrs = kernfs_iattrs_noalloc(kn);
> >         if (!attrs)
> >                 return -ENODATA;
> >
> >         return simple_xattr_get(&attrs->xattrs, name, value, size);
> >  }
> >
> > -static int kernfs_node_xattr_set(const struct xattr_handler *handler,
> > -                                struct kernfs_node *kn, const char *suffix,
> > +static int kernfs_node_xattr_set(struct kernfs_node *kn, const char *name,
> >                                  const void *value, size_t size, int flags)
> >  {
> > -       const char *name = xattr_full_name(handler, suffix);
> > -       struct kernfs_iattrs *attrs;
> > -
> > -       attrs = kernfs_iattrs(kn);
> > +       struct kernfs_iattrs *attrs = kernfs_iattrs(kn);
> >         if (!attrs)
> >                 return -ENOMEM;
> >
>
> ...
>
> > -int kernfs_security_xattr_get(struct kernfs_node *kn, const char *suffix,
> > +int kernfs_security_xattr_get(struct kernfs_node *kn, const char *name,
> >                               void *value, size_t size)
> >  {
> > -       return kernfs_node_xattr_get(&kernfs_security_xattr_handler,
> > -                                    kn, suffix, value, size);
> > +       if (WARN_ON_ONCE(!strstarts(name, XATTR_SECURITY_PREFIX)))
> > +               return -EINVAL;
> > +
> > +       return kernfs_node_xattr_get(kn, name, value, size);
> >  }
> >
> > -int kernfs_security_xattr_set(struct kernfs_node *kn, const char *suffix,
> > +int kernfs_security_xattr_set(struct kernfs_node *kn, const char *name,
> >                               void *value, size_t size, int flags)
> >  {
> > -       return kernfs_node_xattr_set(&kernfs_security_xattr_handler,
> > -                                    kn, suffix, value, size, flags);
> > +       if (WARN_ON_ONCE(!strstarts(name, XATTR_SECURITY_PREFIX)))
> > +               return -EINVAL;
> > +
> > +       return kernfs_node_xattr_set(kn, name, value, size, flags);
> >  }
>
> I think it is reasonable to ask if we even need
> kernfs_security_xattr_{set|get}()?  Can we just call the respective
> kernfs_node_xattr*() functions instead?  I can't imagine the
> WARN_ON_ONCE check being that important.

Indeed, it is now much more natural to just expose all xattrs in those
helpers... I concur that the encapsulation doesn't seem to be worth it
any more. Let me do a simplified respin...


--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.

[PATCH v3] kernfs: fix xattr name handling in LSM helpers

[Ondrej Mosnacek]

The implementation of kernfs_security_xattr_*() helpers reuses the
kernfs_node_xattr_*() functions, which take the suffix of the xattr name
and extract full xattr name from it using xattr_full_name(). However,
this function relies on the fact that the suffix passed to xattr
handlers from VFS is always constructed from the full name by just
incerementing the pointer. This doesn't necessarily hold for the callers
of kernfs_security_xattr_*(), so their usage will easily lead to
out-of-bounds access.

Fix this by moving the xattr name reconstruction to the VFS xattr
handlers and replacing the kernfs_security_xattr_*() helpers with more
general kernfs_xattr_*() helpers that take full xattr name and allow
accessing all kernfs node's xattrs.

Reported-by: kernel test robot <rong.a.chen@intel.com>
Fixes: b230d5aba2d1 ("LSM: add new hook for kernfs node initialization")
Fixes: ec882da5cda9 ("selinux: implement the kernfs_init_security hook")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---

v3: simplify kernfs xattr helpers as per Paul's suggestion
v2: just rebase to update diff context

 fs/kernfs/inode.c        | 62 ++++++++++++++--------------------------
 include/linux/kernfs.h   | 18 ++++++------
 security/selinux/hooks.c |  9 +++---
 3 files changed, 33 insertions(+), 56 deletions(-)

diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
index 673ef598d97d..1e93fe8c67f4 100644
--- a/fs/kernfs/inode.c
+++ b/fs/kernfs/inode.c
@@ -288,63 +288,57 @@ int kernfs_iop_permission(struct inode *inode, int mask)
 	return generic_permission(inode, mask);
 }
 
-static int kernfs_node_xattr_get(const struct xattr_handler *handler,
-				 struct kernfs_node *kn, const char *suffix,
-				 void *value, size_t size)
+int kernfs_node_xattr_get(struct kernfs_node *kn, const char *name,
+			  void *value, size_t size)
 {
-	const char *name = xattr_full_name(handler, suffix);
-	struct kernfs_iattrs *attrs;
-
-	attrs = kernfs_iattrs_noalloc(kn);
+	struct kernfs_iattrs *attrs = kernfs_iattrs_noalloc(kn);
 	if (!attrs)
 		return -ENODATA;
 
 	return simple_xattr_get(&attrs->xattrs, name, value, size);
 }
 
-static int kernfs_node_xattr_set(const struct xattr_handler *handler,
-				 struct kernfs_node *kn, const char *suffix,
-				 const void *value, size_t size, int flags)
+int kernfs_node_xattr_set(struct kernfs_node *kn, const char *name,
+			  const void *value, size_t size, int flags)
 {
-	const char *name = xattr_full_name(handler, suffix);
-	struct kernfs_iattrs *attrs;
-
-	attrs = kernfs_iattrs(kn);
+	struct kernfs_iattrs *attrs = kernfs_iattrs(kn);
 	if (!attrs)
 		return -ENOMEM;
 
 	return simple_xattr_set(&attrs->xattrs, name, value, size, flags);
 }
 
-static int kernfs_xattr_get(const struct xattr_handler *handler,
-			    struct dentry *unused, struct inode *inode,
-			    const char *suffix, void *value, size_t size)
+static int kernfs_vfs_xattr_get(const struct xattr_handler *handler,
+				struct dentry *unused, struct inode *inode,
+				const char *suffix, void *value, size_t size)
 {
+	const char *name = xattr_full_name(handler, suffix);
 	struct kernfs_node *kn = inode->i_private;
 
-	return kernfs_node_xattr_get(handler, kn, suffix, value, size);
+	return kernfs_node_xattr_get(kn, name, value, size);
 }
 
-static int kernfs_xattr_set(const struct xattr_handler *handler,
-			    struct dentry *unused, struct inode *inode,
-			    const char *suffix, const void *value,
-			    size_t size, int flags)
+static int kernfs_vfs_xattr_set(const struct xattr_handler *handler,
+				struct dentry *unused, struct inode *inode,
+				const char *suffix, const void *value,
+				size_t size, int flags)
 {
+	const char *name = xattr_full_name(handler, suffix);
 	struct kernfs_node *kn = inode->i_private;
 
-	return kernfs_node_xattr_set(handler, kn, suffix, value, size, flags);
+	return kernfs_node_xattr_set(kn, name, value, size, flags);
 }
 
 static const struct xattr_handler kernfs_trusted_xattr_handler = {
 	.prefix = XATTR_TRUSTED_PREFIX,
-	.get = kernfs_xattr_get,
-	.set = kernfs_xattr_set,
+	.get = kernfs_vfs_xattr_get,
+	.set = kernfs_vfs_xattr_set,
 };
 
 static const struct xattr_handler kernfs_security_xattr_handler = {
 	.prefix = XATTR_SECURITY_PREFIX,
-	.get = kernfs_xattr_get,
-	.set = kernfs_xattr_set,
+	.get = kernfs_vfs_xattr_get,
+	.set = kernfs_vfs_xattr_set,
 };
 
 const struct xattr_handler *kernfs_xattr_handlers[] = {
@@ -352,17 +346,3 @@ const struct xattr_handler *kernfs_xattr_handlers[] = {
 	&kernfs_security_xattr_handler,
 	NULL
 };
-
-int kernfs_security_xattr_get(struct kernfs_node *kn, const char *suffix,
-			      void *value, size_t size)
-{
-	return kernfs_node_xattr_get(&kernfs_security_xattr_handler,
-				     kn, suffix, value, size);
-}
-
-int kernfs_security_xattr_set(struct kernfs_node *kn, const char *suffix,
-			      void *value, size_t size, int flags)
-{
-	return kernfs_node_xattr_set(&kernfs_security_xattr_handler,
-				     kn, suffix, value, size, flags);
-}
diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h
index 39eea07c2900..b3f62e7bb996 100644
--- a/include/linux/kernfs.h
+++ b/include/linux/kernfs.h
@@ -371,10 +371,10 @@ __poll_t kernfs_generic_poll(struct kernfs_open_file *of,
 			     struct poll_table_struct *pt);
 void kernfs_notify(struct kernfs_node *kn);
 
-int kernfs_security_xattr_get(struct kernfs_node *kn, const char *suffix,
-			      void *value, size_t size);
-int kernfs_security_xattr_set(struct kernfs_node *kn, const char *suffix,
-			      void *value, size_t size, int flags);
+int kernfs_xattr_get(struct kernfs_node *kn, const char *name,
+		     void *value, size_t size);
+int kernfs_xattr_set(struct kernfs_node *kn, const char *name,
+		     void *value, size_t size, int flags);
 
 const void *kernfs_super_ns(struct super_block *sb);
 int kernfs_get_tree(struct fs_context *fc);
@@ -478,14 +478,12 @@ static inline int kernfs_setattr(struct kernfs_node *kn,
 
 static inline void kernfs_notify(struct kernfs_node *kn) { }
 
-static inline int kernfs_security_xattr_get(struct kernfs_node *kn,
-					    const char *suffix, void *value,
-					    size_t size)
+static inline int kernfs_xattr_get(struct kernfs_node *kn, const char *name,
+				   void *value, size_t size)
 { return -ENOSYS; }
 
-static inline int kernfs_security_xattr_set(struct kernfs_node *kn,
-					    const char *suffix, void *value,
-					    size_t size, int flags)
+static inline int kernfs_xattr_set(struct kernfs_node *kn, const char *name,
+				   void *value, size_t size, int flags)
 { return -ENOSYS; }
 
 static inline const void *kernfs_super_ns(struct super_block *sb)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index b6e61524d68d..d5fdcb0d26fe 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3394,7 +3394,7 @@ static int selinux_kernfs_init_security(struct kernfs_node *kn_dir,
 	int rc;
 	char *context;
 
-	rc = kernfs_security_xattr_get(kn_dir, XATTR_SELINUX_SUFFIX, NULL, 0);
+	rc = kernfs_xattr_get(kn_dir, XATTR_NAME_SELINUX, NULL, 0);
 	if (rc == -ENODATA)
 		return 0;
 	else if (rc < 0)
@@ -3405,8 +3405,7 @@ static int selinux_kernfs_init_security(struct kernfs_node *kn_dir,
 	if (!context)
 		return -ENOMEM;
 
-	rc = kernfs_security_xattr_get(kn_dir, XATTR_SELINUX_SUFFIX, context,
-				       clen);
+	rc = kernfs_xattr_get(kn_dir, XATTR_NAME_SELINUX, context, clen);
 	if (rc < 0) {
 		kfree(context);
 		return rc;
@@ -3439,8 +3438,8 @@ static int selinux_kernfs_init_security(struct kernfs_node *kn_dir,
 	if (rc)
 		return rc;
 
-	rc = kernfs_security_xattr_set(kn, XATTR_SELINUX_SUFFIX, context, clen,
-				       XATTR_CREATE);
+	rc = kernfs_xattr_set(kn, XATTR_NAME_SELINUX, context, clen,
+			      XATTR_CREATE);
 	kfree(context);
 	return rc;
 }
-- 
2.20.1

Re: [PATCH v3] kernfs: fix xattr name handling in LSM helpers

[Paul Moore]

On Mon, Apr 1, 2019 at 6:34 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> The implementation of kernfs_security_xattr_*() helpers reuses the
> kernfs_node_xattr_*() functions, which take the suffix of the xattr name
> and extract full xattr name from it using xattr_full_name(). However,
> this function relies on the fact that the suffix passed to xattr
> handlers from VFS is always constructed from the full name by just
> incerementing the pointer. This doesn't necessarily hold for the callers
> of kernfs_security_xattr_*(), so their usage will easily lead to
> out-of-bounds access.
>
> Fix this by moving the xattr name reconstruction to the VFS xattr
> handlers and replacing the kernfs_security_xattr_*() helpers with more
> general kernfs_xattr_*() helpers that take full xattr name and allow
> accessing all kernfs node's xattrs.
>
> Reported-by: kernel test robot <rong.a.chen@intel.com>
> Fixes: b230d5aba2d1 ("LSM: add new hook for kernfs node initialization")
> Fixes: ec882da5cda9 ("selinux: implement the kernfs_init_security hook")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>
> v3: simplify kernfs xattr helpers as per Paul's suggestion
> v2: just rebase to update diff context
>
>  fs/kernfs/inode.c        | 62 ++++++++++++++--------------------------
>  include/linux/kernfs.h   | 18 ++++++------
>  security/selinux/hooks.c |  9 +++---
>  3 files changed, 33 insertions(+), 56 deletions(-)

This is better, thanks.  Merged into selinux/next.

-- 
paul moore
www.paul-moore.com

Re: [PATCH v3] kernfs: fix xattr name handling in LSM helpers

[Paul Moore]

On Tue, Apr 2, 2019 at 7:10 PM Paul Moore <paul@paul-moore.com> wrote:
> On Mon, Apr 1, 2019 at 6:34 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > The implementation of kernfs_security_xattr_*() helpers reuses the
> > kernfs_node_xattr_*() functions, which take the suffix of the xattr name
> > and extract full xattr name from it using xattr_full_name(). However,
> > this function relies on the fact that the suffix passed to xattr
> > handlers from VFS is always constructed from the full name by just
> > incerementing the pointer. This doesn't necessarily hold for the callers
> > of kernfs_security_xattr_*(), so their usage will easily lead to
> > out-of-bounds access.
> >
> > Fix this by moving the xattr name reconstruction to the VFS xattr
> > handlers and replacing the kernfs_security_xattr_*() helpers with more
> > general kernfs_xattr_*() helpers that take full xattr name and allow
> > accessing all kernfs node's xattrs.
> >
> > Reported-by: kernel test robot <rong.a.chen@intel.com>
> > Fixes: b230d5aba2d1 ("LSM: add new hook for kernfs node initialization")
> > Fixes: ec882da5cda9 ("selinux: implement the kernfs_init_security hook")
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >
> > v3: simplify kernfs xattr helpers as per Paul's suggestion
> > v2: just rebase to update diff context
> >
> >  fs/kernfs/inode.c        | 62 ++++++++++++++--------------------------
> >  include/linux/kernfs.h   | 18 ++++++------
> >  security/selinux/hooks.c |  9 +++---
> >  3 files changed, 33 insertions(+), 56 deletions(-)
>
> This is better, thanks.  Merged into selinux/next.

... and I've just popped it off selinux/next.  It looks like you need
to export the kernfs functions.

ld: security/selinux/hooks.o: in function `selinux_kernfs_init_security':
/pjob.data/scratch/t/server-build.lan-2100-uHT0/BUILD/kernel-5.0.fc31/linux-5.1.
0-0.rc3.git1.1.2.secnext.fc31.x86_64/security/selinux/hooks.c:3397: undefined re
ference to `kernfs_xattr_get'
ld: /pjob.data/scratch/t/server-build.lan-2100-uHT0/BUILD/kernel-5.0.fc31/linux-
5.1.0-0.rc3.git1.1.2.secnext.fc31.x86_64/security/selinux/hooks.c:3408: undefine
d reference to `kernfs_xattr_get'
ld: /pjob.data/scratch/t/server-build.lan-2100-uHT0/BUILD/kernel-5.0.fc31/linux-
5.1.0-0.rc3.git1.1.2.secnext.fc31.x86_64/security/selinux/hooks.c:3441: undefine
d reference to `kernfs_xattr_set'
make: *** [Makefile:1033: vmlinux] Error 1

-- 
paul moore
www.paul-moore.com

Re: [PATCH v3] kernfs: fix xattr name handling in LSM helpers

[Ondrej Mosnacek]

On Wed, Apr 3, 2019 at 3:24 AM Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Apr 2, 2019 at 7:10 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Mon, Apr 1, 2019 at 6:34 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > The implementation of kernfs_security_xattr_*() helpers reuses the
> > > kernfs_node_xattr_*() functions, which take the suffix of the xattr name
> > > and extract full xattr name from it using xattr_full_name(). However,
> > > this function relies on the fact that the suffix passed to xattr
> > > handlers from VFS is always constructed from the full name by just
> > > incerementing the pointer. This doesn't necessarily hold for the callers
> > > of kernfs_security_xattr_*(), so their usage will easily lead to
> > > out-of-bounds access.
> > >
> > > Fix this by moving the xattr name reconstruction to the VFS xattr
> > > handlers and replacing the kernfs_security_xattr_*() helpers with more
> > > general kernfs_xattr_*() helpers that take full xattr name and allow
> > > accessing all kernfs node's xattrs.
> > >
> > > Reported-by: kernel test robot <rong.a.chen@intel.com>
> > > Fixes: b230d5aba2d1 ("LSM: add new hook for kernfs node initialization")
> > > Fixes: ec882da5cda9 ("selinux: implement the kernfs_init_security hook")
> > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > ---
> > >
> > > v3: simplify kernfs xattr helpers as per Paul's suggestion
> > > v2: just rebase to update diff context
> > >
> > >  fs/kernfs/inode.c        | 62 ++++++++++++++--------------------------
> > >  include/linux/kernfs.h   | 18 ++++++------
> > >  security/selinux/hooks.c |  9 +++---
> > >  3 files changed, 33 insertions(+), 56 deletions(-)
> >
> > This is better, thanks.  Merged into selinux/next.
>
> ... and I've just popped it off selinux/next.  It looks like you need
> to export the kernfs functions.
>
> ld: security/selinux/hooks.o: in function `selinux_kernfs_init_security':
> /pjob.data/scratch/t/server-build.lan-2100-uHT0/BUILD/kernel-5.0.fc31/linux-5.1.
> 0-0.rc3.git1.1.2.secnext.fc31.x86_64/security/selinux/hooks.c:3397: undefined re
> ference to `kernfs_xattr_get'
> ld: /pjob.data/scratch/t/server-build.lan-2100-uHT0/BUILD/kernel-5.0.fc31/linux-
> 5.1.0-0.rc3.git1.1.2.secnext.fc31.x86_64/security/selinux/hooks.c:3408: undefine
> d reference to `kernfs_xattr_get'
> ld: /pjob.data/scratch/t/server-build.lan-2100-uHT0/BUILD/kernel-5.0.fc31/linux-
> 5.1.0-0.rc3.git1.1.2.secnext.fc31.x86_64/security/selinux/hooks.c:3441: undefine
> d reference to `kernfs_xattr_set'
> make: *** [Makefile:1033: vmlinux] Error 1

Hm, I *thought* I ran the build before posting... The problem is that
the function names in kernfs.h and inode.c didn't match.
EXPORT_SYMBOL_GPL() shouldn't be needed since both kernfs and SELinux
are always built-in and there seems to be an existing trend in kernfs
to not export functions unless necessary.

I should have a fixed patch ready soon.

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.

[PATCH v4] kernfs: fix xattr name handling in LSM helpers

[Ondrej Mosnacek]

The implementation of kernfs_security_xattr_*() helpers reuses the
kernfs_node_xattr_*() functions, which take the suffix of the xattr name
and extract full xattr name from it using xattr_full_name(). However,
this function relies on the fact that the suffix passed to xattr
handlers from VFS is always constructed from the full name by just
incerementing the pointer. This doesn't necessarily hold for the callers
of kernfs_security_xattr_*(), so their usage will easily lead to
out-of-bounds access.

Fix this by moving the xattr name reconstruction to the VFS xattr
handlers and replacing the kernfs_security_xattr_*() helpers with more
general kernfs_xattr_*() helpers that take full xattr name and allow
accessing all kernfs node's xattrs.

Reported-by: kernel test robot <rong.a.chen@intel.com>
Fixes: b230d5aba2d1 ("LSM: add new hook for kernfs node initialization")
Fixes: ec882da5cda9 ("selinux: implement the kernfs_init_security hook")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---

v4: fix function names and types to make it build...
v3: simplify kernfs xattr helpers as per Paul's suggestion
v2: just rebase to update diff context

 fs/kernfs/inode.c        | 62 ++++++++++++++--------------------------
 include/linux/kernfs.h   | 18 ++++++------
 security/selinux/hooks.c |  9 +++---
 3 files changed, 33 insertions(+), 56 deletions(-)

diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
index 673ef598d97d..f89a0f13840e 100644
--- a/fs/kernfs/inode.c
+++ b/fs/kernfs/inode.c
@@ -288,63 +288,57 @@ int kernfs_iop_permission(struct inode *inode, int mask)
 	return generic_permission(inode, mask);
 }
 
-static int kernfs_node_xattr_get(const struct xattr_handler *handler,
-				 struct kernfs_node *kn, const char *suffix,
-				 void *value, size_t size)
+int kernfs_xattr_get(struct kernfs_node *kn, const char *name,
+		     void *value, size_t size)
 {
-	const char *name = xattr_full_name(handler, suffix);
-	struct kernfs_iattrs *attrs;
-
-	attrs = kernfs_iattrs_noalloc(kn);
+	struct kernfs_iattrs *attrs = kernfs_iattrs_noalloc(kn);
 	if (!attrs)
 		return -ENODATA;
 
 	return simple_xattr_get(&attrs->xattrs, name, value, size);
 }
 
-static int kernfs_node_xattr_set(const struct xattr_handler *handler,
-				 struct kernfs_node *kn, const char *suffix,
-				 const void *value, size_t size, int flags)
+int kernfs_xattr_set(struct kernfs_node *kn, const char *name,
+		     const void *value, size_t size, int flags)
 {
-	const char *name = xattr_full_name(handler, suffix);
-	struct kernfs_iattrs *attrs;
-
-	attrs = kernfs_iattrs(kn);
+	struct kernfs_iattrs *attrs = kernfs_iattrs(kn);
 	if (!attrs)
 		return -ENOMEM;
 
 	return simple_xattr_set(&attrs->xattrs, name, value, size, flags);
 }
 
-static int kernfs_xattr_get(const struct xattr_handler *handler,
-			    struct dentry *unused, struct inode *inode,
-			    const char *suffix, void *value, size_t size)
+static int kernfs_vfs_xattr_get(const struct xattr_handler *handler,
+				struct dentry *unused, struct inode *inode,
+				const char *suffix, void *value, size_t size)
 {
+	const char *name = xattr_full_name(handler, suffix);
 	struct kernfs_node *kn = inode->i_private;
 
-	return kernfs_node_xattr_get(handler, kn, suffix, value, size);
+	return kernfs_xattr_get(kn, name, value, size);
 }
 
-static int kernfs_xattr_set(const struct xattr_handler *handler,
-			    struct dentry *unused, struct inode *inode,
-			    const char *suffix, const void *value,
-			    size_t size, int flags)
+static int kernfs_vfs_xattr_set(const struct xattr_handler *handler,
+				struct dentry *unused, struct inode *inode,
+				const char *suffix, const void *value,
+				size_t size, int flags)
 {
+	const char *name = xattr_full_name(handler, suffix);
 	struct kernfs_node *kn = inode->i_private;
 
-	return kernfs_node_xattr_set(handler, kn, suffix, value, size, flags);
+	return kernfs_xattr_set(kn, name, value, size, flags);
 }
 
 static const struct xattr_handler kernfs_trusted_xattr_handler = {
 	.prefix = XATTR_TRUSTED_PREFIX,
-	.get = kernfs_xattr_get,
-	.set = kernfs_xattr_set,
+	.get = kernfs_vfs_xattr_get,
+	.set = kernfs_vfs_xattr_set,
 };
 
 static const struct xattr_handler kernfs_security_xattr_handler = {
 	.prefix = XATTR_SECURITY_PREFIX,
-	.get = kernfs_xattr_get,
-	.set = kernfs_xattr_set,
+	.get = kernfs_vfs_xattr_get,
+	.set = kernfs_vfs_xattr_set,
 };
 
 const struct xattr_handler *kernfs_xattr_handlers[] = {
@@ -352,17 +346,3 @@ const struct xattr_handler *kernfs_xattr_handlers[] = {
 	&kernfs_security_xattr_handler,
 	NULL
 };
-
-int kernfs_security_xattr_get(struct kernfs_node *kn, const char *suffix,
-			      void *value, size_t size)
-{
-	return kernfs_node_xattr_get(&kernfs_security_xattr_handler,
-				     kn, suffix, value, size);
-}
-
-int kernfs_security_xattr_set(struct kernfs_node *kn, const char *suffix,
-			      void *value, size_t size, int flags)
-{
-	return kernfs_node_xattr_set(&kernfs_security_xattr_handler,
-				     kn, suffix, value, size, flags);
-}
diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h
index 39eea07c2900..7987e0f89b69 100644
--- a/include/linux/kernfs.h
+++ b/include/linux/kernfs.h
@@ -371,10 +371,10 @@ __poll_t kernfs_generic_poll(struct kernfs_open_file *of,
 			     struct poll_table_struct *pt);
 void kernfs_notify(struct kernfs_node *kn);
 
-int kernfs_security_xattr_get(struct kernfs_node *kn, const char *suffix,
-			      void *value, size_t size);
-int kernfs_security_xattr_set(struct kernfs_node *kn, const char *suffix,
-			      void *value, size_t size, int flags);
+int kernfs_xattr_get(struct kernfs_node *kn, const char *name,
+		     void *value, size_t size);
+int kernfs_xattr_set(struct kernfs_node *kn, const char *name,
+		     const void *value, size_t size, int flags);
 
 const void *kernfs_super_ns(struct super_block *sb);
 int kernfs_get_tree(struct fs_context *fc);
@@ -478,14 +478,12 @@ static inline int kernfs_setattr(struct kernfs_node *kn,
 
 static inline void kernfs_notify(struct kernfs_node *kn) { }
 
-static inline int kernfs_security_xattr_get(struct kernfs_node *kn,
-					    const char *suffix, void *value,
-					    size_t size)
+static inline int kernfs_xattr_get(struct kernfs_node *kn, const char *name,
+				   void *value, size_t size)
 { return -ENOSYS; }
 
-static inline int kernfs_security_xattr_set(struct kernfs_node *kn,
-					    const char *suffix, void *value,
-					    size_t size, int flags)
+static inline int kernfs_xattr_set(struct kernfs_node *kn, const char *name,
+				   const void *value, size_t size, int flags)
 { return -ENOSYS; }
 
 static inline const void *kernfs_super_ns(struct super_block *sb)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index b6e61524d68d..d5fdcb0d26fe 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3394,7 +3394,7 @@ static int selinux_kernfs_init_security(struct kernfs_node *kn_dir,
 	int rc;
 	char *context;
 
-	rc = kernfs_security_xattr_get(kn_dir, XATTR_SELINUX_SUFFIX, NULL, 0);
+	rc = kernfs_xattr_get(kn_dir, XATTR_NAME_SELINUX, NULL, 0);
 	if (rc == -ENODATA)
 		return 0;
 	else if (rc < 0)
@@ -3405,8 +3405,7 @@ static int selinux_kernfs_init_security(struct kernfs_node *kn_dir,
 	if (!context)
 		return -ENOMEM;
 
-	rc = kernfs_security_xattr_get(kn_dir, XATTR_SELINUX_SUFFIX, context,
-				       clen);
+	rc = kernfs_xattr_get(kn_dir, XATTR_NAME_SELINUX, context, clen);
 	if (rc < 0) {
 		kfree(context);
 		return rc;
@@ -3439,8 +3438,8 @@ static int selinux_kernfs_init_security(struct kernfs_node *kn_dir,
 	if (rc)
 		return rc;
 
-	rc = kernfs_security_xattr_set(kn, XATTR_SELINUX_SUFFIX, context, clen,
-				       XATTR_CREATE);
+	rc = kernfs_xattr_set(kn, XATTR_NAME_SELINUX, context, clen,
+			      XATTR_CREATE);
 	kfree(context);
 	return rc;
 }
-- 
2.20.1

Re: [PATCH v3] kernfs: fix xattr name handling in LSM helpers

[Paul Moore]

On Wed, Apr 3, 2019 at 3:25 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Wed, Apr 3, 2019 at 3:24 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Tue, Apr 2, 2019 at 7:10 PM Paul Moore <paul@paul-moore.com> wrote:
> > > On Mon, Apr 1, 2019 at 6:34 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > > The implementation of kernfs_security_xattr_*() helpers reuses the
> > > > kernfs_node_xattr_*() functions, which take the suffix of the xattr name
> > > > and extract full xattr name from it using xattr_full_name(). However,
> > > > this function relies on the fact that the suffix passed to xattr
> > > > handlers from VFS is always constructed from the full name by just
> > > > incerementing the pointer. This doesn't necessarily hold for the callers
> > > > of kernfs_security_xattr_*(), so their usage will easily lead to
> > > > out-of-bounds access.
> > > >
> > > > Fix this by moving the xattr name reconstruction to the VFS xattr
> > > > handlers and replacing the kernfs_security_xattr_*() helpers with more
> > > > general kernfs_xattr_*() helpers that take full xattr name and allow
> > > > accessing all kernfs node's xattrs.
> > > >
> > > > Reported-by: kernel test robot <rong.a.chen@intel.com>
> > > > Fixes: b230d5aba2d1 ("LSM: add new hook for kernfs node initialization")
> > > > Fixes: ec882da5cda9 ("selinux: implement the kernfs_init_security hook")
> > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > > ---
> > > >
> > > > v3: simplify kernfs xattr helpers as per Paul's suggestion
> > > > v2: just rebase to update diff context
> > > >
> > > >  fs/kernfs/inode.c        | 62 ++++++++++++++--------------------------
> > > >  include/linux/kernfs.h   | 18 ++++++------
> > > >  security/selinux/hooks.c |  9 +++---
> > > >  3 files changed, 33 insertions(+), 56 deletions(-)
> > >
> > > This is better, thanks.  Merged into selinux/next.
> >
> > ... and I've just popped it off selinux/next.  It looks like you need
> > to export the kernfs functions.
> >
> > ld: security/selinux/hooks.o: in function `selinux_kernfs_init_security':
> > /pjob.data/scratch/t/server-build.lan-2100-uHT0/BUILD/kernel-5.0.fc31/linux-5.1.
> > 0-0.rc3.git1.1.2.secnext.fc31.x86_64/security/selinux/hooks.c:3397: undefined re
> > ference to `kernfs_xattr_get'
> > ld: /pjob.data/scratch/t/server-build.lan-2100-uHT0/BUILD/kernel-5.0.fc31/linux-
> > 5.1.0-0.rc3.git1.1.2.secnext.fc31.x86_64/security/selinux/hooks.c:3408: undefine
> > d reference to `kernfs_xattr_get'
> > ld: /pjob.data/scratch/t/server-build.lan-2100-uHT0/BUILD/kernel-5.0.fc31/linux-
> > 5.1.0-0.rc3.git1.1.2.secnext.fc31.x86_64/security/selinux/hooks.c:3441: undefine
> > d reference to `kernfs_xattr_set'
> > make: *** [Makefile:1033: vmlinux] Error 1
>
> Hm, I *thought* I ran the build before posting...

Spoiler alert: you didn't.  On my end I know I didn't build a full
kernel to test that your patch built, I just built the fs/kernfs and
security/selinux directories.  I should add a verification step to
help ensure stuff like this doesn't get through review, and you should
*ALWAYS* make sure you can build and boot a kernel with your patches
before submitting them; I don't care if you just changed the name of a
variable, build a new kernel and boot it.

> The problem is that
> the function names in kernfs.h and inode.c didn't match.
> EXPORT_SYMBOL_GPL() shouldn't be needed since both kernfs and SELinux
> are always built-in and there seems to be an existing trend in kernfs
> to not export functions unless necessary.

I didn't look that closely at the failure, I just yanked your patch
out of the tree.  Historically most failures like this are usually
caused by wrong/missing exports, so I just assumed that was the case.

-- 
paul moore
www.paul-moore.com

Re: [PATCH v4] kernfs: fix xattr name handling in LSM helpers

[Paul Moore]

On Wed, Apr 3, 2019 at 3:29 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> The implementation of kernfs_security_xattr_*() helpers reuses the
> kernfs_node_xattr_*() functions, which take the suffix of the xattr name
> and extract full xattr name from it using xattr_full_name(). However,
> this function relies on the fact that the suffix passed to xattr
> handlers from VFS is always constructed from the full name by just
> incerementing the pointer. This doesn't necessarily hold for the callers
> of kernfs_security_xattr_*(), so their usage will easily lead to
> out-of-bounds access.
>
> Fix this by moving the xattr name reconstruction to the VFS xattr
> handlers and replacing the kernfs_security_xattr_*() helpers with more
> general kernfs_xattr_*() helpers that take full xattr name and allow
> accessing all kernfs node's xattrs.
>
> Reported-by: kernel test robot <rong.a.chen@intel.com>
> Fixes: b230d5aba2d1 ("LSM: add new hook for kernfs node initialization")
> Fixes: ec882da5cda9 ("selinux: implement the kernfs_init_security hook")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>
> v4: fix function names and types to make it build...
> v3: simplify kernfs xattr helpers as per Paul's suggestion
> v2: just rebase to update diff context
>
>  fs/kernfs/inode.c        | 62 ++++++++++++++--------------------------
>  include/linux/kernfs.h   | 18 ++++++------
>  security/selinux/hooks.c |  9 +++---
>  3 files changed, 33 insertions(+), 56 deletions(-)

Merged.

-- 
paul moore
www.paul-moore.com