linux-hardening - Re: [PATCH] ovl: Check for NULL OVL

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOQ4uxg8rNPUTk8dqz2HmvT9Avy_6WMW4xOMPtG0b8tSUWAKcQ@mail.gmail.com>
Date: Mon, 18 Nov 2024 19:20:52 +0100
From: Amir Goldstein <amir73il@...il.com>
To: Kees Cook <kees@...nel.org>
Cc: Miklos Szeredi <miklos@...redi.hu>, linux-unionfs@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] ovl: Check for NULL OVL_E() results

On Sun, Nov 17, 2024 at 5:46 AM Kees Cook <kees@...nel.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@...nel.org>
> ---
> Cc: Miklos Szeredi <miklos@...redi.hu>
> Cc: Amir Goldstein <amir73il@...il.com>
> Cc: linux-unionfs@...r.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
>