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 <[email protected]>
Cc: [email protected]
---
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;
/* 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;
--
1.7.11.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
Instead of using the private field xdr->p from struct xdr_stream,
use the public xdr_stream_pos().
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/nfs4xdr.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 54d3f5a..1bfbd67 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -5045,10 +5045,10 @@ static int decode_getacl(struct xdr_stream *xdr, struct rpc_rqst *req,
struct nfs_getaclres *res)
{
unsigned int savep;
- __be32 *bm_p;
uint32_t attrlen,
bitmap[3] = {0};
int status;
+ unsigned int pg_offset;
res->acl_len = 0;
if ((status = decode_op_hdr(xdr, OP_GETATTR)) != 0)
@@ -5056,7 +5056,8 @@ static int decode_getacl(struct xdr_stream *xdr, struct rpc_rqst *req,
xdr_enter_page(xdr, xdr->buf->page_len);
- bm_p = xdr->p;
+ /* Calculate the offset of the page data */
+ pg_offset = xdr->buf->head[0].iov_len;
if ((status = decode_attr_bitmap(xdr, bitmap)) != 0)
goto out;
@@ -5070,18 +5071,18 @@ 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.*/
- res->acl_data_offset = (xdr->p - bm_p) << 2;
+ res->acl_data_offset = xdr_stream_pos(xdr) - pg_offset;
/* We ignore &savep and don't do consistency checks on
* the attr length. Let userspace figure it out.... */
res->acl_len = attrlen;
- if (attrlen + res->acl_data_offset > xdr->buf->page_len) {
+ if (attrlen > (xdr->nwords << 2)) {
if (res->acl_flags & NFS4_ACL_LEN_REQUEST) {
/* getxattr interface called with a NULL buf */
goto out;
}
dprintk("NFS: acl reply: attrlen %u > page_len %u\n",
- attrlen, xdr->buf->page_len);
+ attrlen, xdr->nwords << 2);
return -EINVAL;
}
} else
--
1.7.11.2
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 <[email protected]>
> Cc: [email protected]
> ---
> 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;
>
Currently, we do not take into account the size of the 16 byte
struct nfs4_cached_acl header, when deciding whether or not we should
cache the acl data. Consequently, we will end up allocating an
8k buffer in order to fit a maximum size 4k acl.
This patch adjusts the calculation so that we limit the cache size
to 4k for the acl header+data.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/nfs4proc.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 286ab70..6352741 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3737,9 +3737,10 @@ out:
static void nfs4_write_cached_acl(struct inode *inode, struct page **pages, size_t pgbase, size_t acl_len)
{
struct nfs4_cached_acl *acl;
+ size_t buflen = sizeof(*acl) + acl_len;
- if (pages && acl_len <= PAGE_SIZE) {
- acl = kmalloc(sizeof(*acl) + acl_len, GFP_KERNEL);
+ if (pages && buflen <= PAGE_SIZE) {
+ acl = kmalloc(buflen, GFP_KERNEL);
if (acl == NULL)
goto out;
acl->cached = 1;
--
1.7.11.2