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@xxxxxxxxxxxxxxx
> Fixes: 1be5683b03a76 ("pnfs: CB_NOTIFY_DEVICEID")
> Signed-off-by: Xiaomeng Tong <xiam0nd.tong@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
