| Message ID | 20241117044612.work.304-kees@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ovl: Check for NULL OVL_E() results | expand |
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 >
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;
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(+)