diff mbox

[1/3] NFSv4: Fix pointer arithmetic in decode_getacl

Message ID 1344984447-17804-1-git-send-email-Trond.Myklebust@netapp.com (mailing list archive)
State New, archived
Headers show

Commit Message

Trond Myklebust Aug. 14, 2012, 10:47 p.m. UTC 
Resetting the cursor xdr->p to a previous value is not a safe
practice: if the xdr_stream has crossed out of the initial iovec,
then a bunch of other fields would need to be reset too.

Fix this issue by using xdr_enter_page() so that the buffer gets
page aligned at the bitmap _before_ we decode it.

Also fix the confusion of the ACL length with the page buffer length
by not adding the base offset to the ACL length...

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
Cc: stable@vger.kernel.org
---
 fs/nfs/nfs4proc.c |  2 +-
 fs/nfs/nfs4xdr.c  | 21 +++++++--------------
 2 files changed, 8 insertions(+), 15 deletions(-)

Comments

Sachin Prabhu Aug. 16, 2012, 1:29 p.m. UTC | #1 
On Tue, 2012-08-14 at 18:47 -0400, Trond Myklebust wrote:
> Resetting the cursor xdr->p to a previous value is not a safe
> practice: if the xdr_stream has crossed out of the initial iovec,
> then a bunch of other fields would need to be reset too.
> 
> Fix this issue by using xdr_enter_page() so that the buffer gets
> page aligned at the bitmap _before_ we decode it.
> 
> Also fix the confusion of the ACL length with the page buffer length
> by not adding the base offset to the ACL length...
> 
> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
> Cc: stable@vger.kernel.org
> ---
>  fs/nfs/nfs4proc.c |  2 +-
>  fs/nfs/nfs4xdr.c  | 21 +++++++--------------
>  2 files changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index c77d296..286ab70 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -3819,7 +3819,7 @@ static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu
>  	if (ret)
>  		goto out_free;
>  
> -	acl_len = res.acl_len - res.acl_data_offset;
> +	acl_len = res.acl_len;
>  	if (acl_len > args.acl_len)
>  		nfs4_write_cached_acl(inode, NULL, 0, acl_len);
>  	else
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index ca13483..54d3f5a 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -5049,18 +5049,14 @@ static int decode_getacl(struct xdr_stream *xdr, struct rpc_rqst *req,
>  	uint32_t attrlen,
>  		 bitmap[3] = {0};
>  	int status;
> -	size_t page_len = xdr->buf->page_len;
>  
>  	res->acl_len = 0;
>  	if ((status = decode_op_hdr(xdr, OP_GETATTR)) != 0)
>  		goto out;
>  
> +	xdr_enter_page(xdr, xdr->buf->page_len);
> +
>  	bm_p = xdr->p;
> -	res->acl_data_offset = be32_to_cpup(bm_p) + 2;
> -	res->acl_data_offset <<= 2;
> -	/* Check if the acl data starts beyond the allocated buffer */
> -	if (res->acl_data_offset > page_len)
> -		return -ERANGE;
>  
>  	if ((status = decode_attr_bitmap(xdr, bitmap)) != 0)
>  		goto out;
> @@ -5074,23 +5070,20 @@ static int decode_getacl(struct xdr_stream *xdr, struct rpc_rqst *req,
>  		/* The bitmap (xdr len + bitmaps) and the attr xdr len words
>  		 * are stored with the acl data to handle the problem of
>  		 * variable length bitmaps.*/
> -		xdr->p = bm_p;
> +		res->acl_data_offset = (xdr->p - bm_p) << 2;

The problem here is with an unbounded bitmap. In the case where the
server decides to send a large bitmap and the value of bitmap array + 2
(for the acl length attribute) is greater than the buffer allocated, we
will move to the tail section of the xdr and the pointer arithmetic here
will be wrong. 

Maybe we are better off setting res->acl_data_offset as in the earlier
block. We will not need the variable bm_p in that case.

Sachin Prabhu
>  
>  		/* We ignore &savep and don't do consistency checks on
>  		 * the attr length.  Let userspace figure it out.... */
> -		attrlen += res->acl_data_offset;
> -		if (attrlen > page_len) {
> +		res->acl_len = attrlen;
> +		if (attrlen + res->acl_data_offset > xdr->buf->page_len) {
>  			if (res->acl_flags & NFS4_ACL_LEN_REQUEST) {
>  				/* getxattr interface called with a NULL buf */
> -				res->acl_len = attrlen;
>  				goto out;
>  			}
> -			dprintk("NFS: acl reply: attrlen %u > page_len %zu\n",
> -					attrlen, page_len);
> +			dprintk("NFS: acl reply: attrlen %u > page_len %u\n",
> +					attrlen, xdr->buf->page_len);
>  			return -EINVAL;
>  		}
> -		xdr_read_pages(xdr, attrlen);
> -		res->acl_len = attrlen;
>  	} else
>  		status = -EOPNOTSUPP;
>  



--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Trond Myklebust Aug. 16, 2012, 1:37 p.m. UTC | #2 
T24gVGh1LCAyMDEyLTA4LTE2IGF0IDE0OjI5ICswMTAwLCBTYWNoaW4gUHJhYmh1IHdyb3RlOg0K
PiBPbiBUdWUsIDIwMTItMDgtMTQgYXQgMTg6NDcgLTA0MDAsIFRyb25kIE15a2xlYnVzdCB3cm90
ZToNCj4gPiBSZXNldHRpbmcgdGhlIGN1cnNvciB4ZHItPnAgdG8gYSBwcmV2aW91cyB2YWx1ZSBp
cyBub3QgYSBzYWZlDQo+ID4gcHJhY3RpY2U6IGlmIHRoZSB4ZHJfc3RyZWFtIGhhcyBjcm9zc2Vk
IG91dCBvZiB0aGUgaW5pdGlhbCBpb3ZlYywNCj4gPiB0aGVuIGEgYnVuY2ggb2Ygb3RoZXIgZmll
bGRzIHdvdWxkIG5lZWQgdG8gYmUgcmVzZXQgdG9vLg0KPiA+IA0KPiA+IEZpeCB0aGlzIGlzc3Vl
IGJ5IHVzaW5nIHhkcl9lbnRlcl9wYWdlKCkgc28gdGhhdCB0aGUgYnVmZmVyIGdldHMNCj4gPiBw
YWdlIGFsaWduZWQgYXQgdGhlIGJpdG1hcCBfYmVmb3JlXyB3ZSBkZWNvZGUgaXQuDQo+ID4gDQo+
ID4gQWxzbyBmaXggdGhlIGNvbmZ1c2lvbiBvZiB0aGUgQUNMIGxlbmd0aCB3aXRoIHRoZSBwYWdl
IGJ1ZmZlciBsZW5ndGgNCj4gPiBieSBub3QgYWRkaW5nIHRoZSBiYXNlIG9mZnNldCB0byB0aGUg
QUNMIGxlbmd0aC4uLg0KPiA+IA0KPiA+IFNpZ25lZC1vZmYtYnk6IFRyb25kIE15a2xlYnVzdCA8
VHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20+DQo+ID4gQ2M6IHN0YWJsZUB2Z2VyLmtlcm5lbC5v
cmcNCj4gPiAtLS0NCj4gPiAgZnMvbmZzL25mczRwcm9jLmMgfCAgMiArLQ0KPiA+ICBmcy9uZnMv
bmZzNHhkci5jICB8IDIxICsrKysrKystLS0tLS0tLS0tLS0tLQ0KPiA+ICAyIGZpbGVzIGNoYW5n
ZWQsIDggaW5zZXJ0aW9ucygrKSwgMTUgZGVsZXRpb25zKC0pDQo+ID4gDQo+ID4gZGlmZiAtLWdp
dCBhL2ZzL25mcy9uZnM0cHJvYy5jIGIvZnMvbmZzL25mczRwcm9jLmMNCj4gPiBpbmRleCBjNzdk
Mjk2Li4yODZhYjcwIDEwMDY0NA0KPiA+IC0tLSBhL2ZzL25mcy9uZnM0cHJvYy5jDQo+ID4gKysr
IGIvZnMvbmZzL25mczRwcm9jLmMNCj4gPiBAQCAtMzgxOSw3ICszODE5LDcgQEAgc3RhdGljIHNz
aXplX3QgX19uZnM0X2dldF9hY2xfdW5jYWNoZWQoc3RydWN0IGlub2RlICppbm9kZSwgdm9pZCAq
YnVmLCBzaXplX3QgYnUNCj4gPiAgCWlmIChyZXQpDQo+ID4gIAkJZ290byBvdXRfZnJlZTsNCj4g
PiAgDQo+ID4gLQlhY2xfbGVuID0gcmVzLmFjbF9sZW4gLSByZXMuYWNsX2RhdGFfb2Zmc2V0Ow0K
PiA+ICsJYWNsX2xlbiA9IHJlcy5hY2xfbGVuOw0KPiA+ICAJaWYgKGFjbF9sZW4gPiBhcmdzLmFj
bF9sZW4pDQo+ID4gIAkJbmZzNF93cml0ZV9jYWNoZWRfYWNsKGlub2RlLCBOVUxMLCAwLCBhY2xf
bGVuKTsNCj4gPiAgCWVsc2UNCj4gPiBkaWZmIC0tZ2l0IGEvZnMvbmZzL25mczR4ZHIuYyBiL2Zz
L25mcy9uZnM0eGRyLmMNCj4gPiBpbmRleCBjYTEzNDgzLi41NGQzZjVhIDEwMDY0NA0KPiA+IC0t
LSBhL2ZzL25mcy9uZnM0eGRyLmMNCj4gPiArKysgYi9mcy9uZnMvbmZzNHhkci5jDQo+ID4gQEAg
LTUwNDksMTggKzUwNDksMTQgQEAgc3RhdGljIGludCBkZWNvZGVfZ2V0YWNsKHN0cnVjdCB4ZHJf
c3RyZWFtICp4ZHIsIHN0cnVjdCBycGNfcnFzdCAqcmVxLA0KPiA+ICAJdWludDMyX3QgYXR0cmxl
biwNCj4gPiAgCQkgYml0bWFwWzNdID0gezB9Ow0KPiA+ICAJaW50IHN0YXR1czsNCj4gPiAtCXNp
emVfdCBwYWdlX2xlbiA9IHhkci0+YnVmLT5wYWdlX2xlbjsNCj4gPiAgDQo+ID4gIAlyZXMtPmFj
bF9sZW4gPSAwOw0KPiA+ICAJaWYgKChzdGF0dXMgPSBkZWNvZGVfb3BfaGRyKHhkciwgT1BfR0VU
QVRUUikpICE9IDApDQo+ID4gIAkJZ290byBvdXQ7DQo+ID4gIA0KPiA+ICsJeGRyX2VudGVyX3Bh
Z2UoeGRyLCB4ZHItPmJ1Zi0+cGFnZV9sZW4pOw0KPiA+ICsNCj4gPiAgCWJtX3AgPSB4ZHItPnA7
DQo+ID4gLQlyZXMtPmFjbF9kYXRhX29mZnNldCA9IGJlMzJfdG9fY3B1cChibV9wKSArIDI7DQo+
ID4gLQlyZXMtPmFjbF9kYXRhX29mZnNldCA8PD0gMjsNCj4gPiAtCS8qIENoZWNrIGlmIHRoZSBh
Y2wgZGF0YSBzdGFydHMgYmV5b25kIHRoZSBhbGxvY2F0ZWQgYnVmZmVyICovDQo+ID4gLQlpZiAo
cmVzLT5hY2xfZGF0YV9vZmZzZXQgPiBwYWdlX2xlbikNCj4gPiAtCQlyZXR1cm4gLUVSQU5HRTsN
Cj4gPiAgDQo+ID4gIAlpZiAoKHN0YXR1cyA9IGRlY29kZV9hdHRyX2JpdG1hcCh4ZHIsIGJpdG1h
cCkpICE9IDApDQo+ID4gIAkJZ290byBvdXQ7DQo+ID4gQEAgLTUwNzQsMjMgKzUwNzAsMjAgQEAg
c3RhdGljIGludCBkZWNvZGVfZ2V0YWNsKHN0cnVjdCB4ZHJfc3RyZWFtICp4ZHIsIHN0cnVjdCBy
cGNfcnFzdCAqcmVxLA0KPiA+ICAJCS8qIFRoZSBiaXRtYXAgKHhkciBsZW4gKyBiaXRtYXBzKSBh
bmQgdGhlIGF0dHIgeGRyIGxlbiB3b3Jkcw0KPiA+ICAJCSAqIGFyZSBzdG9yZWQgd2l0aCB0aGUg
YWNsIGRhdGEgdG8gaGFuZGxlIHRoZSBwcm9ibGVtIG9mDQo+ID4gIAkJICogdmFyaWFibGUgbGVu
Z3RoIGJpdG1hcHMuKi8NCj4gPiAtCQl4ZHItPnAgPSBibV9wOw0KPiA+ICsJCXJlcy0+YWNsX2Rh
dGFfb2Zmc2V0ID0gKHhkci0+cCAtIGJtX3ApIDw8IDI7DQo+IA0KPiBUaGUgcHJvYmxlbSBoZXJl
IGlzIHdpdGggYW4gdW5ib3VuZGVkIGJpdG1hcC4gSW4gdGhlIGNhc2Ugd2hlcmUgdGhlDQo+IHNl
cnZlciBkZWNpZGVzIHRvIHNlbmQgYSBsYXJnZSBiaXRtYXAgYW5kIHRoZSB2YWx1ZSBvZiBiaXRt
YXAgYXJyYXkgKyAyDQo+IChmb3IgdGhlIGFjbCBsZW5ndGggYXR0cmlidXRlKSBpcyBncmVhdGVy
IHRoYW4gdGhlIGJ1ZmZlciBhbGxvY2F0ZWQsIHdlDQo+IHdpbGwgbW92ZSB0byB0aGUgdGFpbCBz
ZWN0aW9uIG9mIHRoZSB4ZHIgYW5kIHRoZSBwb2ludGVyIGFyaXRobWV0aWMgaGVyZQ0KPiB3aWxs
IGJlIHdyb25nLiANCg0KPiBNYXliZSB3ZSBhcmUgYmV0dGVyIG9mZiBzZXR0aW5nIHJlcy0+YWNs
X2RhdGFfb2Zmc2V0IGFzIGluIHRoZSBlYXJsaWVyDQo+IGJsb2NrLiBXZSB3aWxsIG5vdCBuZWVk
IHRoZSB2YXJpYWJsZSBibV9wIGluIHRoYXQgY2FzZS4NCg0KDQpJZiB3ZSBvdmVyZmxvdyB0aGUg
cGFnZSB3aGlsZSByZWFkaW5nIHRoZSBiaXRtYXAsIHRoZW4gd2UgYXJlIHNjcmV3ZWQNCmFueXdh
eSwgc2luY2UgeGRyX2lubGluZV9kZWNvZGUoKSB3aWxsIGZhaWwuIENoYW5naW5nIHRoZSB3YXkg
dGhhdCB0aGUNCmRhdGEgb2Zmc2V0IGlzIGNhbGN1bGF0ZWQgd29uJ3QgZml4IHRoYXQuDQoNCi0t
IA0KVHJvbmQgTXlrbGVidXN0DQpMaW51eCBORlMgY2xpZW50IG1haW50YWluZXINCg0KTmV0QXBw
DQpUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbQ0Kd3d3Lm5ldGFwcC5jb20NCg0K
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index c77d296..286ab70 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3819,7 +3819,7 @@  static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu
 	if (ret)
 		goto out_free;
 
-	acl_len = res.acl_len - res.acl_data_offset;
+	acl_len = res.acl_len;
 	if (acl_len > args.acl_len)
 		nfs4_write_cached_acl(inode, NULL, 0, acl_len);
 	else
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index ca13483..54d3f5a 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -5049,18 +5049,14 @@  static int decode_getacl(struct xdr_stream *xdr, struct rpc_rqst *req,
 	uint32_t attrlen,
 		 bitmap[3] = {0};
 	int status;
-	size_t page_len = xdr->buf->page_len;
 
 	res->acl_len = 0;
 	if ((status = decode_op_hdr(xdr, OP_GETATTR)) != 0)
 		goto out;
 
+	xdr_enter_page(xdr, xdr->buf->page_len);
+
 	bm_p = xdr->p;
-	res->acl_data_offset = be32_to_cpup(bm_p) + 2;
-	res->acl_data_offset <<= 2;
-	/* Check if the acl data starts beyond the allocated buffer */
-	if (res->acl_data_offset > page_len)
-		return -ERANGE;
 
 	if ((status = decode_attr_bitmap(xdr, bitmap)) != 0)
 		goto out;
@@ -5074,23 +5070,20 @@  static int decode_getacl(struct xdr_stream *xdr, struct rpc_rqst *req,
 		/* The bitmap (xdr len + bitmaps) and the attr xdr len words
 		 * are stored with the acl data to handle the problem of
 		 * variable length bitmaps.*/
-		xdr->p = bm_p;
+		res->acl_data_offset = (xdr->p - bm_p) << 2;
 
 		/* We ignore &savep and don't do consistency checks on
 		 * the attr length.  Let userspace figure it out.... */
-		attrlen += res->acl_data_offset;
-		if (attrlen > page_len) {
+		res->acl_len = attrlen;
+		if (attrlen + res->acl_data_offset > xdr->buf->page_len) {
 			if (res->acl_flags & NFS4_ACL_LEN_REQUEST) {
 				/* getxattr interface called with a NULL buf */
-				res->acl_len = attrlen;
 				goto out;
 			}
-			dprintk("NFS: acl reply: attrlen %u > page_len %zu\n",
-					attrlen, page_len);
+			dprintk("NFS: acl reply: attrlen %u > page_len %u\n",
+					attrlen, xdr->buf->page_len);
 			return -EINVAL;
 		}
-		xdr_read_pages(xdr, attrlen);
-		res->acl_len = attrlen;
 	} else
 		status = -EOPNOTSUPP;