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

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <436766b6fb5f3ec513629d4fa0888b77c65cfa16.camel@hammerspace.com>
Date:   Sun, 27 Mar 2022 15:20:42 +0000
From:   Trond Myklebust <trondmy@...merspace.com>
To:     "anna@...nel.org" <anna@...nel.org>,
        "xiam0nd.tong@...il.com" <xiam0nd.tong@...il.com>
CC:     "bhalevy@...asas.com" <bhalevy@...asas.com>,
        "linux-nfs@...r.kernel.org" <linux-nfs@...r.kernel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "bharrosh@...asas.com" <bharrosh@...asas.com>
Subject: Re: [PATCH] nfs: callback_proc: fix an incorrect NULL check on list
 iterator

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@...r.kernel.org
> Fixes: 1be5683b03a76 ("pnfs: CB_NOTIFY_DEVICEID")
> Signed-off-by: Xiaomeng Tong <xiam0nd.tong@...il.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@...merspace.com