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

["J. Bruce Fields" <bfields@fieldses.org>]

On Sat, May 11, 2013 at 05:44:52PM -0400, Steve Dickson wrote:
> tatus: RO
> Content-Length: 2809
> Lines: 82
> 
> 
> 
> 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. 

Ah-hah, thanks!

> 
> 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] = '

Applied to my tree.

--b.