Re: [PATCH 16/17] NFSD: Server implementation of MAC Labeling

[Steve Dickson <SteveD@redhat.com>]

On 08/05/13 21:50, J. Bruce Fields wrote:
> This also doesn't handle the unsupported case correctly.
> 
> Applying the following fix to my tree.
With in the following commit

commit ad8684190a6eb3913a54eda7838f31ead8a73168
Author: David Quigley <dpquigl@davequigley.com>
Date:   Thu May 2 13:19:10 2013 -0400

    NFSD: Server implementation of MAC Labeling

The following chunk of code was committed:

 @@ -380,6 +386,33 @@ nfsd4_decode_fattr(struct nfsd4_compoundargs *argp, u32 *bm
                        goto xdr_error;
                }
        }
+
+       label->len = 0;
+#ifdef CONFIG_NFSD_V4_SECURITY_LABEL
+       if (bmval[2] & FATTR4_WORD2_SECURITY_LABEL) {
+               READ_BUF(4);
+               len += 4;
+               READ32(dummy32); /* lfs: we don't use it */
+               READ_BUF(4);
+               len += 4;
+               READ32(dummy32); /* pi: we don't use it either */
+               READ_BUF(4);
+               len += 4;
+               READ32(dummy32);
+               READ_BUF(dummy32);
+               if (dummy32 > NFSD4_MAX_SEC_LABEL_LEN)
+                       return nfserr_badlabel;
+               len += (XDR_QUADLEN(dummy32) << 2);
+               READMEM(buf, dummy32);
+               label->data = kzalloc(dummy32 + 1, GFP_KERNEL);
+               if (!label->data)
+                       return nfserr_jukebox;
+               defer_free(argp, kfree, label->data);
+               memcpy(label->data, buf, dummy32);
+               label->data[len] = '\0';
+       }
+#endif
+
which is not quite right.... 

You really don't need to NULL out label->data since kzalloc(dummy32 + 1) was 
used and especially with 'len' since only 'dummy32' amount of space was allocated 
which is significantly less than the 'len' value. But you do want to set 
label->len to dummy32 otherwise nfsd4_setattr() will never call nfsd4_set_nfs4_label() 
to process the label. 

commit 965ac99ed149eba2f7d2e37abe07919decf8afa5
Author: Steve Dickson <steved@redhat.com>
Date:   Sat May 11 17:36:21 2013 -0400

    nfsd: Fixed memory corruption in the decoding of labels
    
    Commit ad86841 caused a memory corruption in
    how labels are allocated in nfsd4_decode_fattr()
    by using 'len' to set the NULL in label->data.
    
    Also label->len was not being set which cause
    labels not to be process in nfsd4_setattr()
    
    Signed-off-by: Steve Dickson <steved@redhat.com>

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 96f07cb..a1a3669 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -409,7 +409,7 @@ nfsd4_decode_fattr(struct nfsd4_compoundargs *argp, u32 *bmval,
 			return nfserr_jukebox;
 		defer_free(argp, kfree, label->data);
 		memcpy(label->data, buf, dummy32);
-		label->data[len] = '\0';
+		label->len = dummy32;
 	}
 #endif