linux-kernel - re: NFS: handle source server reboot

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

Message-ID: <c64dfc7b-0c0b-e571-5a49-f32034eccaa5@canonical.com>
Date:   Fri, 15 Nov 2019 10:05:19 +0000
From:   Colin Ian King <colin.king@...onical.com>
To:     Olga Kornievskaia <kolga@...app.com>,
        Trond Myklebust <trond.myklebust@...merspace.com>,
        Anna Schumaker <anna.schumaker@...app.com>,
        linux-nfs@...r.kernel.org
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: re: NFS: handle source server reboot

Hi,

Static analysis with Coverity has detected a memory leak in the
following commit:

commit 0e65a32c8a569db363048e17a708b1a0913adbef
Author: Olga Kornievskaia <kolga@...app.com>
Date:   Fri Jun 14 14:38:40 2019 -0400

    NFS: handle source server reboot

In function __nfs4_copy_file_range() in fs/nfs/nfs4file.c, analysis is
as follows:


155retry:
   5. Condition !nfs42_files_from_same_server(file_in, file_out), taking
false branch.
   9. Condition !nfs42_files_from_same_server(file_in, file_out), taking
false branch.

156        if (!nfs42_files_from_same_server(file_in, file_out)) {
157                /* for inter copy, if copy size if smaller than 12 RPC
158                 * payloads, fallback to traditional copy. There are
159                 * 14 RPCs during an NFSv4.x mount between source/dest
160                 * servers.
161                 */
162                if (sync ||
163                        count <= 14 *
NFS_SERVER(file_inode(file_in))->rsize)
164                        return -EOPNOTSUPP;
165                cn_resp = kzalloc(sizeof(struct nfs42_copy_notify_res),
166                                GFP_NOFS);
167                if (unlikely(cn_resp == NULL))
168                        return -ENOMEM;
169
170                ret = nfs42_proc_copy_notify(file_in, file_out, cn_resp);
171                if (ret) {
172                        ret = -EOPNOTSUPP;
173                        goto out;
174                }
175                nss = &cn_resp->cnr_src;
176                cnrs = &cn_resp->cnr_stateid;
177        }
178        ret = nfs42_proc_copy(file_in, pos_in, file_out, pos_out, count,
179                                nss, cnrs, sync);
180out:
   6. freed_arg: kfree frees cn_resp.

   CID 91571 (#1 of 1): Double free (USE_AFTER_FREE)10. double_free:
Calling kfree frees pointer cn_resp which has already been freed.

181        kfree(cn_resp);

   7. Condition ret == -11, taking true branch.

182        if (ret == -EAGAIN)
   8. Jumping to label retry.

183                goto retry;
184        return ret;
185}
186

On the 2nd iteration of the retry loop, cn_resp is being free'd twice if
the call to nfs42_files_from_same_server() returns zero since cn_resp is
not kalloc'd in the 2nd iteration. A naive fix would be to set cn_resp
to NULL after the kfree on line 181, but I'm not sure if there is a
better way to resolve this.

Colin