[PATCH] ovl: Check for NULL OVL_E() results

[PATCH] ovl: Check for NULL OVL_E() results

[Kees Cook]

GCC notices that it is possible for OVL_E() to return NULL (which
implies that d_inode(dentry) may be NULL). This would result in out
of bounds reads via container_of(), seen with GCC 15's -Warray-bounds
-fdiagnostics-details. For example:

In file included from ./arch/x86/include/generated/asm/rwonce.h:1,
                 from ../include/linux/compiler.h:339,
                 from ../include/linux/export.h:5,
                 from ../include/linux/linkage.h:7,
                 from ../include/linux/fs.h:5,
                 from ../fs/overlayfs/util.c:7:
In function 'ovl_upperdentry_dereference',
    inlined from 'ovl_dentry_upper' at ../fs/overlayfs/util.c:305:9,
    inlined from 'ovl_path_type' at ../fs/overlayfs/util.c:216:6:
../include/asm-generic/rwonce.h:44:26: error: array subscript 0 is outside array bounds of 'struct inode[7486503276667837]' [-Werror=array-bounds=]
   44 | #define __READ_ONCE(x)  (*(const volatile __unqual_scalar_typeof(x) *)&(x))                       |                         ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~                 ../include/asm-generic/rwonce.h:50:9: note: in expansion of macro '__READ_ONCE'
   50 |         __READ_ONCE(x);                                                 \
      |         ^~~~~~~~~~~
../fs/overlayfs/ovl_entry.h:195:16: note: in expansion of macro 'READ_ONCE'                           195 |         return READ_ONCE(oi->__upperdentry);
      |                ^~~~~~~~~
  'ovl_path_type': event 1
  185 |         return inode ? OVL_I(inode)->oe : NULL;
  'ovl_path_type': event 2

Explicitly check the result of OVL_E() and return accordingly.

Signed-off-by: Kees Cook <kees@kernel.org>
---
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Amir Goldstein <amir73il@gmail.com>
Cc: linux-unionfs@vger.kernel.org
---
 fs/overlayfs/util.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 3bb107471fb4..32ec5eec32fa 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -213,6 +213,9 @@ enum ovl_path_type ovl_path_type(struct dentry *dentry)
 	struct ovl_entry *oe = OVL_E(dentry);
 	enum ovl_path_type type = 0;
 
