Re: [PATCH] [fs] exportfs/expfs.c Validate the dentry in exportfs_decode_fh function

[Amir Goldstein]

On Sun, Oct 21, 2018 at 8:42 AM Monthero Ronald <rhmcruiser@xxxxxxxxx> wrote:
>
>     Linux kernel 3.16
>

So basically, you want to backport Neil Brown's upstream commit
09bb8bfffd29 exportfs: be careful to only return expected errors.
to stable kernel 3.16 maintained by Ben Hutchings.

You got the wrong address for this patch.
And I suspect you did not check upstream for fixes, before writing your patch?
Please try to apply Neil's patch to 3.16 and it it solves your problem
post the backport to Ben Cc: <stable@xxxxxxxxxxxxxxx>

>     Strengthen the check for invalid dentry returned by fh_to_dentry,
>     because it does not catch the error when dentry is a non-zero invalid address. This results
>     a kernel panic in nfsd.
>
>     Some details of crashed context and issue:
>     crashed task: nfsd
>     crash> bt | grep RIP
>         [exception RIP: exportfs_decode_fh+0x8d]
>         RIP: ffffffff812947fd  RSP: ffff8808085f7bf0  RFLAGS: 00010207
>
>     Disassembly of crash IP:
>     crash> dis -r ffffffff812947fd | tail
>     0xffffffff812947d0 <exportfs_decode_fh+0x60>:       mov    %r8,%r12
>     0xffffffff812947d3 <exportfs_decode_fh+0x63>:       mov    -0x144(%rbp),%edx
>     0xffffffff812947d9 <exportfs_decode_fh+0x69>:       mov    -0x140(%rbp),%rsi
>     0xffffffff812947e0 <exportfs_decode_fh+0x70>:       callq  0xffffffff81337db0 <__x86_indirect_thunk_rax>
>     0xffffffff812947e5 <exportfs_decode_fh+0x75>:       test   %rax,%rax
>     0xffffffff812947e8 <exportfs_decode_fh+0x78>:       mov    %rax,%r13
>     0xffffffff812947eb <exportfs_decode_fh+0x7b>:       je     0xffffffff812948b0 <exportfs_decode_fh+0x140>
>     0xffffffff812947f1 <exportfs_decode_fh+0x81>:       cmp    $0xfffffffffffff000,%rax
>     0xffffffff812947f7 <exportfs_decode_fh+0x87>:       ja     0xffffffff812948a2 <exportfs_decode_fh+0x132>
>     0xffffffff812947fd <exportfs_decode_fh+0x8d>:       mov    0x30(%rax),%rax            << Crashed here
>
>     Source code of exportfs_decode_fh function context with assembly snippet for the checks
>
>                  /*
>                   * Try to get any dentry for the given file handle from the filesystem.
>                   */
>                  if (!nop || !nop->fh_to_dentry)
>                          return ERR_PTR(-ESTALE);
>                  result = nop->fh_to_dentry(mnt->mnt_sb, fid, fh_len, fileid_type);    => mov    %rax,%r13
>
>     => The dentry returned for this filesystem was corrupted and the value held in RAX was mov    %rax,%r13
>        Register   R13: 0000000000000028    RAX: 0000000000000028
>
>                  if (!result)           < Error check bypassed as dentry was not 0x0 but was still an invalid value 0x28
>                          result = ERR_PTR(-ESTALE);
>                  if (IS_ERR(result))                  < As well
>                          return result;
>
>                  if (S_ISDIR(result->d_inode->i_mode)) {      << Crashed  here during dereference
>
>     In assembly       <exportfs_decode_fh+0x8d>:      mov   0x30(%rax),%rax  << Crashed assembly instruction
>
>     The offset 0x30 for d_inode attempted to dereference was
>     crash> struct dentry -ox | grep d_inode
>       [0x30] struct inode *d_inode;   < Offset
>
>     Register  RAX: 0000000000000028  ( dentry held in result = nop->fh_to_dentry(mnt->mnt_sb,
>                                                                  fid, fh_len, fileid_type); )
>     RAX + offset 0x30  = 0x58 which was the invalid address tried to deference for d_inode
>     from dentry structure and hence the crash
>
>     Crash string: PANIC: "BUG: unable to handle kernel NULL pointer dereference at 0000000000000058  <<
>
> Signed-off-by: Monthero Ronald <rhmcruiser@xxxxxxxxx>
> ---
>  fs/exportfs/expfs.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
> index b01fbfb..d9e1adf 100644
> --- a/fs/exportfs/expfs.c
> +++ b/fs/exportfs/expfs.c
> @@ -423,12 +423,12 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,
>         if (!nop || !nop->fh_to_dentry)
>                 return ERR_PTR(-ESTALE);
>         result = nop->fh_to_dentry(mnt->mnt_sb, fid, fh_len, fileid_type);
> -       if (!result)
> -               result = ERR_PTR(-ESTALE);
> -       if (IS_ERR(result))
> -               return result;
> +       if (PTR_ERR(result) == -ENOMEM)
> +               return ERR_CAST(result);
> +       if (IS_ERR_OR_NULL(result))
> +               return ERR_PTR(-ESTALE);
>
> -       if (S_ISDIR(result->d_inode->i_mode)) {
> +       if (d_is_dir(result)) {
>                 /*
>                  * This request is for a directory.
>                  *
> --
> 1.8.3.1
>