Re: [PATCH v3 2/3] ovl: ovl_permission: Fix null pointer dereference at realinode in rcu-walk mode

[Amir Goldstein]

On Tue, May 16, 2023 at 5:19 PM Zhihao Cheng <chengzhihao1@xxxxxxxxxx> wrote:
>
> Following process:
>           P1                     P2
>  path_lookupat
>   link_path_walk
>    inode_permission
>     ovl_permission
>       ovl_i_path_real(inode, &realpath)
>         path->dentry = ovl_i_dentry_upper(inode)
>                           drop_cache
>                            __dentry_kill(ovl_dentry)
>                             iput(ovl_inode)
>                              ovl_destroy_inode(ovl_inode)
>                               dput(oi->__upperdentry)
>                                dentry_kill(upperdentry)
>                                 dentry_unlink_inode
>                                  upperdentry->d_inode = NULL
>       realinode = d_inode(realpath.dentry) // return NULL
>       inode_permission(realinode)
>        inode->i_sb  // NULL pointer dereference
> , will trigger an null pointer dereference at realinode:
>   [  335.664979] BUG: kernel NULL pointer dereference,
>                  address: 0000000000000002
>   [  335.668032] CPU: 0 PID: 2592 Comm: ls Not tainted 6.3.0
>   [  335.669956] RIP: 0010:inode_permission+0x33/0x2c0
>   [  335.678939] Call Trace:
>   [  335.679165]  <TASK>
>   [  335.679371]  ovl_permission+0xde/0x320
>   [  335.679723]  inode_permission+0x15e/0x2c0
>   [  335.680090]  link_path_walk+0x115/0x550
>   [  335.680771]  path_lookupat.isra.0+0xb2/0x200
>   [  335.681170]  filename_lookup+0xda/0x240
>   [  335.681922]  vfs_statx+0xa6/0x1f0
>   [  335.682233]  vfs_fstatat+0x7b/0xb0
>
> Fetch a reproducer in [Link].
>
> Use the helper ovl_i_path_realinode() to get realinode and then do
> non-nullptr checking.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217405
> Fixes: 4b7791b2e958 ("ovl: handle idmappings in ovl_permission()")
> Signed-off-by: Zhihao Cheng <chengzhihao1@xxxxxxxxxx>
> Suggested-by: Christian Brauner <brauner@xxxxxxxxxx>
> Suggested-by: Amir Goldstein <amir73il@xxxxxxxxx>

Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx>

> ---
>  fs/overlayfs/inode.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 541cf3717fc2..ca56b1328a2c 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -288,8 +288,8 @@ int ovl_permission(struct mnt_idmap *idmap,
>         int err;
>
>         /* Careful in RCU walk mode */
> -       ovl_i_path_real(inode, &realpath);
> -       if (!realpath.dentry) {
> +       realinode = ovl_i_path_real(inode, &realpath);
> +       if (!realinode) {
>                 WARN_ON(!(mask & MAY_NOT_BLOCK));
>                 return -ECHILD;
>         }
> @@ -302,7 +302,6 @@ int ovl_permission(struct mnt_idmap *idmap,
>         if (err)
>                 return err;
>
> -       realinode = d_inode(realpath.dentry);
>         old_cred = ovl_override_creds(inode->i_sb);
>         if (!upperinode &&
>             !special_file(realinode->i_mode) && mask & MAY_WRITE) {
> --
> 2.39.2
>