linux-kernel - Re: [PATCH] [fs] exportfs/expfs.c Validate the dentry in exportfs_decode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [day] [month] [year] [list]
Message-ID: <CAOQ4uxh2Tq90YzYJ=OY0di8MgB9oOhtzBBs94kYGRaE6QoGhgg@mail.gmail.com>
Date:   Sun, 21 Oct 2018 09:42:23 +0300
From:   Amir Goldstein <amir73il@...il.com>
To:     rhmcruiser@...il.com
Cc:     Miklos Szeredi <mszeredi@...hat.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        NeilBrown <neilb@...e.com>, Ben Hutchings <ben@...adent.org.uk>
Subject: Re: [PATCH] [fs] exportfs/expfs.c Validate the dentry in
 exportfs_decode_fh function

On Sun, Oct 21, 2018 at 8:42 AM Monthero Ronald <rhmcruiser@...il.com> 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@...r.kernel.org>

>     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@...il.com>
> ---
>  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
>