+	if (WARN_ON_ONCE(oe == NULL))
+		return 0;
+
 	if (ovl_dentry_upper(dentry)) {
 		type = __OVL_PATH_UPPER;
 
@@ -1312,6 +1315,9 @@ bool ovl_is_metacopy_dentry(struct dentry *dentry)
 {
 	struct ovl_entry *oe = OVL_E(dentry);
 
+	if (WARN_ON_ONCE(oe == NULL))
+		return false;
+
 	if (!d_is_reg(dentry))
 		return false;
 
-- 
2.34.1

Re: [PATCH] ovl: Check for NULL OVL_E() results

[Amir Goldstein]

On Sun, Nov 17, 2024 at 5:46 AM Kees Cook <kees@kernel.org> wrote:
>
> GCC notices that it is possible for OVL_E() to return NULL (which
> implies that d_inode(dentry) may be NULL).

I cannot follow this logic.

Yes, OVL_E() can be NULL, but
it does not imply that inode is NULL, so if you think that
code should to be fortified, what's wrong with:

 struct dentry *ovl_dentry_upper(struct dentry *dentry)
 {
-       return ovl_upperdentry_dereference(OVL_I(d_inode(dentry)));
+       struct inode *inode = d_inode(dentry);
+
+       return inode ? ovl_upperdentry_dereference(OVL_I(inode)) : NULL;
 }

TBH, I don't know where the line should be drawn for fortifying against
future bugs, but if the goal of this patch is to silene a compiler warning
then please specify this in the commit message, because I don't think
there is any evidence of an actual bug, is there?

Thanks,
Amir.

> This would result in out
> of bounds reads via container_of(), seen with GCC 15's -Warray-bounds
> -fdiagnostics-details. For example:
>
> In file included from ./arch/x86/include/generated/asm/rwonce.h:1,
>                  from ../include/linux/compiler.h:339,
>                  from ../include/linux/export.h:5,
>                  from ../include/linux/linkage.h:7,
>                  from ../include/linux/fs.h:5,
>                  from ../fs/overlayfs/util.c:7:
> In function 'ovl_upperdentry_dereference',
>     inlined from 'ovl_dentry_upper' at ../fs/overlayfs/util.c:305:9,
>     inlined from 'ovl_path_type' at ../fs/overlayfs/util.c:216:6:
> ../include/asm-generic/rwonce.h:44:26: error: array subscript 0 is outside array bounds of 'struct inode[7486503276667837]' [-Werror=array-bounds=]
>    44 | #define __READ_ONCE(x)  (*(const volatile __unqual_scalar_typeof(x) *)&(x))                       |                         ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~                 ../include/asm-generic/rwonce.h:50:9: note: in expansion of macro '__READ_ONCE'
>    50 |         __READ_ONCE(x);                                                 \
>       |         ^~~~~~~~~~~
> ../fs/overlayfs/ovl_entry.h:195:16: note: in expansion of macro 'READ_ONCE'                           195 |         return READ_ONCE(oi->__upperdentry);
>       |                ^~~~~~~~~
>   'ovl_path_type': event 1
>   185 |         return inode ? OVL_I(inode)->oe : NULL;
>   'ovl_path_type': event 2
>
> Explicitly check the result of OVL_E() and return accordingly.
>
> Signed-off-by: Kees Cook <kees@kernel.org>
> ---
> Cc: Miklos Szeredi <miklos@szeredi.hu>
> Cc: Amir Goldstein <amir73il@gmail.com>
> Cc: linux-unionfs@vger.kernel.org
> ---
>  fs/overlayfs/util.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 3bb107471fb4..32ec5eec32fa 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -213,6 +213,9 @@ enum ovl_path_type ovl_path_type(struct dentry *dentry)
>         struct ovl_entry *oe = OVL_E(dentry);
>         enum ovl_path_type type = 0;
>
> +       if (WARN_ON_ONCE(oe == NULL))
> +               return 0;
> +
>         if (ovl_dentry_upper(dentry)) {
>                 type = __OVL_PATH_UPPER;
>
> @@ -1312,6 +1315,9 @@ bool ovl_is_metacopy_dentry(struct dentry *dentry)
>  {
>         struct ovl_entry *oe = OVL_E(dentry);
>
> +       if (WARN_ON_ONCE(oe == NULL))
> +               return false;
> +
>         if (!d_is_reg(dentry))
>                 return false;
>
> --
> 2.34.1
>

Re: [PATCH] ovl: Check for NULL OVL_E() results

[kernel test robot]

Hello,

kernel test robot noticed "WARNING:at_fs/overlayfs/util.c:#ovl_path_type[overlay]" on:

commit: d6b14141241121ff7761dc8dfb33d27284fc5331 ("[PATCH] ovl: Check for NULL OVL_E() results")
url: https://github.com/intel-lab-lkp/linux/commits/Kees-Cook/ovl-Check-for-NULL-OVL_E-results/20241121-100558
base: https://git.kernel.org/cgit/linux/kernel/git/vfs/vfs.git vfs.all
patch link: https://lore.kernel.org/all/20241117044612.work.304-kees@kernel.org/
patch subject: [PATCH] ovl: Check for NULL OVL_E() results

in testcase: ltp
version: ltp-x86_64-14c1f76-1_20241111
with following parameters:

	disk: 1HDD
	fs: ext4
	test: syscalls-07



config: x86_64-rhel-9.4-ltp
compiler: gcc-12
test machine: 4 threads 1 sockets Intel(R) Core(TM) i3-3220 CPU @ 3.30GHz (Ivy Bridge) with 8G memory

(please refer to attached dmesg/kmsg for entire log/backtrace)



If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202411251652.ecbb3c7e-lkp@intel.com


The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20241125/202411251652.ecbb3c7e-lkp@intel.com


kern  :warn  : [  407.439702] ------------[ cut here ]------------
user  :notice: [  407.448057] fanotify06.c:134: TPASS: group 0 got event: mask 2 pid=5267 fd=13
kern  :warn  : [  407.448607] WARNING: CPU: 0 PID: 5267 at fs/overlayfs/util.c:211 ovl_path_type+0xdb/0x220 [overlay]

kern  :warn  : [  407.461773] Modules linked in:
user  :notice: [  407.473065] fanotify06.c:134: TPASS: group 1 got event: mask 2 pid=5267 fd=13
kern  :warn  : [  407.473711]  overlay

kern  :warn  : [  407.485300]  brd exfat vfat fat xfs
user  :notice: [  407.489747] fanotify06.c:134: TPASS: group 2 got event: mask 2 pid=5267 fd=13
kern  :warn  : [  407.490395]  ext2 ext4 mbcache jbd2 netconsole btrfs blake2b_generic xor zstd_compress

kern  :warn  : [  407.502433]  raid6_pq libcrc32c snd_hda_codec_realtek snd_hda_codec_generic snd_hda_scodec_component
user  :notice: [  407.512136] fanotify06.c:217: TPASS: group 3 got no event
kern  :warn  : [  407.513249]  intel_rapl_msr intel_rapl_common sd_mod x86_pkg_temp_thermal

kern  :warn  : [  407.529180]  intel_powerclamp sg coretemp snd_hda_intel
user  :notice: [  407.537741] fanotify06.c:217: TPASS: group 4 got no event
kern  :warn  : [  407.538876]  kvm_intel ipmi_devintf ipmi_msghandler

kern  :warn  : [  407.550897]  i915 snd_intel_dspcfg snd_intel_sdw_acpi kvm
user  :notice: [  407.557559] fanotify06.c:217: TPASS: group 5 got no event
kern  :warn  : [  407.558678]  cec snd_hda_codec snd_hda_core

kern  :warn  : [  407.570873]  intel_gtt snd_hwdep crct10dif_pclmul crc32_pclmul drm_buddy
user  :notice: [  407.576845] fanotify06.c:217: TPASS: group 6 got no event
kern  :warn  : [  407.577961]  crc32c_intel ahci ghash_clmulni_intel

user  :notice: [  407.586986] fanotify06.c:217: TPASS: group 7 got no event
kern  :warn  : [  407.591490]  snd_pcm

kern  :warn  : [  407.599214]  drm_display_helper ttm rapl libahci snd_timer intel_cstate
user  :notice: [  407.606451] fanotify06.c:217: TPASS: group 8 got no event
kern  :warn  : [  407.608219]  wmi_bmof

kern  :warn  : [  407.617727]  mei_me intel_uncore drm_kms_helper snd pcspkr
user  :notice: [  407.625498] fanotify06.c:158: TINFO: Test #1: Fanotify merge overlayfs mount mark
kern  :warn  : [  407.626824]  i2c_i801 lpc_ich i2c_smbus

kern  :warn  : [  407.635208]  libata mei soundcore video wmi binfmt_misc loop fuse drm dm_mod ip_tables
kern  :warn  : [  407.659894] CPU: 0 UID: 0 PID: 5267 Comm: fanotify06 Tainted: G S                 6.12.0-rc5-00193-gd6b141412411 #1
kern  :warn  : [  407.671076] Tainted: [S]=CPU_OUT_OF_SPEC
kern  :warn  : [  407.675743] Hardware name: Hewlett-Packard HP Pro 3340 MT/17A1, BIOS 8.07 01/24/2013
kern  :warn  : [  407.684219] RIP: 0010:ovl_path_type+0xdb/0x220 [overlay]
kern  :warn  : [  407.690304] Code: 01 00 00 41 8b 04 24 4d 85 f6 0f 84 b7 00 00 00 41 bc 01 00 00 00 85 c0 75 25 5b 44 89 e0 5d 41 5c 41 5d 41 5e c3 cc cc cc cc <0f> 0b 45 31 e4 5b 5d 44 89 e0 41 5c 41 5d 41 5e c3 cc cc cc cc 4c
kern  :warn  : [  407.709811] RSP: 0018:ffffc900021ff870 EFLAGS: 00010246
kern  :warn  : [  407.715790] RAX: dffffc0000000000 RBX: ffff8881e6ea33a0 RCX: ffffffffc2f437e4
kern  :warn  : [  407.723669] RDX: 1ffff1103cdd46c2 RSI: ffffc900021ff8f0 RDI: ffff8881e6ea3610
kern  :warn  : [  407.731551] RBP: ffff888001a9e800 R08: 0000000000000000 R09: ffffed1000353d0f
kern  :warn  : [  407.739432] R10: ffff888001a9e87f R11: ffff88811835de00 R12: 0000000000000000
kern  :warn  : [  407.747312] R13: ffff888001a9e830 R14: ffffc900021ff8f0 R15: 000000000011801e
kern  :warn  : [  407.755192] FS:  00007fc80cb3a740(0000) GS:ffff888174c00000(0000) knlGS:0000000000000000
kern  :warn  : [  407.764026] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
kern  :warn  : [  407.770512] CR2: 00007f3378006280 CR3: 000000013af18005 CR4: 00000000001726f0
kern  :warn  : [  407.778391] Call Trace:
kern  :warn  : [  407.781606]  <TASK>
kern  :warn  : [  407.784463]  ? __warn+0xcd/0x260
kern  :warn  : [  407.788451]  ? ovl_path_type+0xdb/0x220 [overlay]
kern  :warn  : [  407.793929]  ? report_bug+0x25d/0x2c0
kern  :warn  : [  407.798337]  ? handle_bug+0x53/0xa0
kern  :warn  : [  407.802582]  ? exc_invalid_op+0x13/0x40
kern  :warn  : [  407.807159]  ? asm_exc_invalid_op+0x16/0x20
kern  :warn  : [  407.812092]  ? ovl_already_copied_up+0x94/0x110 [overlay]
kern  :warn  : [  407.818264]  ? ovl_path_type+0xdb/0x220 [overlay]
kern  :warn  : [  407.823744]  ovl_path_realdata+0x16/0x200 [overlay]
kern  :warn  : [  407.829384]  ovl_open+0x179/0x220 [overlay]
kern  :warn  : [  407.834335]  ? __pfx_ovl_open+0x10/0x10 [overlay]
kern  :warn  : [  407.839807]  ? revert_creds+0x7a/0xb0
kern  :warn  : [  407.844218]  ? ovl_permission+0x143/0x1f0 [overlay]
kern  :warn  : [  407.849862]  do_dentry_open+0x453/0x10d0
kern  :warn  : [  407.854532]  ? __pfx_ovl_open+0x10/0x10 [overlay]
kern  :warn  : [  407.860011]  vfs_open+0x75/0x340
kern  :warn  : [  407.863987]  do_open+0x41c/0xd40
kern  :warn  : [  407.867963]  path_openat+0x23b/0x630
kern  :warn  : [  407.872289]  ? __pfx_path_openat+0x10/0x10
kern  :warn  : [  407.877141]  ? __pfx_ksys_write+0x10/0x10
kern  :warn  : [  407.881898]  ? kill_pid_info_type+0xa6/0xc0
kern  :warn  : [  407.886824]  do_filp_open+0x1b0/0x3e0
kern  :warn  : [  407.891240]  ? syscall_exit_to_user_mode+0xc/0x1e0
kern  :warn  : [  407.896780]  ? __pfx_do_filp_open+0x10/0x10
kern  :warn  : [  407.901705]  ? kfree+0xef/0x400
kern  :warn  : [  407.905595]  ? _raw_spin_lock+0x81/0xe0
kern  :warn  : [  407.910185]  ? strncpy_from_user+0x28/0x1f0
kern  :warn  : [  407.915111]  ? alloc_fd+0x269/0x440
kern  :warn  : [  407.919350]  do_sys_openat2+0x11e/0x160
kern  :warn  : [  407.923934]  ? __kasan_slab_alloc+0x2f/0x70
kern  :warn  : [  407.928867]  ? __pfx_do_sys_openat2+0x10/0x10
kern  :warn  : [  407.933975]  ? syscall_exit_to_user_mode+0xc/0x1e0
kern  :warn  : [  407.939513]  ? syscall_exit_to_user_mode+0xc/0x1e0
kern  :warn  : [  407.945048]  __x64_sys_openat+0x135/0x1d0
kern  :warn  : [  407.949806]  ? __pfx___x64_sys_openat+0x10/0x10
kern  :warn  : [  407.955082]  ? syscall_exit_to_user_mode+0x1c1/0x1e0
kern  :warn  : [  407.960795]  ? do_syscall_64+0x85/0x150
kern  :warn  : [  407.965377]  do_syscall_64+0x79/0x150
kern  :warn  : [  407.969790]  ? kmem_cache_free+0x265/0x4d0
kern  :warn  : [  407.974635]  ? switch_fpu_return+0xe8/0x1f0
kern  :warn  : [  407.979567]  ? __task_pid_nr_ns+0x21e/0x2a0
kern  :warn  : [  407.984497]  ? syscall_exit_to_user_mode+0xc/0x1e0
kern  :warn  : [  407.990035]  ? do_syscall_64+0x85/0x150
kern  :warn  : [  407.994617]  ? do_syscall_64+0x85/0x150
kern  :warn  : [  407.999198]  ? do_syscall_64+0x85/0x150
kern  :warn  : [  408.003785]  ? exc_page_fault+0x57/0xc0
kern  :warn  : [  408.008361]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
kern  :warn  : [  408.014144] RIP: 0033:0x7fc80cc34f81
kern  :warn  : [  408.018477] Code: 75 57 89 f0 25 00 00 41 00 3d 00 00 41 00 74 49 80 3d 6a 26 0e 00 00 74 6d 89 da 48 89 ee bf 9c ff ff ff b8 01 01 00 00 0f 05 <48> 3d 00 f0 ff ff 0f 87 93 00 00 00 48 8b 54 24 28 64 48 2b 14 25
kern  :warn  : [  408.037988] RSP: 002b:00007ffdfed4d7b0 EFLAGS: 00000202 ORIG_RAX: 0000000000000101
kern  :warn  : [  408.046305] RAX: ffffffffffffffda RBX: 0000000000000041 RCX: 00007fc80cc34f81
kern  :warn  : [  408.054181] RDX: 0000000000000041 RSI: 000055e24d04dd40 RDI: 00000000ffffff9c
kern  :warn  : [  408.062062] RBP: 000055e24d04dd40 R08: 00000000000001a4 R09: 0000000000000000
kern  :warn  : [  408.069934] R10: 00000000000001b6 R11: 0000000000000202 R12: 00000000000000a6
kern  :warn  : [  408.077813] R13: 00000000000001a4 R14: 0000000000000000 R15: 0000000000000000
kern  :warn  : [  408.085692]  </TASK>

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] ovl: Check for NULL OVL_E() results

[Kees Cook]

On Mon, Nov 18, 2024 at 07:20:52PM +0100, Amir Goldstein wrote:
> On Sun, Nov 17, 2024 at 5:46 AM Kees Cook <kees@kernel.org> wrote:
> >
> > GCC notices that it is possible for OVL_E() to return NULL (which
> > implies that d_inode(dentry) may be NULL).
> 
> I cannot follow this logic.
> 
> Yes, OVL_E() can be NULL, but
> it does not imply that inode is NULL, so if you think that
> code should to be fortified, what's wrong with:
> 
>  struct dentry *ovl_dentry_upper(struct dentry *dentry)
>  {
> -       return ovl_upperdentry_dereference(OVL_I(d_inode(dentry)));
> +       struct inode *inode = d_inode(dentry);
> +
> +       return inode ? ovl_upperdentry_dereference(OVL_I(inode)) : NULL;
>  }
> 
> TBH, I don't know where the line should be drawn for fortifying against
> future bugs, but if the goal of this patch is to silene a compiler warning
> then please specify this in the commit message, because I don't think
> there is any evidence of an actual bug, is there?

Sorry for the delay on this! I'm finally coming back around to these
fixes. :)

Yes, your suggestion works very nicely! That entirely solves the GCC
warning.

And correct, this was to deal with an over-eager compiler warning --
there was no bug here that I'm aware of.

I will send an updated patch with your suggestion.

-Kees

-- 
Kees Cook