On Sun, Nov 17, 2024 at 5:46 AM Kees Cook <kees@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> Cc: Miklos Szeredi <miklos@xxxxxxxxxx>
> Cc: Amir Goldstein <amir73il@xxxxxxxxx>
> Cc: linux-unionfs@xxxxxxxxxxxxxxx
> ---
> 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
>
