Re: [PATCH] NFSv4: Fix free of uninitialized nfs4_label on referral lookup.

[Trond Myklebust <trondmy@hammerspace.com>]

On Fri, 2022-05-13 at 15:45 -0400, Benjamin Coddington wrote:
> Send along the already-allocated fattr along with nfs4_fs_locations,
> and
> drop the memcpy of fattr.  We end up growing two more allocations,
> but this

Thanks Ben!

I know this is more work than your original fix, but I think in the
long run we're better off. Doing memcpy() on a struct fattr can be
risky, since there are several pointers to malloc()ed memory, so it
really needs to be discouraged and fixed when we see legacy code cases
like this one.

> fixes up a crash as:
> 
> PID: 790    TASK: ffff88811b43c000  CPU: 0   COMMAND: "ls"
>  #0 [ffffc90000857920] panic at ffffffff81b9bfde
>  #1 [ffffc900008579c0] do_trap at ffffffff81023a9b
>  #2 [ffffc90000857a10] do_error_trap at ffffffff81023b78
>  #3 [ffffc90000857a58] exc_stack_segment at ffffffff81be1f45
>  #4 [ffffc90000857a80] asm_exc_stack_segment at ffffffff81c009de
>  #5 [ffffc90000857b08] nfs_lookup at ffffffffa0302322 [nfs]
>  #6 [ffffc90000857b70] __lookup_slow at ffffffff813a4a5f
>  #7 [ffffc90000857c60] walk_component at ffffffff813a86c4
>  #8 [ffffc90000857cb8] path_lookupat at ffffffff813a9553
>  #9 [ffffc90000857cf0] filename_lookup at ffffffff813ab86b
> 
> Suggested-by: Trond Myklebust <trondmy@hammerspace.com>
> Fixes: 9558a007dbc3 ("NFS: Remove the label from the nfs4_lookup_res
> struct")
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/nfs/nfs4namespace.c  |  4 ++++
>  fs/nfs/nfs4proc.c       | 15 +++++++--------
>  fs/nfs/nfs4state.c      |  8 +++++++-
>  fs/nfs/nfs4xdr.c        |  4 ++--
>  include/linux/nfs_xdr.h |  2 +-
>  5 files changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c
> index 3680c8da510c..de5f5db6c416 100644
> --- a/fs/nfs/nfs4namespace.c
> +++ b/fs/nfs/nfs4namespace.c
> @@ -417,6 +417,9 @@ static int nfs_do_refmount(struct fs_context *fc,
> struct rpc_clnt *client)
>         fs_locations = kmalloc(sizeof(struct nfs4_fs_locations),
> GFP_KERNEL);
>         if (!fs_locations)
>                 goto out_free;
> +       fs_locations->fattr = nfs_alloc_fattr();
> +       if (!fs_locations->fattr)
> +               goto out_free;

Won't this leak the memory at fs_locations()?

>  
>         /* Get locations */
>         dentry = ctx->clone_data.dentry;
> @@ -436,6 +439,7 @@ static int nfs_do_refmount(struct fs_context *fc,
> struct rpc_clnt *client)
>  
>         err = nfs_follow_referral(fc, fs_locations);
>  out_free_2:
> +       kfree(fs_locations->fattr);
>         kfree(fs_locations);
>  out_free:
>         __free_page(page);
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index a79f66432bd3..0600f85b6016 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -4243,6 +4243,8 @@ static int nfs4_get_referral(struct rpc_clnt
> *client, struct inode *dir,
>         if (locations == NULL)
>                 goto out;
>  
> +       locations->fattr = fattr;
> +
>         status = nfs4_proc_fs_locations(client, dir, name, locations,
> page);
>         if (status != 0)
>                 goto out;
> @@ -4252,17 +4254,14 @@ static int nfs4_get_referral(struct rpc_clnt
> *client, struct inode *dir,
>          * referral.  Cause us to drop into the exception handler,
> which
>          * will kick off migration recovery.
>          */
> -       if (nfs_fsid_equal(&NFS_SERVER(dir)->fsid, &locations-
> >fattr.fsid)) {
> +       if (nfs_fsid_equal(&NFS_SERVER(dir)->fsid, &fattr->fsid)) {
>                 dprintk("%s: server did not return a different fsid
> for"
>                         " a referral at %s\n", __func__, name->name);
>                 status = -NFS4ERR_MOVED;
>                 goto out;
>         }
>         /* Fixup attributes for the nfs_lookup() call to nfs_fhget()
> */
> -       nfs_fixup_referral_attributes(&locations->fattr);
> -
> -       /* replace the lookup nfs_fattr with the locations nfs_fattr
> */
> -       memcpy(fattr, &locations->fattr, sizeof(struct nfs_fattr));
> +       nfs_fixup_referral_attributes(fattr);
>         memset(fhandle, 0, sizeof(struct nfs_fh));
>  out:
>         if (page)
> @@ -7902,7 +7901,7 @@ static int _nfs4_proc_fs_locations(struct
> rpc_clnt *client, struct inode *dir,
>         else
>                 bitmask[1] &= ~FATTR4_WORD1_MOUNTED_ON_FILEID;
>  
> -       nfs_fattr_init(&fs_locations->fattr);
> +       nfs_fattr_init(fs_locations->fattr);
>         fs_locations->server = server;
>         fs_locations->nlocations = 0;
>         status = nfs4_call_sync(client, server, &msg, &args.seq_args,
> &res.seq_res, 0);
> @@ -7967,7 +7966,7 @@ static int _nfs40_proc_get_locations(struct
> nfs_server *server,
>         unsigned long now = jiffies;
>         int status;
>  
> -       nfs_fattr_init(&locations->fattr);
> +       nfs_fattr_init(locations->fattr);
>         locations->server = server;
>         locations->nlocations = 0;
>  
> @@ -8032,7 +8031,7 @@ static int _nfs41_proc_get_locations(struct
> nfs_server *server,
>         };
>         int status;
>  
> -       nfs_fattr_init(&locations->fattr);
> +       nfs_fattr_init(locations->fattr);
>         locations->server = server;
>         locations->nlocations = 0;
>  
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index 9e1c987c81e7..067b0f1938f5 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -2106,6 +2106,11 @@ static int nfs4_try_migration(struct
> nfs_server *server, const struct cred *cred
>                 dprintk("<-- %s: no memory\n", __func__);
>                 goto out;
>         }
> +       locations->fattr = nfs_alloc_fattr();
> +       if (locations->fattr == NULL) {
> +               dprintk("<-- %s: no memory\n", __func__);
> +               goto out;
> +       }
>  
>         inode = d_inode(server->super->s_root);
>         result = nfs4_proc_get_locations(server, NFS_FH(inode),
> locations,
> @@ -2120,7 +2125,7 @@ static int nfs4_try_migration(struct nfs_server
> *server, const struct cred *cred
>         if (!locations->nlocations)
>                 goto out;
>  
> -       if (!(locations->fattr.valid & NFS_ATTR_FATTR_V4_LOCATIONS))
> {
> +       if (!(locations->fattr->valid & NFS_ATTR_FATTR_V4_LOCATIONS))
> {
>                 dprintk("<-- %s: No fs_locations data, migration
> skipped\n",
>                         __func__);
>                 goto out;
> @@ -2145,6 +2150,7 @@ static int nfs4_try_migration(struct nfs_server
> *server, const struct cred *cred
>  out:
>         if (page != NULL)
>                 __free_page(page);
> +       kfree(locations->fattr);

Won't this blow up if locations == NULL?

>         kfree(locations);
>         if (result) {
>                 pr_err("NFS: migration recovery failed (server
> %s)\n",
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index 86a5f6516928..5d822594336d 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -7051,7 +7051,7 @@ static int nfs4_xdr_dec_fs_locations(struct
> rpc_rqst *req,
>         if (res->migration) {
>                 xdr_enter_page(xdr, PAGE_SIZE);
>                 status = decode_getfattr_generic(xdr,
> -                                       &res->fs_locations->fattr,
> +                                       res->fs_locations->fattr,
>                                          NULL, res->fs_locations,
>                                          res->fs_locations->server);
>                 if (status)
> @@ -7064,7 +7064,7 @@ static int nfs4_xdr_dec_fs_locations(struct
> rpc_rqst *req,
>                         goto out;
>                 xdr_enter_page(xdr, PAGE_SIZE);
>                 status = decode_getfattr_generic(xdr,
> -                                       &res->fs_locations->fattr,
> +                                       res->fs_locations->fattr,
>                                          NULL, res->fs_locations,
>                                          res->fs_locations->server);
>         }
> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> index 2863e5a69c6a..20e97329fe46 100644
> --- a/include/linux/nfs_xdr.h
> +++ b/include/linux/nfs_xdr.h
> @@ -1212,7 +1212,7 @@ struct nfs4_fs_location {
>  
>  #define NFS4_FS_LOCATIONS_MAXENTRIES 10
>  struct nfs4_fs_locations {
> -       struct nfs_fattr fattr;
> +       struct nfs_fattr *fattr;
>         const struct nfs_server *server;
>         struct nfs4_pathname fs_path;
>         int nlocations;

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com