[PATCH] ovl: Check for NULL d_inode() in ovl_dentry_upper()

[PATCH] ovl: Check for NULL d_inode() in ovl_dentry_upper()

[Kees Cook]

In ovl_path_type() and ovl_is_metacopy_dentry() 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

Avoid this by allowing ovl_dentry_upper() to return NULL if d_inode() is
NULL, as that means the problematic dereferencing can never be reached.
Note that this fixes the over-eager compiler warning in an effort to
being able to enable -Warray-bounds globally. There is no known
behavioral bug here.

Suggested-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Kees Cook <kees@kernel.org>
---
This is the spirital v2 of https://lore.kernel.org/lkml/20241117044612.work.304-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 | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index 0819c739cc2f..5d6b60d56c27 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -305,7 +305,9 @@ enum ovl_path_type ovl_path_realdata(struct dentry *dentry, struct path *path)
 
 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;
 }
 
 struct dentry *ovl_dentry_lower(struct dentry *dentry)
-- 
2.34.1

Re: [PATCH] ovl: Check for NULL d_inode() in ovl_dentry_upper()

[Miklos Szeredi]

On Tue, 22 Apr 2025 at 01:15, Kees Cook <kees@kernel.org> wrote:
>
> In ovl_path_type() and ovl_is_metacopy_dentry() 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
>
> Avoid this by allowing ovl_dentry_upper() to return NULL if d_inode() is
> NULL, as that means the problematic dereferencing can never be reached.
> Note that this fixes the over-eager compiler warning in an effort to
> being able to enable -Warray-bounds globally. There is no known
> behavioral bug here.
>
> Suggested-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Kees Cook <kees@kernel.org>

Applied, thanks.

Miklos