* Re: [PATCH] [fs] exportfs/expfs.c Validate the dentry in exportfs_decode_fh function
[not found] <1540100386-28137-1-git-send-email-rhmcruiser@gmail.com>
@ 2018-10-21 6:42 ` Amir Goldstein
0 siblings, 0 replies; only message in thread
From: Amir Goldstein @ 2018-10-21 6:42 UTC (permalink / raw)
To: rhmcruiser; +Cc: Miklos Szeredi, linux-kernel, NeilBrown, Ben Hutchings
On Sun, Oct 21, 2018 at 8:42 AM Monthero Ronald <rhmcruiser@gmail.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@vger.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@gmail.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
>
^ permalink raw reply [flat|nested] only message in thread