nfsd: fix rare symlink decoding bug

From: "J. Bruce Fields" <[email protected]>

An NFS operation that creates a new symlink includes the symlink data, which is xdr-encoded as a length followed by the data plus 0 to 3 bytes of zero-padding as required to reach a 4-byte boundary.

The vfs, on the other hand, wants null-terminated data.

The simple way to handle this would be by copying the data into a newly allocated buffer with space for the final null.

The current nfsd_symlink code tries to be more clever by skipping that step in the (likely) case where the byte following the string is already 0.

But that assumes that the byte following the string is ours to look at. In fact, it might be the first byte of a page that we can't read, or of some object that another task might modify.

Worse, the NFSv4 code tries to fix the problem by actually writing to that byte.

In the NFSv2/v3 cases this actually appears to be safe:

- nfs3svc_decode_symlinkargs explicitly null-terminates the data (after first checking its length and copying it to a new page).
- NFSv2 limits symlinks to 1k. The buffer holding the rpc request is always at least a page, and the link data (and previous fields) have maximum lengths that prevent the request from reaching the end of a page.

In the NFSv4 case the CREATE op is potentially just one part of a long compound so can end up on the end of a page if you're unlucky.

The minimal fix here is to copy and null-terminate in the NFSv4 case. The nfsd_symlink() interface here seems too fragile, though. It should really either do the copy itself every time or just require a null-terminated string.

Reported-by Jeff Layton <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/nfs4proc.c |   22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

fs/nfsd/nfs4proc.c +3 -6

index 6851b00..453c907 100644
@@ -601,6 +601,8 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 {
        struct svc_fh resfh;
        __be32 status;
+       u32 len;
+       char *data;
        dev_t rdev;
 
        fh_init(&resfh, NFS4_FHSIZE);

@@ -617,19 +619,21 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 
        switch (create->cr_type) {
        case NF4LNK:
-               /* ugh! we have to null-terminate the linktext, or
-                * vfs_symlink() will choke.  it is always safe to
-                * null-terminate by brute force, since at worst we
-                * will overwrite the first byte of the create namelen
-                * in the XDR buffer, which has already been extracted
-                * during XDR decode.
+               len = create->cr_linklen;

fs/nfsd/nfs4proc.c +3 -2

+               data[len] = '\0';
                status = nfsd_symlink(rqstp, &cstate->current_fh,
                                      create->cr_name, create->cr_namelen,
-                                     create->cr_linkname, create->cr_linklen,
+                                     data, len,
                                      &resfh, &create->cr_iattr);
+               kfree(data);
                break;
 
        case NF4BLK:
--

1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Review

Quoted 11 lines · J. Bruce Fields

(quoted diff) +8 -2

+               len = create->cr_linklen;
+               data = kmalloc(len + 1, GFP_KERNEL);
+               /*
+                * Null-terminating in place isn't safe since
+                * cr_linkname might end on a page boundary.
                 */
-               create->cr_linkname[create->cr_linklen] = 0;
-
+               if (!data)
+                       return nfserr_jukebox;
+               memcpy(data, create->cr_linkname, len + 1);

Shouldn't that be a copy of 'len' bytes?

fs/nfsd/nfs4xdr.c +5 -1

index 2d305a1..56bdf4a 100644
@@ -600,7 +600,18 @@ nfsd4_decode_create(struct nfsd4_compoundargs *argp, struct nfsd4_create *create
                READ_BUF(4);
                create->cr_linklen = be32_to_cpup(p++);
                READ_BUF(create->cr_linklen);
-               SAVEMEM(create->cr_linkname, create->cr_linklen);
+               /*
+                * The VFS will want a null-terminated string, and
+                * null-terminating in place isn't safe since this might
+                * end on a page boundary:
+                */

Quoted 14 lines · J. Bruce Fields

fs/nfsd/nfs4xdr.c +5 -1

index 2d305a1..56bdf4a 100644
@@ -600,7 +600,18 @@ nfsd4_decode_create(struct nfsd4_compoundargs *argp, struct nfsd4_create *create
                READ_BUF(4);
                create->cr_linklen = be32_to_cpup(p++);
                READ_BUF(create->cr_linklen);
-               SAVEMEM(create->cr_linkname, create->cr_linklen);
+               /*
+                * The VFS will want a null-terminated string, and
+                * null-terminating in place isn't safe since this might
+                * end on a page boundary:
+                */

fs/nfsd/nfs4xdr.c nfsd4_decode_create(struct nfsd4_compoundargs *argp, struct nfsd4_create *create · line 617

Quoted 10 lines

fs/nfsd/nfs4xdr.c +7 -0

+               create->cr_linkname =
+                               kmalloc(create->cr_linklen + 1, GFP_KERNEL);
+               if (!create->cr_linkname)
+                       return nfserr_jukebox;
+               memcpy(create->cr_linkname, p, create->cr_linklen);
+               create->cr_linkname[create->cr_linklen] = '\0';
+               defer_free(argp, kfree, create->cr_linkname);
                break;
        case NF4BLK:
        case NF4CHR:

Note that "defer_free()" does yet another allocation here in order to make space for a small 'struct tmpbuf' structure. Unlike the first allocation, there is no check for whether or not that second allocation is successful above, so you can easily end up with a silent memory leakage (ditto for a number of other callers of defer_free()).

Looking around at all the other users, wouldn't it perhaps make sense to replace defer_free() with a helper that does just a single allocation for both the memory buffer and the struct tmpbuf?