From: James Morris Subject: Re: [PATCH 3/8][RFC v05] NFSv3: add client implementation of XATTR protocol Date: Thu, 24 Jun 2010 18:35:33 +1000 (EST) Message-ID: References: <20100624043351.GA6024@hallyn.com> Mime-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Cc: linux-nfs@vger.kernel.org, linux-security-module@vger.kernel.org, Trond Myklebust , "J. Bruce Fields" , Neil Brown , linux-fsdevel@vger.kernel.org, Stephen Smalley To: "Serge E. Hallyn" Return-path: In-Reply-To: <20100624043351.GA6024@hallyn.com> Sender: linux-security-module-owner@vger.kernel.org List-ID: On Wed, 23 Jun 2010, Serge E. Hallyn wrote: > > + status = res.xattr_val_len; > > + if (status <= size) > > res.xattr_val_len was set to size, as was status, and none of the > 3 has been changed, so here status can't be > size can it? res is part of msg, and is updated by the RPC layer when decoding the response (via xdr_decode_string_inplace()). > Was this just a safety to prevent overrun, or did you mean to > do some other check? (If a safety, then you'll still return > status > size, but with garbage in value, so i think you'd want > to also change status) > > > + memcpy(value, res.xattr_val, status); > Yes, the check stops us from copying more than the max. expected size to 'value'. It looks like we do need to return -EINVAL if the check fails, and likely the same with listxattr(). (Or is there a better error code for reporting invalid messages from the peer?) - James -- James Morris