On Sun, Jun 21, 2020 at 3:33 PM youngjun <her0gyugyu@xxxxxxxxx> wrote: > > lowerdentry could be NULL, and dereferenced by calling d_inode. > code flow which described below > shows possibility of null dereference in ovl_get_inode. I think this is not possible... > > (export.c) ovl_lower_fh_to_d > |_(export.c) ovl_get_dentry(sb, upper, NULL, NULL); This gets called if ovl_index_upper() succeeded and ovl_index_upper() can only return d_is_dir(). > |_(export.c) ovl_obtain_alias (sb, upper, NULL, NULL); This only gets called for !d_is_dir() > |_(inode.c) ovl_get_inode(sb, &oip); > |_(in ovl_get_inode) realinode = d_inode(lowerdentry); > > Fixes: 09d8b586731bf("ovl: move __upperdentry to ovl_inode") > Signed-off-by: youngjun <her0gyugyu@xxxxxxxxx> > --- > fs/overlayfs/inode.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c > index 8be6cd264f66..53d82ef68ba8 100644 > --- a/fs/overlayfs/inode.c > +++ b/fs/overlayfs/inode.c > @@ -958,8 +958,10 @@ struct inode *ovl_get_inode(struct super_block *sb, > unsigned long ino = 0; > int err = oip->newinode ? -EEXIST : -ENOMEM; > > - if (!realinode) > + if (!realinode && lowerdentry) > realinode = d_inode(lowerdentry); > + else > + return ERR_PTR(-EINVAL); > oip->lowerpath and oip->upperdentry should not be both NULL. If you want you can add a WARN_ON about this and return EINVAL, because that would be a bug that needs to be fixed, but I saw no proof that this bug exists. If the problem was reported by a static analysis tool, maybe you can re-factor the helpers in export.c to be less entangled. For example, ovl_obtain_alias() part can be open-coded in the two call sites of ovl_get_dentry() that care about non-dir and then we can assert that ovl_get_dentry() only handles directories. Thanks, Amir.