Re: [PATCH] nfs: callback_proc: fix an incorrect NULL check on list iterator

[Trond Myklebust <trondmy@hammerspace.com>]

On Sun, 2022-03-27 at 16:02 +0800, Xiaomeng Tong wrote:
> The bug is here:
>         if (!server ||
>         server->pnfs_curr_ld->id != dev->cbd_layout_type) {
> 
> The list iterator value 'server' will *always* be set and non-NULL
> by list_for_each_entry_rcu, so it is incorrect to assume that the
> iterator value will be NULL if the list is empty or no element is
> found (In fact, it will be a bogus pointer to an invalid struct
> object containing the HEAD, which is used for above check at next
> outer loop). Otherwise it may bypass the check in theory (iif
> server->pnfs_curr_ld->id == dev->cbd_layout_type, 'server' now is
> a bogus pointer) and lead to invalid memory access passing the check.
> 
> To fix the bug, use a new variable 'iter' as the list iterator,
> while use the original variable 'server' as a dedicated pointer to
> point to the found element.
> 
> Cc: stable@vger.kernel.org
> Fixes: 1be5683b03a76 ("pnfs: CB_NOTIFY_DEVICEID")
> Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
> ---
>  fs/nfs/callback_proc.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
> index c343666d9a42..84779785dc8d 100644
> --- a/fs/nfs/callback_proc.c
> +++ b/fs/nfs/callback_proc.c
> @@ -361,7 +361,7 @@ __be32 nfs4_callback_devicenotify(void *argp,
> void *resp,
>         uint32_t i;
>         __be32 res = 0;
>         struct nfs_client *clp = cps->clp;
> -       struct nfs_server *server = NULL;
> +       struct nfs_server *server = NULL, *iter;
>  
>         if (!clp) {
>                 res = cpu_to_be32(NFS4ERR_OP_NOT_IN_SESSION);
> @@ -374,10 +374,11 @@ __be32 nfs4_callback_devicenotify(void *argp,
> void *resp,
>                 if (!server ||
>                     server->pnfs_curr_ld->id != dev->cbd_layout_type)
> {
>                         rcu_read_lock();
> -                       list_for_each_entry_rcu(server, &clp-
> >cl_superblocks, client_link)
> -                               if (server->pnfs_curr_ld &&
> -                                   server->pnfs_curr_ld->id == dev-
> >cbd_layout_type) {
> +                       list_for_each_entry_rcu(iter, &clp-
> >cl_superblocks, client_link)
> +                               if (iter->pnfs_curr_ld &&
> +                                   iter->pnfs_curr_ld->id == dev-
> >cbd_layout_type) {
>                                         rcu_read_unlock();
> +                                       server = iter;

Hmm... We're not holding any locks on the super block for 'iter' here,
so nothing is preventing it from going away while we're.

Given that we really only want a pointer to the struct
pnfs_layoutdriver_type anyway, why not just convert the code to save a
pointer to that (and do it while holding the rcu_read_lock())?

The struct pnfs_layoutdriver is always expected to be a statically
allocated structure, so it won't go away as long as the pNFS driver
module remains loaded.

>                                         goto found;
>                                 }
>                         rcu_read_unlock();

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