* [PATCH] Avoid array overflow in __nfs4_get_acl_uncached @ 2012-08-24 14:16 Sachin Prabhu 2012-08-24 15:07 ` Myklebust, Trond 0 siblings, 1 reply; 13+ messages in thread From: Sachin Prabhu @ 2012-08-24 14:16 UTC (permalink / raw) To: Linux NFS mailing list; +Cc: Trond Myklebust This fixes a bug introduced by commit 5a00689930ab975fdd1b37b034475017e460cf2a The patch adds an extra page to npages to hold the bitmap returned by the server. Bruce Fields pointed out that the changes introduced by the patch will cause the array npages to overflow if a buffer of size greater than or equal to XATTR_SIZE_MAX is passed to __nfs4_get_acl_uncached() Signed-off-by: Sachin Prabhu <sprabhu@redhat.com> --- fs/nfs/nfs4proc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 6352741..86333b8 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -3768,7 +3768,7 @@ out: */ static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t buflen) { - struct page *pages[NFS4ACL_MAXPAGES] = {NULL, }; + struct page *pages[NFS4ACL_MAXPAGES+1] = {NULL, }; struct nfs_getaclargs args = { .fh = NFS_FH(inode), .acl_pages = pages, -- 1.7.11.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] Avoid array overflow in __nfs4_get_acl_uncached 2012-08-24 14:16 [PATCH] Avoid array overflow in __nfs4_get_acl_uncached Sachin Prabhu @ 2012-08-24 15:07 ` Myklebust, Trond 2012-08-24 21:31 ` Sachin Prabhu 0 siblings, 1 reply; 13+ messages in thread From: Myklebust, Trond @ 2012-08-24 15:07 UTC (permalink / raw) To: Sachin Prabhu; +Cc: Linux NFS mailing list T24gRnJpLCAyMDEyLTA4LTI0IGF0IDE1OjE2ICswMTAwLCBTYWNoaW4gUHJhYmh1IHdyb3RlOg0K PiBUaGlzIGZpeGVzIGEgYnVnIGludHJvZHVjZWQgYnkgY29tbWl0DQo+IDVhMDA2ODk5MzBhYjk3 NWZkZDFiMzdiMDM0NDc1MDE3ZTQ2MGNmMmENCj4gVGhlIHBhdGNoIGFkZHMgYW4gZXh0cmEgcGFn ZSB0byBucGFnZXMgdG8gaG9sZCB0aGUgYml0bWFwIHJldHVybmVkIGJ5DQo+IHRoZSBzZXJ2ZXIu DQo+IA0KPiBCcnVjZSBGaWVsZHMgcG9pbnRlZCBvdXQgdGhhdCB0aGUgY2hhbmdlcyBpbnRyb2R1 Y2VkIGJ5IHRoZSBwYXRjaCB3aWxsDQo+IGNhdXNlIHRoZSBhcnJheSBucGFnZXMgdG8gb3ZlcmZs b3cgaWYgYSBidWZmZXIgb2Ygc2l6ZSBncmVhdGVyIHRoYW4gb3INCj4gZXF1YWwgdG8gWEFUVFJf U0laRV9NQVggaXMgcGFzc2VkIHRvIF9fbmZzNF9nZXRfYWNsX3VuY2FjaGVkKCkNCg0KSSdkIHRo aW5rIHRoYXQgdGhlIHJpZ2h0IHRoaW5nIHRvIGRvIGhlcmUgaXMgcmF0aGVyIHRvIGFkZCBhcHBy b3ByaWF0ZQ0KYnVmZmVyIG92ZXJmbG93IGNoZWNrcy4gSG93IGFib3V0IHNvbWV0aGluZyBsaWtl IHRoZSBmb2xsb3dpbmc/DQoNCjg8LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0NCkZyb20gN2MzNWNlMjIwOTI0MTgyMjg0YWVhOWY4 YWVjMzliMGQ5OTE2MDBkZiBNb24gU2VwIDE3IDAwOjAwOjAwIDIwMDENCkZyb206IFRyb25kIE15 a2xlYnVzdCA8VHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20+DQpEYXRlOiBGcmksIDI0IEF1ZyAy MDEyIDEwOjU5OjI1IC0wNDAwDQpTdWJqZWN0OiBbUEFUQ0hdIE5GU3Y0OiBGaXggcmFuZ2UgY2hl Y2tpbmcgaW4gX19uZnM0X2dldF9hY2xfdW5jYWNoZWQgYW5kDQogX19uZnM0X3Byb2Nfc2V0X2Fj bA0KDQpFbnN1cmUgdGhhdCB0aGUgdXNlciBzdXBwbGllZCBidWZmZXIgc2l6ZSBkb2Vzbid0IGNh dXNlIHVzIHRvIG92ZXJmbG93DQp0aGUgJ3BhZ2VzJyBhcnJheS4NCg0KQWxzbyBmaXggdXAgc29t ZSBjb25mdXNpb24gYmV0d2VlbiB0aGUgdXNlIG9mIFBBR0VfU0laRSBhbmQNClBBR0VfQ0FDSEVf U0laRSB3aGVuIGNhbGN1bGF0aW5nIGJ1ZmZlciBzaXplcy4gV2UncmUgbm90IHVzaW5nDQp0aGUg cGFnZSBjYWNoZSBmb3IgYW55dGhpbmcgaGVyZS4NCg0KU2lnbmVkLW9mZi1ieTogVHJvbmQgTXlr bGVidXN0IDxUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbT4NCi0tLQ0KIGZzL25mcy9uZnM0cHJv Yy5jIHwgMjAgKysrKysrKysrKystLS0tLS0tLS0NCiAxIGZpbGUgY2hhbmdlZCwgMTEgaW5zZXJ0 aW9ucygrKSwgOSBkZWxldGlvbnMoLSkNCg0KZGlmZiAtLWdpdCBhL2ZzL25mcy9uZnM0cHJvYy5j IGIvZnMvbmZzL25mczRwcm9jLmMNCmluZGV4IDYzNTI3NDEuLjY1NGRjMzggMTAwNjQ0DQotLS0g YS9mcy9uZnMvbmZzNHByb2MuYw0KKysrIGIvZnMvbmZzL25mczRwcm9jLmMNCkBAIC0zNjUzLDEx ICszNjUzLDExIEBAIHN0YXRpYyBpbmxpbmUgaW50IG5mczRfc2VydmVyX3N1cHBvcnRzX2FjbHMo c3RydWN0IG5mc19zZXJ2ZXIgKnNlcnZlcikNCiAJCSYmIChzZXJ2ZXItPmFjbF9iaXRtYXNrICYg QUNMNF9TVVBQT1JUX0RFTllfQUNMKTsNCiB9DQogDQotLyogQXNzdW1pbmcgdGhhdCBYQVRUUl9T SVpFX01BWCBpcyBhIG11bHRpcGxlIG9mIFBBR0VfQ0FDSEVfU0laRSwgYW5kIHRoYXQNCi0gKiBp dCdzIE9LIHRvIHB1dCBzaXplb2Yodm9pZCkgKiAoWEFUVFJfU0laRV9NQVgvUEFHRV9DQUNIRV9T SVpFKSBieXRlcyBvbg0KKy8qIEFzc3VtaW5nIHRoYXQgWEFUVFJfU0laRV9NQVggaXMgYSBtdWx0 aXBsZSBvZiBQQUdFX1NJWkUsIGFuZCB0aGF0DQorICogaXQncyBPSyB0byBwdXQgc2l6ZW9mKHZv aWQpICogKFhBVFRSX1NJWkVfTUFYL1BBR0VfU0laRSkgYnl0ZXMgb24NCiAgKiB0aGUgc3RhY2su DQogICovDQotI2RlZmluZSBORlM0QUNMX01BWFBBR0VTIChYQVRUUl9TSVpFX01BWCA+PiBQQUdF X0NBQ0hFX1NISUZUKQ0KKyNkZWZpbmUgTkZTNEFDTF9NQVhQQUdFUyBESVZfUk9VTkRfVVAoWEFU VFJfU0laRV9NQVgsIFBBR0VfU0laRSkNCiANCiBzdGF0aWMgaW50IGJ1Zl90b19wYWdlc19ub3Ns YWIoY29uc3Qgdm9pZCAqYnVmLCBzaXplX3QgYnVmbGVuLA0KIAkJc3RydWN0IHBhZ2UgKipwYWdl cywgdW5zaWduZWQgaW50ICpwZ2Jhc2UpDQpAQCAtMzY2OCw3ICszNjY4LDcgQEAgc3RhdGljIGlu dCBidWZfdG9fcGFnZXNfbm9zbGFiKGNvbnN0IHZvaWQgKmJ1Ziwgc2l6ZV90IGJ1ZmxlbiwNCiAJ c3BhZ2VzID0gcGFnZXM7DQogDQogCWRvIHsNCi0JCWxlbiA9IG1pbl90KHNpemVfdCwgUEFHRV9D QUNIRV9TSVpFLCBidWZsZW4pOw0KKwkJbGVuID0gbWluX3Qoc2l6ZV90LCBQQUdFX1NJWkUsIGJ1 Zmxlbik7DQogCQluZXdwYWdlID0gYWxsb2NfcGFnZShHRlBfS0VSTkVMKTsNCiANCiAJCWlmIChu ZXdwYWdlID09IE5VTEwpDQpAQCAtMzc4MiwxNyArMzc4MiwxNiBAQCBzdGF0aWMgc3NpemVfdCBf X25mczRfZ2V0X2FjbF91bmNhY2hlZChzdHJ1Y3QgaW5vZGUgKmlub2RlLCB2b2lkICpidWYsIHNp emVfdCBidQ0KIAkJLnJwY19hcmdwID0gJmFyZ3MsDQogCQkucnBjX3Jlc3AgPSAmcmVzLA0KIAl9 Ow0KLQlpbnQgcmV0ID0gLUVOT01FTSwgbnBhZ2VzLCBpOw0KKwl1bnNpZ25lZCBpbnQgbnBhZ2Vz ID0gRElWX1JPVU5EX1VQKGJ1ZmxlbiwgUEFHRV9TSVpFKTsNCisJaW50IHJldCA9IC1FTk9NRU0s IGk7DQogCXNpemVfdCBhY2xfbGVuID0gMDsNCiANCi0JbnBhZ2VzID0gKGJ1ZmxlbiArIFBBR0Vf U0laRSAtIDEpID4+IFBBR0VfU0hJRlQ7DQogCS8qIEFzIGxvbmcgYXMgd2UncmUgZG9pbmcgYSBy b3VuZCB0cmlwIHRvIHRoZSBzZXJ2ZXIgYW55d2F5LA0KIAkgKiBsZXQncyBiZSBwcmVwYXJlZCBm b3IgYSBwYWdlIG9mIGFjbCBkYXRhLiAqLw0KIAlpZiAobnBhZ2VzID09IDApDQogCQlucGFnZXMg PSAxOw0KLQ0KLQkvKiBBZGQgYW4gZXh0cmEgcGFnZSB0byBoYW5kbGUgdGhlIGJpdG1hcCByZXR1 cm5lZCAqLw0KLQlucGFnZXMrKzsNCisJaWYgKG5wYWdlcyA+IEFSUkFZX1NJWkUocGFnZXMpKQ0K KwkJcmV0dXJuIC1FUkFOR0U7DQogDQogCWZvciAoaSA9IDA7IGkgPCBucGFnZXM7IGkrKykgew0K IAkJcGFnZXNbaV0gPSBhbGxvY19wYWdlKEdGUF9LRVJORUwpOw0KQEAgLTM4OTEsMTAgKzM4OTAs MTMgQEAgc3RhdGljIGludCBfX25mczRfcHJvY19zZXRfYWNsKHN0cnVjdCBpbm9kZSAqaW5vZGUs IGNvbnN0IHZvaWQgKmJ1Ziwgc2l6ZV90IGJ1ZmwNCiAJCS5ycGNfYXJncAk9ICZhcmcsDQogCQku cnBjX3Jlc3AJPSAmcmVzLA0KIAl9Ow0KKwl1bnNpZ25lZCBpbnQgbnBhZ2VzID0gRElWX1JPVU5E X1VQKGJ1ZmxlbiwgUEFHRV9TSVpFKTsNCiAJaW50IHJldCwgaTsNCiANCiAJaWYgKCFuZnM0X3Nl cnZlcl9zdXBwb3J0c19hY2xzKHNlcnZlcikpDQogCQlyZXR1cm4gLUVPUE5PVFNVUFA7DQorCWlm IChucGFnZXMgPiBBUlJBWV9TSVpFKHBhZ2VzKSkNCisJCXJldHVybiAtRVJBTkdFOw0KIAlpID0g YnVmX3RvX3BhZ2VzX25vc2xhYihidWYsIGJ1ZmxlbiwgYXJnLmFjbF9wYWdlcywgJmFyZy5hY2xf cGdiYXNlKTsNCiAJaWYgKGkgPCAwKQ0KIAkJcmV0dXJuIGk7DQotLSANCjEuNy4xMS40DQoNCg0K LS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFpbnRhaW5lcg0KDQpOZXRB cHANClRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tDQp3d3cubmV0YXBwLmNvbQ0KDQo= ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Avoid array overflow in __nfs4_get_acl_uncached 2012-08-24 15:07 ` Myklebust, Trond @ 2012-08-24 21:31 ` Sachin Prabhu 2012-08-24 21:38 ` Myklebust, Trond 0 siblings, 1 reply; 13+ messages in thread From: Sachin Prabhu @ 2012-08-24 21:31 UTC (permalink / raw) To: Myklebust, Trond; +Cc: Linux NFS mailing list On Fri, 2012-08-24 at 15:07 +0000, Myklebust, Trond wrote: > On Fri, 2012-08-24 at 15:16 +0100, Sachin Prabhu wrote: > > This fixes a bug introduced by commit > > 5a00689930ab975fdd1b37b034475017e460cf2a > > The patch adds an extra page to npages to hold the bitmap returned by > > the server. > > > > Bruce Fields pointed out that the changes introduced by the patch will > > cause the array npages to overflow if a buffer of size greater than or > > equal to XATTR_SIZE_MAX is passed to __nfs4_get_acl_uncached() > > I'd think that the right thing to do here is rather to add appropriate > buffer overflow checks. How about something like the following? > > 8<-------------------------------------------------------------- > From 7c35ce220924182284aea9f8aec39b0d991600df Mon Sep 17 00:00:00 2001 > From: Trond Myklebust <Trond.Myklebust@netapp.com> > Date: Fri, 24 Aug 2012 10:59:25 -0400 > Subject: [PATCH] NFSv4: Fix range checking in __nfs4_get_acl_uncached and > __nfs4_proc_set_acl > > Ensure that the user supplied buffer size doesn't cause us to overflow > the 'pages' array. > > Also fix up some confusion between the use of PAGE_SIZE and > PAGE_CACHE_SIZE when calculating buffer sizes. We're not using > the page cache for anything here. > > Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com> This patch is susceptible to the problem described in commit 5a00689930ab975fdd1b37b034475017e460cf2a This can be demonstrated by the following patch to pynfs which pads the bitmap with 1000 extra elements. To reproduce, on the server cd newpynfs/nfs4.0/ ./setup.py build_ext --inplace ./nfs4server.py on the client, mount -o vers=4 SERVER:/ /mnt touch /mnt/a nfs4_getfacl /mnt/a With this new patch, you will get a general protection fault in _copy_from_pages(). >From 1ca7fbab80ded2fc79c668786ba22d585a988ab5 Mon Sep 17 00:00:00 2001 From: Sachin Prabhu <sprabhu@redhat.com> Date: Fri, 24 Aug 2012 22:16:16 +0100 Subject: [PATCH] Demonstrate unbounded bitmap problem for nfs4 getacl --- nfs4.0/lib/nfs4/nfs4lib.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/nfs4.0/lib/nfs4/nfs4lib.py b/nfs4.0/lib/nfs4/nfs4lib.py index 0a38ebc..940ae86 100644 --- a/nfs4.0/lib/nfs4/nfs4lib.py +++ b/nfs4.0/lib/nfs4/nfs4lib.py @@ -87,9 +87,17 @@ class FancyNFS4Packer(nfs4_pack.NFS4Packer): """Handle fattr4 and dirlist4 more cleanly than auto-generated methods""" def filter_bitmap4(self, data): out = [] + flag = 0; while data: + flag = 1; out.append(data & 0xffffffffL) data >>= 32 + if (flag and out[0] == 4096) : + print "add test data"; + i = 1000 + while i: + out.append(0) + i -= 1 return out def filter_fattr4(self, data): -- 1.7.11.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] Avoid array overflow in __nfs4_get_acl_uncached 2012-08-24 21:31 ` Sachin Prabhu @ 2012-08-24 21:38 ` Myklebust, Trond 2012-08-24 21:51 ` Sachin Prabhu 0 siblings, 1 reply; 13+ messages in thread From: Myklebust, Trond @ 2012-08-24 21:38 UTC (permalink / raw) To: Sachin Prabhu; +Cc: Linux NFS mailing list T24gRnJpLCAyMDEyLTA4LTI0IGF0IDIyOjMxICswMTAwLCBTYWNoaW4gUHJhYmh1IHdyb3RlOg0K PiBPbiBGcmksIDIwMTItMDgtMjQgYXQgMTU6MDcgKzAwMDAsIE15a2xlYnVzdCwgVHJvbmQgd3Jv dGU6DQo+ID4gT24gRnJpLCAyMDEyLTA4LTI0IGF0IDE1OjE2ICswMTAwLCBTYWNoaW4gUHJhYmh1 IHdyb3RlOg0KPiA+ID4gVGhpcyBmaXhlcyBhIGJ1ZyBpbnRyb2R1Y2VkIGJ5IGNvbW1pdA0KPiA+ ID4gNWEwMDY4OTkzMGFiOTc1ZmRkMWIzN2IwMzQ0NzUwMTdlNDYwY2YyYQ0KPiA+ID4gVGhlIHBh dGNoIGFkZHMgYW4gZXh0cmEgcGFnZSB0byBucGFnZXMgdG8gaG9sZCB0aGUgYml0bWFwIHJldHVy bmVkIGJ5DQo+ID4gPiB0aGUgc2VydmVyLg0KPiA+ID4gDQo+ID4gPiBCcnVjZSBGaWVsZHMgcG9p bnRlZCBvdXQgdGhhdCB0aGUgY2hhbmdlcyBpbnRyb2R1Y2VkIGJ5IHRoZSBwYXRjaCB3aWxsDQo+ ID4gPiBjYXVzZSB0aGUgYXJyYXkgbnBhZ2VzIHRvIG92ZXJmbG93IGlmIGEgYnVmZmVyIG9mIHNp emUgZ3JlYXRlciB0aGFuIG9yDQo+ID4gPiBlcXVhbCB0byBYQVRUUl9TSVpFX01BWCBpcyBwYXNz ZWQgdG8gX19uZnM0X2dldF9hY2xfdW5jYWNoZWQoKQ0KPiA+IA0KPiA+IEknZCB0aGluayB0aGF0 IHRoZSByaWdodCB0aGluZyB0byBkbyBoZXJlIGlzIHJhdGhlciB0byBhZGQgYXBwcm9wcmlhdGUN Cj4gPiBidWZmZXIgb3ZlcmZsb3cgY2hlY2tzLiBIb3cgYWJvdXQgc29tZXRoaW5nIGxpa2UgdGhl IGZvbGxvd2luZz8NCj4gPiANCj4gPiA4PC0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tDQo+ID4gRnJvbSA3YzM1Y2UyMjA5MjQxODIy ODRhZWE5ZjhhZWMzOWIwZDk5MTYwMGRmIE1vbiBTZXAgMTcgMDA6MDA6MDAgMjAwMQ0KPiA+IEZy b206IFRyb25kIE15a2xlYnVzdCA8VHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20+DQo+ID4gRGF0 ZTogRnJpLCAyNCBBdWcgMjAxMiAxMDo1OToyNSAtMDQwMA0KPiA+IFN1YmplY3Q6IFtQQVRDSF0g TkZTdjQ6IEZpeCByYW5nZSBjaGVja2luZyBpbiBfX25mczRfZ2V0X2FjbF91bmNhY2hlZCBhbmQN Cj4gPiAgX19uZnM0X3Byb2Nfc2V0X2FjbA0KPiA+IA0KPiA+IEVuc3VyZSB0aGF0IHRoZSB1c2Vy IHN1cHBsaWVkIGJ1ZmZlciBzaXplIGRvZXNuJ3QgY2F1c2UgdXMgdG8gb3ZlcmZsb3cNCj4gPiB0 aGUgJ3BhZ2VzJyBhcnJheS4NCj4gPiANCj4gPiBBbHNvIGZpeCB1cCBzb21lIGNvbmZ1c2lvbiBi ZXR3ZWVuIHRoZSB1c2Ugb2YgUEFHRV9TSVpFIGFuZA0KPiA+IFBBR0VfQ0FDSEVfU0laRSB3aGVu IGNhbGN1bGF0aW5nIGJ1ZmZlciBzaXplcy4gV2UncmUgbm90IHVzaW5nDQo+ID4gdGhlIHBhZ2Ug Y2FjaGUgZm9yIGFueXRoaW5nIGhlcmUuDQo+ID4gDQo+ID4gU2lnbmVkLW9mZi1ieTogVHJvbmQg TXlrbGVidXN0IDxUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbT4NCj4gDQo+IFRoaXMgcGF0Y2gg aXMgc3VzY2VwdGlibGUgdG8gdGhlIHByb2JsZW0gZGVzY3JpYmVkIGluIGNvbW1pdA0KPiA1YTAw Njg5OTMwYWI5NzVmZGQxYjM3YjAzNDQ3NTAxN2U0NjBjZjJhDQo+IA0KPiBUaGlzIGNhbiBiZSBk ZW1vbnN0cmF0ZWQgYnkgdGhlIGZvbGxvd2luZyBwYXRjaCB0byBweW5mcyB3aGljaCBwYWRzIHRo ZQ0KPiBiaXRtYXAgd2l0aCAxMDAwIGV4dHJhIGVsZW1lbnRzLg0KPiANCj4gVG8gcmVwcm9kdWNl LCBvbiB0aGUgc2VydmVyDQo+IGNkIG5ld3B5bmZzL25mczQuMC8NCj4gLi9zZXR1cC5weSBidWls ZF9leHQgLS1pbnBsYWNlDQo+IC4vbmZzNHNlcnZlci5weQ0KPiANCj4gb24gdGhlIGNsaWVudCwg DQo+IG1vdW50IC1vIHZlcnM9NCBTRVJWRVI6LyAvbW50DQo+IHRvdWNoIC9tbnQvYQ0KPiBuZnM0 X2dldGZhY2wgL21udC9hDQo+IA0KPiBXaXRoIHRoaXMgbmV3IHBhdGNoLCB5b3Ugd2lsbCBnZXQg YSBnZW5lcmFsIHByb3RlY3Rpb24gZmF1bHQgaW4NCj4gX2NvcHlfZnJvbV9wYWdlcygpLg0KDQpJ cyB0aGlzIG9uIGEga2VybmVsIHdpdGggY29tbWl0IDUxOWQzOTU5ZTMwYTk4ZjhlMTM1ZTdhMTY2 NDdjMTBhZjVhZDYzZDUNCihORlN2NDogRml4IHBvaW50ZXIgYXJpdGhtZXRpYyBpbiBkZWNvZGVf Z2V0YWNsKSBhcHBsaWVkPw0KDQotLSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVu dCBtYWludGFpbmVyDQoNCk5ldEFwcA0KVHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5u ZXRhcHAuY29tDQoNCg== ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Avoid array overflow in __nfs4_get_acl_uncached 2012-08-24 21:38 ` Myklebust, Trond @ 2012-08-24 21:51 ` Sachin Prabhu 2012-08-24 22:02 ` Myklebust, Trond 0 siblings, 1 reply; 13+ messages in thread From: Sachin Prabhu @ 2012-08-24 21:51 UTC (permalink / raw) To: Myklebust, Trond; +Cc: Linux NFS mailing list On Fri, 2012-08-24 at 21:38 +0000, Myklebust, Trond wrote: > On Fri, 2012-08-24 at 22:31 +0100, Sachin Prabhu wrote: > > On Fri, 2012-08-24 at 15:07 +0000, Myklebust, Trond wrote: > > > On Fri, 2012-08-24 at 15:16 +0100, Sachin Prabhu wrote: > > > > This fixes a bug introduced by commit > > > > 5a00689930ab975fdd1b37b034475017e460cf2a > > > > The patch adds an extra page to npages to hold the bitmap returned by > > > > the server. > > > > > > > > Bruce Fields pointed out that the changes introduced by the patch will > > > > cause the array npages to overflow if a buffer of size greater than or > > > > equal to XATTR_SIZE_MAX is passed to __nfs4_get_acl_uncached() > > > > > > I'd think that the right thing to do here is rather to add appropriate > > > buffer overflow checks. How about something like the following? > > > > > > 8<-------------------------------------------------------------- > > > From 7c35ce220924182284aea9f8aec39b0d991600df Mon Sep 17 00:00:00 2001 > > > From: Trond Myklebust <Trond.Myklebust@netapp.com> > > > Date: Fri, 24 Aug 2012 10:59:25 -0400 > > > Subject: [PATCH] NFSv4: Fix range checking in __nfs4_get_acl_uncached and > > > __nfs4_proc_set_acl > > > > > > Ensure that the user supplied buffer size doesn't cause us to overflow > > > the 'pages' array. > > > > > > Also fix up some confusion between the use of PAGE_SIZE and > > > PAGE_CACHE_SIZE when calculating buffer sizes. We're not using > > > the page cache for anything here. > > > > > > Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com> > > > > This patch is susceptible to the problem described in commit > > 5a00689930ab975fdd1b37b034475017e460cf2a > > > > This can be demonstrated by the following patch to pynfs which pads the > > bitmap with 1000 extra elements. > > > > To reproduce, on the server > > cd newpynfs/nfs4.0/ > > ./setup.py build_ext --inplace > > ./nfs4server.py > > > > on the client, > > mount -o vers=4 SERVER:/ /mnt > > touch /mnt/a > > nfs4_getfacl /mnt/a > > > > With this new patch, you will get a general protection fault in > > _copy_from_pages(). > > Is this on a kernel with commit 519d3959e30a98f8e135e7a16647c10af5ad63d5 > (NFSv4: Fix pointer arithmetic in decode_getacl) applied? > Yes. Sachin Prabhu ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Avoid array overflow in __nfs4_get_acl_uncached 2012-08-24 21:51 ` Sachin Prabhu @ 2012-08-24 22:02 ` Myklebust, Trond 2012-08-25 23:31 ` Sachin Prabhu 0 siblings, 1 reply; 13+ messages in thread From: Myklebust, Trond @ 2012-08-24 22:02 UTC (permalink / raw) To: Sachin Prabhu; +Cc: Linux NFS mailing list T24gRnJpLCAyMDEyLTA4LTI0IGF0IDIyOjUxICswMTAwLCBTYWNoaW4gUHJhYmh1IHdyb3RlOg0K PiBPbiBGcmksIDIwMTItMDgtMjQgYXQgMjE6MzggKzAwMDAsIE15a2xlYnVzdCwgVHJvbmQgd3Jv dGU6DQo+ID4gT24gRnJpLCAyMDEyLTA4LTI0IGF0IDIyOjMxICswMTAwLCBTYWNoaW4gUHJhYmh1 IHdyb3RlOg0KPiA+ID4gT24gRnJpLCAyMDEyLTA4LTI0IGF0IDE1OjA3ICswMDAwLCBNeWtsZWJ1 c3QsIFRyb25kIHdyb3RlOg0KPiA+ID4gPiBPbiBGcmksIDIwMTItMDgtMjQgYXQgMTU6MTYgKzAx MDAsIFNhY2hpbiBQcmFiaHUgd3JvdGU6DQo+ID4gPiA+ID4gVGhpcyBmaXhlcyBhIGJ1ZyBpbnRy b2R1Y2VkIGJ5IGNvbW1pdA0KPiA+ID4gPiA+IDVhMDA2ODk5MzBhYjk3NWZkZDFiMzdiMDM0NDc1 MDE3ZTQ2MGNmMmENCj4gPiA+ID4gPiBUaGUgcGF0Y2ggYWRkcyBhbiBleHRyYSBwYWdlIHRvIG5w YWdlcyB0byBob2xkIHRoZSBiaXRtYXAgcmV0dXJuZWQgYnkNCj4gPiA+ID4gPiB0aGUgc2VydmVy Lg0KPiA+ID4gPiA+IA0KPiA+ID4gPiA+IEJydWNlIEZpZWxkcyBwb2ludGVkIG91dCB0aGF0IHRo ZSBjaGFuZ2VzIGludHJvZHVjZWQgYnkgdGhlIHBhdGNoIHdpbGwNCj4gPiA+ID4gPiBjYXVzZSB0 aGUgYXJyYXkgbnBhZ2VzIHRvIG92ZXJmbG93IGlmIGEgYnVmZmVyIG9mIHNpemUgZ3JlYXRlciB0 aGFuIG9yDQo+ID4gPiA+ID4gZXF1YWwgdG8gWEFUVFJfU0laRV9NQVggaXMgcGFzc2VkIHRvIF9f bmZzNF9nZXRfYWNsX3VuY2FjaGVkKCkNCj4gPiA+ID4gDQo+ID4gPiA+IEknZCB0aGluayB0aGF0 IHRoZSByaWdodCB0aGluZyB0byBkbyBoZXJlIGlzIHJhdGhlciB0byBhZGQgYXBwcm9wcmlhdGUN Cj4gPiA+ID4gYnVmZmVyIG92ZXJmbG93IGNoZWNrcy4gSG93IGFib3V0IHNvbWV0aGluZyBsaWtl IHRoZSBmb2xsb3dpbmc/DQo+ID4gPiA+IA0KPiA+ID4gPiA4PC0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tDQo+ID4gPiA+IEZyb20g N2MzNWNlMjIwOTI0MTgyMjg0YWVhOWY4YWVjMzliMGQ5OTE2MDBkZiBNb24gU2VwIDE3IDAwOjAw OjAwIDIwMDENCj4gPiA+ID4gRnJvbTogVHJvbmQgTXlrbGVidXN0IDxUcm9uZC5NeWtsZWJ1c3RA bmV0YXBwLmNvbT4NCj4gPiA+ID4gRGF0ZTogRnJpLCAyNCBBdWcgMjAxMiAxMDo1OToyNSAtMDQw MA0KPiA+ID4gPiBTdWJqZWN0OiBbUEFUQ0hdIE5GU3Y0OiBGaXggcmFuZ2UgY2hlY2tpbmcgaW4g X19uZnM0X2dldF9hY2xfdW5jYWNoZWQgYW5kDQo+ID4gPiA+ICBfX25mczRfcHJvY19zZXRfYWNs DQo+ID4gPiA+IA0KPiA+ID4gPiBFbnN1cmUgdGhhdCB0aGUgdXNlciBzdXBwbGllZCBidWZmZXIg c2l6ZSBkb2Vzbid0IGNhdXNlIHVzIHRvIG92ZXJmbG93DQo+ID4gPiA+IHRoZSAncGFnZXMnIGFy cmF5Lg0KPiA+ID4gPiANCj4gPiA+ID4gQWxzbyBmaXggdXAgc29tZSBjb25mdXNpb24gYmV0d2Vl biB0aGUgdXNlIG9mIFBBR0VfU0laRSBhbmQNCj4gPiA+ID4gUEFHRV9DQUNIRV9TSVpFIHdoZW4g Y2FsY3VsYXRpbmcgYnVmZmVyIHNpemVzLiBXZSdyZSBub3QgdXNpbmcNCj4gPiA+ID4gdGhlIHBh Z2UgY2FjaGUgZm9yIGFueXRoaW5nIGhlcmUuDQo+ID4gPiA+IA0KPiA+ID4gPiBTaWduZWQtb2Zm LWJ5OiBUcm9uZCBNeWtsZWJ1c3QgPFRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tPg0KPiA+ID4g DQo+ID4gPiBUaGlzIHBhdGNoIGlzIHN1c2NlcHRpYmxlIHRvIHRoZSBwcm9ibGVtIGRlc2NyaWJl ZCBpbiBjb21taXQNCj4gPiA+IDVhMDA2ODk5MzBhYjk3NWZkZDFiMzdiMDM0NDc1MDE3ZTQ2MGNm MmENCj4gPiA+IA0KPiA+ID4gVGhpcyBjYW4gYmUgZGVtb25zdHJhdGVkIGJ5IHRoZSBmb2xsb3dp bmcgcGF0Y2ggdG8gcHluZnMgd2hpY2ggcGFkcyB0aGUNCj4gPiA+IGJpdG1hcCB3aXRoIDEwMDAg ZXh0cmEgZWxlbWVudHMuDQo+ID4gPiANCj4gPiA+IFRvIHJlcHJvZHVjZSwgb24gdGhlIHNlcnZl cg0KPiA+ID4gY2QgbmV3cHluZnMvbmZzNC4wLw0KPiA+ID4gLi9zZXR1cC5weSBidWlsZF9leHQg LS1pbnBsYWNlDQo+ID4gPiAuL25mczRzZXJ2ZXIucHkNCj4gPiA+IA0KPiA+ID4gb24gdGhlIGNs aWVudCwgDQo+ID4gPiBtb3VudCAtbyB2ZXJzPTQgU0VSVkVSOi8gL21udA0KPiA+ID4gdG91Y2gg L21udC9hDQo+ID4gPiBuZnM0X2dldGZhY2wgL21udC9hDQo+ID4gPiANCj4gPiA+IFdpdGggdGhp cyBuZXcgcGF0Y2gsIHlvdSB3aWxsIGdldCBhIGdlbmVyYWwgcHJvdGVjdGlvbiBmYXVsdCBpbg0K PiA+ID4gX2NvcHlfZnJvbV9wYWdlcygpLg0KPiA+IA0KPiA+IElzIHRoaXMgb24gYSBrZXJuZWwg d2l0aCBjb21taXQgNTE5ZDM5NTllMzBhOThmOGUxMzVlN2ExNjY0N2MxMGFmNWFkNjNkNQ0KPiA+ IChORlN2NDogRml4IHBvaW50ZXIgYXJpdGhtZXRpYyBpbiBkZWNvZGVfZ2V0YWNsKSBhcHBsaWVk Pw0KPiA+IA0KPiANCj4gWWVzLg0KDQpPSywgc28gY2FuIHlvdSBsb29rIGludG8gd2hpY2ggcGFy YW1ldGVycyBhcmUgaW5jb3JyZWN0IGFuZCB3aHk/IFRoZQ0KY2hlY2tzIGluIGRlY29kZV9nZXRh Y2wgYXJlIHN1cHBvc2VkIGVuc3VyZSB0aGF0IHdlIGRvbid0IG92ZXJmbG93IHRoZQ0KeGRyLT5i dWYtPnBhZ2VfbGVuLCBzbyBpZiB0aG9zZSBhcmUgaW5zdWZmaWNpZW50LCB0aGVuIEknZCBsaWtl IHRvDQp1bmRlcnN0YW5kIHdoeS4NCg0KLS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBj bGllbnQgbWFpbnRhaW5lcg0KDQpOZXRBcHANClRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tDQp3 d3cubmV0YXBwLmNvbQ0KDQo= ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Avoid array overflow in __nfs4_get_acl_uncached 2012-08-24 22:02 ` Myklebust, Trond @ 2012-08-25 23:31 ` Sachin Prabhu 2012-08-26 18:57 ` Myklebust, Trond 0 siblings, 1 reply; 13+ messages in thread From: Sachin Prabhu @ 2012-08-25 23:31 UTC (permalink / raw) To: Myklebust, Trond; +Cc: Linux NFS mailing list On Fri, 2012-08-24 at 22:02 +0000, Myklebust, Trond wrote: > On Fri, 2012-08-24 at 22:51 +0100, Sachin Prabhu wrote: > > On Fri, 2012-08-24 at 21:38 +0000, Myklebust, Trond wrote: > > > On Fri, 2012-08-24 at 22:31 +0100, Sachin Prabhu wrote: > > > > On Fri, 2012-08-24 at 15:07 +0000, Myklebust, Trond wrote: > > > > > On Fri, 2012-08-24 at 15:16 +0100, Sachin Prabhu wrote: > > > > > > This fixes a bug introduced by commit > > > > > > 5a00689930ab975fdd1b37b034475017e460cf2a > > > > > > The patch adds an extra page to npages to hold the bitmap returned by > > > > > > the server. > > > > > > > > > > > > Bruce Fields pointed out that the changes introduced by the patch will > > > > > > cause the array npages to overflow if a buffer of size greater than or > > > > > > equal to XATTR_SIZE_MAX is passed to __nfs4_get_acl_uncached() > > > > > > > > > > I'd think that the right thing to do here is rather to add appropriate > > > > > buffer overflow checks. How about something like the following? > > > > > > > > > > 8<-------------------------------------------------------------- > > > > > From 7c35ce220924182284aea9f8aec39b0d991600df Mon Sep 17 00:00:00 2001 > > > > > From: Trond Myklebust <Trond.Myklebust@netapp.com> > > > > > Date: Fri, 24 Aug 2012 10:59:25 -0400 > > > > > Subject: [PATCH] NFSv4: Fix range checking in __nfs4_get_acl_uncached and > > > > > __nfs4_proc_set_acl > > > > > > > > > > Ensure that the user supplied buffer size doesn't cause us to overflow > > > > > the 'pages' array. > > > > > > > > > > Also fix up some confusion between the use of PAGE_SIZE and > > > > > PAGE_CACHE_SIZE when calculating buffer sizes. We're not using > > > > > the page cache for anything here. > > > > > > > > > > Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com> > > > > > > > > This patch is susceptible to the problem described in commit > > > > 5a00689930ab975fdd1b37b034475017e460cf2a > > > > > > > > This can be demonstrated by the following patch to pynfs which pads the > > > > bitmap with 1000 extra elements. > > > > > > > > To reproduce, on the server > > > > cd newpynfs/nfs4.0/ > > > > ./setup.py build_ext --inplace > > > > ./nfs4server.py > > > > > > > > on the client, > > > > mount -o vers=4 SERVER:/ /mnt > > > > touch /mnt/a > > > > nfs4_getfacl /mnt/a > > > > > > > > With this new patch, you will get a general protection fault in > > > > _copy_from_pages(). > > > > > > Is this on a kernel with commit 519d3959e30a98f8e135e7a16647c10af5ad63d5 > > > (NFSv4: Fix pointer arithmetic in decode_getacl) applied? > > > > > > > Yes. > > OK, so can you look into which parameters are incorrect and why? The > checks in decode_getacl are supposed ensure that we don't overflow the > xdr->buf->page_len, so if those are insufficient, then I'd like to > understand why. > The problem is seen because we do not check to see if the acl_len + bitmap size is contained within the page buffer we allocate for it. It will fail if bitmap array size + acl length attribute size + ACLs > pages allocated for it. In this case, when we call nfs4_write_cached_acl() to write the returned ACLs into cache, we call _copy_from_pages() to read from ie. We call nfs4_write_cached_acl nfs4_write_cached_acl(inode, pages, res.acl_data_offset, acl_len); which in turn calls static void nfs4_write_cached_acl(struct inode *inode, struct page **pages, size_t pgbase, size_t acl_len) { .. _copy_from_pages(acl->data, pages, pgbase, acl_len); .. } A simple example is if the ACL size is just less than a PAGE_SIZE but large enough so that ACL + Bitmap crosses a PAGE_SIZE, it will fail. Another example is if the server sends a large bitmap array along with smaller ACL data causing the bitmap array + ACL to go past the allocated page, it will fail again. We exploit the bug using the second example given above. We configured pynfs to append 1000 extra elements to the bitmap array which results in BITMAP+ACL size to be little over a PAGE_SIZE. This leads to a General Protection Fault in _copy_from_pages(). Using a simple printk in __nfs4_get_acl_uncached, I see the following values args.acl_len=4096, res.acl_data_offset=4012, res.acl_len=156 We end up calling _copy_from_pages(acl->data, pages, 4012, 156); When tested _without_ this patch, where the minimum npages sent is 2, and the bitmap padding is set so that we cross 2*PAGESIZE, we fail in decode_attr_bitmap-> xdr_decode_inline because we cross buffer boundary and the allocated scratch page is insufficient to read the bitmap returned. Although it prevent the crash, it is not the ideal solution to the problem. Sachin Prabhu ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Avoid array overflow in __nfs4_get_acl_uncached 2012-08-25 23:31 ` Sachin Prabhu @ 2012-08-26 18:57 ` Myklebust, Trond 2012-08-28 14:09 ` Sachin Prabhu 0 siblings, 1 reply; 13+ messages in thread From: Myklebust, Trond @ 2012-08-26 18:57 UTC (permalink / raw) To: Sachin Prabhu; +Cc: Linux NFS mailing list T24gU3VuLCAyMDEyLTA4LTI2IGF0IDAwOjMxICswMTAwLCBTYWNoaW4gUHJhYmh1IHdyb3RlOg0K PiBPbiBGcmksIDIwMTItMDgtMjQgYXQgMjI6MDIgKzAwMDAsIE15a2xlYnVzdCwgVHJvbmQgd3Jv dGU6DQo+ID4gT24gRnJpLCAyMDEyLTA4LTI0IGF0IDIyOjUxICswMTAwLCBTYWNoaW4gUHJhYmh1 IHdyb3RlOg0KPiA+ID4gT24gRnJpLCAyMDEyLTA4LTI0IGF0IDIxOjM4ICswMDAwLCBNeWtsZWJ1 c3QsIFRyb25kIHdyb3RlOg0KPiA+ID4gPiBPbiBGcmksIDIwMTItMDgtMjQgYXQgMjI6MzEgKzAx MDAsIFNhY2hpbiBQcmFiaHUgd3JvdGU6DQo+ID4gPiA+ID4gT24gRnJpLCAyMDEyLTA4LTI0IGF0 IDE1OjA3ICswMDAwLCBNeWtsZWJ1c3QsIFRyb25kIHdyb3RlOg0KPiA+ID4gPiA+ID4gT24gRnJp LCAyMDEyLTA4LTI0IGF0IDE1OjE2ICswMTAwLCBTYWNoaW4gUHJhYmh1IHdyb3RlOg0KPiA+ID4g PiA+ID4gPiBUaGlzIGZpeGVzIGEgYnVnIGludHJvZHVjZWQgYnkgY29tbWl0DQo+ID4gPiA+ID4g PiA+IDVhMDA2ODk5MzBhYjk3NWZkZDFiMzdiMDM0NDc1MDE3ZTQ2MGNmMmENCj4gPiA+ID4gPiA+ ID4gVGhlIHBhdGNoIGFkZHMgYW4gZXh0cmEgcGFnZSB0byBucGFnZXMgdG8gaG9sZCB0aGUgYml0 bWFwIHJldHVybmVkIGJ5DQo+ID4gPiA+ID4gPiA+IHRoZSBzZXJ2ZXIuDQo+ID4gPiA+ID4gPiA+ IA0KPiA+ID4gPiA+ID4gPiBCcnVjZSBGaWVsZHMgcG9pbnRlZCBvdXQgdGhhdCB0aGUgY2hhbmdl cyBpbnRyb2R1Y2VkIGJ5IHRoZSBwYXRjaCB3aWxsDQo+ID4gPiA+ID4gPiA+IGNhdXNlIHRoZSBh cnJheSBucGFnZXMgdG8gb3ZlcmZsb3cgaWYgYSBidWZmZXIgb2Ygc2l6ZSBncmVhdGVyIHRoYW4g b3INCj4gPiA+ID4gPiA+ID4gZXF1YWwgdG8gWEFUVFJfU0laRV9NQVggaXMgcGFzc2VkIHRvIF9f bmZzNF9nZXRfYWNsX3VuY2FjaGVkKCkNCj4gPiA+ID4gPiA+IA0KPiA+ID4gPiA+ID4gSSdkIHRo aW5rIHRoYXQgdGhlIHJpZ2h0IHRoaW5nIHRvIGRvIGhlcmUgaXMgcmF0aGVyIHRvIGFkZCBhcHBy b3ByaWF0ZQ0KPiA+ID4gPiA+ID4gYnVmZmVyIG92ZXJmbG93IGNoZWNrcy4gSG93IGFib3V0IHNv bWV0aGluZyBsaWtlIHRoZSBmb2xsb3dpbmc/DQo+ID4gPiA+ID4gPiANCj4gPiA+ID4gPiA+IDg8 LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0NCj4gPiA+ID4gPiA+IEZyb20gN2MzNWNlMjIwOTI0MTgyMjg0YWVhOWY4YWVjMzliMGQ5 OTE2MDBkZiBNb24gU2VwIDE3IDAwOjAwOjAwIDIwMDENCj4gPiA+ID4gPiA+IEZyb206IFRyb25k IE15a2xlYnVzdCA8VHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20+DQo+ID4gPiA+ID4gPiBEYXRl OiBGcmksIDI0IEF1ZyAyMDEyIDEwOjU5OjI1IC0wNDAwDQo+ID4gPiA+ID4gPiBTdWJqZWN0OiBb UEFUQ0hdIE5GU3Y0OiBGaXggcmFuZ2UgY2hlY2tpbmcgaW4gX19uZnM0X2dldF9hY2xfdW5jYWNo ZWQgYW5kDQo+ID4gPiA+ID4gPiAgX19uZnM0X3Byb2Nfc2V0X2FjbA0KPiA+ID4gPiA+ID4gDQo+ ID4gPiA+ID4gPiBFbnN1cmUgdGhhdCB0aGUgdXNlciBzdXBwbGllZCBidWZmZXIgc2l6ZSBkb2Vz bid0IGNhdXNlIHVzIHRvIG92ZXJmbG93DQo+ID4gPiA+ID4gPiB0aGUgJ3BhZ2VzJyBhcnJheS4N Cj4gPiA+ID4gPiA+IA0KPiA+ID4gPiA+ID4gQWxzbyBmaXggdXAgc29tZSBjb25mdXNpb24gYmV0 d2VlbiB0aGUgdXNlIG9mIFBBR0VfU0laRSBhbmQNCj4gPiA+ID4gPiA+IFBBR0VfQ0FDSEVfU0la RSB3aGVuIGNhbGN1bGF0aW5nIGJ1ZmZlciBzaXplcy4gV2UncmUgbm90IHVzaW5nDQo+ID4gPiA+ ID4gPiB0aGUgcGFnZSBjYWNoZSBmb3IgYW55dGhpbmcgaGVyZS4NCj4gPiA+ID4gPiA+IA0KPiA+ ID4gPiA+ID4gU2lnbmVkLW9mZi1ieTogVHJvbmQgTXlrbGVidXN0IDxUcm9uZC5NeWtsZWJ1c3RA bmV0YXBwLmNvbT4NCj4gPiA+ID4gPiANCj4gPiA+ID4gPiBUaGlzIHBhdGNoIGlzIHN1c2NlcHRp YmxlIHRvIHRoZSBwcm9ibGVtIGRlc2NyaWJlZCBpbiBjb21taXQNCj4gPiA+ID4gPiA1YTAwNjg5 OTMwYWI5NzVmZGQxYjM3YjAzNDQ3NTAxN2U0NjBjZjJhDQo+ID4gPiA+ID4gDQo+ID4gPiA+ID4g VGhpcyBjYW4gYmUgZGVtb25zdHJhdGVkIGJ5IHRoZSBmb2xsb3dpbmcgcGF0Y2ggdG8gcHluZnMg d2hpY2ggcGFkcyB0aGUNCj4gPiA+ID4gPiBiaXRtYXAgd2l0aCAxMDAwIGV4dHJhIGVsZW1lbnRz Lg0KPiA+ID4gPiA+IA0KPiA+ID4gPiA+IFRvIHJlcHJvZHVjZSwgb24gdGhlIHNlcnZlcg0KPiA+ ID4gPiA+IGNkIG5ld3B5bmZzL25mczQuMC8NCj4gPiA+ID4gPiAuL3NldHVwLnB5IGJ1aWxkX2V4 dCAtLWlucGxhY2UNCj4gPiA+ID4gPiAuL25mczRzZXJ2ZXIucHkNCj4gPiA+ID4gPiANCj4gPiA+ ID4gPiBvbiB0aGUgY2xpZW50LCANCj4gPiA+ID4gPiBtb3VudCAtbyB2ZXJzPTQgU0VSVkVSOi8g L21udA0KPiA+ID4gPiA+IHRvdWNoIC9tbnQvYQ0KPiA+ID4gPiA+IG5mczRfZ2V0ZmFjbCAvbW50 L2ENCj4gPiA+ID4gPiANCj4gPiA+ID4gPiBXaXRoIHRoaXMgbmV3IHBhdGNoLCB5b3Ugd2lsbCBn ZXQgYSBnZW5lcmFsIHByb3RlY3Rpb24gZmF1bHQgaW4NCj4gPiA+ID4gPiBfY29weV9mcm9tX3Bh Z2VzKCkuDQo+ID4gPiA+IA0KPiA+ID4gPiBJcyB0aGlzIG9uIGEga2VybmVsIHdpdGggY29tbWl0 IDUxOWQzOTU5ZTMwYTk4ZjhlMTM1ZTdhMTY2NDdjMTBhZjVhZDYzZDUNCj4gPiA+ID4gKE5GU3Y0 OiBGaXggcG9pbnRlciBhcml0aG1ldGljIGluIGRlY29kZV9nZXRhY2wpIGFwcGxpZWQ/DQo+ID4g PiA+IA0KPiA+ID4gDQo+ID4gPiBZZXMuDQo+ID4gDQo+ID4gT0ssIHNvIGNhbiB5b3UgbG9vayBp bnRvIHdoaWNoIHBhcmFtZXRlcnMgYXJlIGluY29ycmVjdCBhbmQgd2h5PyBUaGUNCj4gPiBjaGVj a3MgaW4gZGVjb2RlX2dldGFjbCBhcmUgc3VwcG9zZWQgZW5zdXJlIHRoYXQgd2UgZG9uJ3Qgb3Zl cmZsb3cgdGhlDQo+ID4geGRyLT5idWYtPnBhZ2VfbGVuLCBzbyBpZiB0aG9zZSBhcmUgaW5zdWZm aWNpZW50LCB0aGVuIEknZCBsaWtlIHRvDQo+ID4gdW5kZXJzdGFuZCB3aHkuDQo+ID4gDQo+IA0K PiBUaGUgcHJvYmxlbSBpcyBzZWVuIGJlY2F1c2Ugd2UgZG8gbm90IGNoZWNrIHRvIHNlZSBpZiB0 aGUgYWNsX2xlbiArDQo+IGJpdG1hcCBzaXplIGlzIGNvbnRhaW5lZCB3aXRoaW4gdGhlIHBhZ2Ug YnVmZmVyIHdlIGFsbG9jYXRlIGZvciBpdC4gDQo+IA0KPiBJdCB3aWxsIGZhaWwgaWYNCj4gYml0 bWFwIGFycmF5IHNpemUgKyBhY2wgbGVuZ3RoIGF0dHJpYnV0ZSBzaXplICsgQUNMcyA+IHBhZ2Vz IGFsbG9jYXRlZA0KPiBmb3IgaXQuICANCj4gDQo+IEluIHRoaXMgY2FzZSwgd2hlbiB3ZSBjYWxs IG5mczRfd3JpdGVfY2FjaGVkX2FjbCgpIHRvIHdyaXRlIHRoZSByZXR1cm5lZA0KPiBBQ0xzIGlu dG8gY2FjaGUsIHdlIGNhbGwgX2NvcHlfZnJvbV9wYWdlcygpIHRvIHJlYWQgZnJvbSANCj4gaWUu IFdlIGNhbGwgbmZzNF93cml0ZV9jYWNoZWRfYWNsDQo+ICAgICAgICAgICAgICAgICBuZnM0X3dy aXRlX2NhY2hlZF9hY2woaW5vZGUsIHBhZ2VzLCByZXMuYWNsX2RhdGFfb2Zmc2V0LA0KPiAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGFjbF9sZW4pOw0KPiB3aGljaCBpbiB0 dXJuIGNhbGxzDQo+IHN0YXRpYyB2b2lkIG5mczRfd3JpdGVfY2FjaGVkX2FjbChzdHJ1Y3QgaW5v ZGUgKmlub2RlLCBzdHJ1Y3QgcGFnZQ0KPiAqKnBhZ2VzLCBzaXplX3QgcGdiYXNlLCBzaXplX3Qg YWNsX2xlbikNCj4gew0KPiAuLg0KPiAgICAgICAgICAgICAgICAgX2NvcHlfZnJvbV9wYWdlcyhh Y2wtPmRhdGEsIHBhZ2VzLCBwZ2Jhc2UsIGFjbF9sZW4pOw0KPiAuLg0KPiB9DQoNClJpZ2h0LiBU aGF0IGlzIGJhc2ljYWxseSBkdWUgdG8gaW5jb21wbGV0ZSBidWZmZXIgY2hlY2tpbmcgaW4NCm5m czRfd3JpdGVfY2FjaGVkX2FjbCgpIGl0c2VsZi4gV2UgY2FuJ3QgY2F0Y2ggdGhhdCBjYXNlIHdo ZW4NCk5GUzRfQUNMX0xFTl9SRVFVRVNUIGlzIHNldCwgYmVjYXVzZSBkZWNvZGVfYWNsIGlnbm9y ZXMgdGhlIGNoZWNrLg0KDQpPSy4gSG93IGFib3V0IHRoaXMgcGF0Y2gsIHdoaWNoIGFwcGxpZXMg b24gdG9wIG9mIHRoZSBvbmUgSSBzZW50IHlvdT8NCjg8LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0NCkZyb20gMjJjOWUyNDc1 NTYwZWRkYTkyNWY5NzFhMmEwMmJhYzU4NTM2NDE0YiBNb24gU2VwIDE3IDAwOjAwOjAwIDIwMDEN CkZyb206IFRyb25kIE15a2xlYnVzdCA8VHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20+DQpEYXRl OiBTdW4sIDI2IEF1ZyAyMDEyIDExOjQ0OjQzIC0wNzAwDQpTdWJqZWN0OiBbUEFUQ0hdIE5GU3Y0 OiBBZGQgYnVmZmVyIG92ZXJmbG93IGNoZWNraW5nIHRvIG5mczRfd3JpdGVfY2FjaGVkX2FjbA0K DQpDdXJyZW50bHksIHRoZSBidWZmZXIgb3ZlcmZsb3cgY2hlY2tpbmcgaXMgZG9uZSBpbmNvcnJl Y3RseSBieSB0aGUNCmNhbGxlci4gTW92ZSB0aGUgb3ZlcmZsb3cgY2hlY2tpbmcgaW50byBuZnM0 X3dyaXRlX2NhY2hlZF9hY2wgaXRzZWxmDQpmb3Igcm9idXN0bmVzcywgYW5kIGZpeCB0aGUgb3Zl cmZsb3cgY2FsY3VsYXRpb24gdG8gdGFrZSBpbnRvIGFjY291bnQNCnRoZSAncGdiYXNlJyBhcmd1 bWVudCB0byBfY29weV9mcm9tX3BhZ2VzLg0KDQpTaWduZWQtb2ZmLWJ5OiBUcm9uZCBNeWtsZWJ1 c3QgPFRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tPg0KLS0tDQogZnMvbmZzL25mczRwcm9jLmMg fCAxMiArKysrKy0tLS0tLS0NCiAxIGZpbGUgY2hhbmdlZCwgNSBpbnNlcnRpb25zKCspLCA3IGRl bGV0aW9ucygtKQ0KDQpkaWZmIC0tZ2l0IGEvZnMvbmZzL25mczRwcm9jLmMgYi9mcy9uZnMvbmZz NHByb2MuYw0KaW5kZXggNjU0ZGMzOC4uYWY0ZWJjMyAxMDA2NDQNCi0tLSBhL2ZzL25mcy9uZnM0 cHJvYy5jDQorKysgYi9mcy9uZnMvbmZzNHByb2MuYw0KQEAgLTM3MzQsMTIgKzM3MzQsMTMgQEAg b3V0Og0KIAlyZXR1cm4gcmV0Ow0KIH0NCiANCi1zdGF0aWMgdm9pZCBuZnM0X3dyaXRlX2NhY2hl ZF9hY2woc3RydWN0IGlub2RlICppbm9kZSwgc3RydWN0IHBhZ2UgKipwYWdlcywgc2l6ZV90IHBn YmFzZSwgc2l6ZV90IGFjbF9sZW4pDQorc3RhdGljIHZvaWQgbmZzNF93cml0ZV9jYWNoZWRfYWNs KHN0cnVjdCBpbm9kZSAqaW5vZGUsIHN0cnVjdCBwYWdlICoqcGFnZXMsDQorCQlzaXplX3Qgc3Jj bGVuLCBzaXplX3QgcGdiYXNlLCBzaXplX3QgYWNsX2xlbikNCiB7DQogCXN0cnVjdCBuZnM0X2Nh Y2hlZF9hY2wgKmFjbDsNCiAJc2l6ZV90IGJ1ZmxlbiA9IHNpemVvZigqYWNsKSArIGFjbF9sZW47 DQogDQotCWlmIChwYWdlcyAmJiBidWZsZW4gPD0gUEFHRV9TSVpFKSB7DQorCWlmIChidWZsZW4g PD0gUEFHRV9TSVpFICYmIHNyY2xlbiA8PSBwZ2Jhc2UgKyBhY2xfbGVuKSB7DQogCQlhY2wgPSBr bWFsbG9jKGJ1ZmxlbiwgR0ZQX0tFUk5FTCk7DQogCQlpZiAoYWNsID09IE5VTEwpDQogCQkJZ290 byBvdXQ7DQpAQCAtMzgyMCwxMSArMzgyMSw4IEBAIHN0YXRpYyBzc2l6ZV90IF9fbmZzNF9nZXRf YWNsX3VuY2FjaGVkKHN0cnVjdCBpbm9kZSAqaW5vZGUsIHZvaWQgKmJ1Ziwgc2l6ZV90IGJ1DQog CQlnb3RvIG91dF9mcmVlOw0KIA0KIAlhY2xfbGVuID0gcmVzLmFjbF9sZW47DQotCWlmIChhY2xf bGVuID4gYXJncy5hY2xfbGVuKQ0KLQkJbmZzNF93cml0ZV9jYWNoZWRfYWNsKGlub2RlLCBOVUxM LCAwLCBhY2xfbGVuKTsNCi0JZWxzZQ0KLQkJbmZzNF93cml0ZV9jYWNoZWRfYWNsKGlub2RlLCBw YWdlcywgcmVzLmFjbF9kYXRhX29mZnNldCwNCi0JCQkJICAgICAgYWNsX2xlbik7DQorCW5mczRf d3JpdGVfY2FjaGVkX2FjbChpbm9kZSwgcGFnZXMsIGFyZ3MuYWNsX2xlbiwNCisJCQlyZXMuYWNs X2RhdGFfb2Zmc2V0LCByZXMuYWNsX2xlbik7DQogCWlmIChidWYpIHsNCiAJCXJldCA9IC1FUkFO R0U7DQogCQlpZiAoYWNsX2xlbiA+IGJ1ZmxlbikNCi0tIA0KMS43LjExLjQNCg0KDQotLSANClRy b25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyDQoNCk5ldEFwcA0KVHJv bmQuTXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5uZXRhcHAuY29tDQoNCg== ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Avoid array overflow in __nfs4_get_acl_uncached 2012-08-26 18:57 ` Myklebust, Trond @ 2012-08-28 14:09 ` Sachin Prabhu 2012-09-03 19:11 ` Myklebust, Trond 0 siblings, 1 reply; 13+ messages in thread From: Sachin Prabhu @ 2012-08-28 14:09 UTC (permalink / raw) To: Myklebust, Trond; +Cc: Linux NFS mailing list > OK. How about this patch, which applies on top of the one I sent you? > 8<----------------------------------------------------------------- > From 22c9e2475560edda925f971a2a02bac58536414b Mon Sep 17 00:00:00 2001 > From: Trond Myklebust <Trond.Myklebust@netapp.com> > Date: Sun, 26 Aug 2012 11:44:43 -0700 > Subject: [PATCH] NFSv4: Add buffer overflow checking to nfs4_write_cached_acl > > Currently, the buffer overflow checking is done incorrectly by the > caller. Move the overflow checking into nfs4_write_cached_acl itself > for robustness, and fix the overflow calculation to take into account > the 'pgbase' argument to _copy_from_pages. > > Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com> > --- > fs/nfs/nfs4proc.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index 654dc38..af4ebc3 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -3734,12 +3734,13 @@ out: > return ret; > } > > -static void nfs4_write_cached_acl(struct inode *inode, struct page **pages, size_t pgbase, size_t acl_len) > +static void nfs4_write_cached_acl(struct inode *inode, struct page **pages, > + size_t srclen, size_t pgbase, size_t acl_len) > { > struct nfs4_cached_acl *acl; > size_t buflen = sizeof(*acl) + acl_len; > > - if (pages && buflen <= PAGE_SIZE) { > + if (buflen <= PAGE_SIZE && srclen <= pgbase + acl_len) { > acl = kmalloc(buflen, GFP_KERNEL); > if (acl == NULL) > goto out; > @@ -3820,11 +3821,8 @@ static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu > goto out_free; > > acl_len = res.acl_len; > - if (acl_len > args.acl_len) > - nfs4_write_cached_acl(inode, NULL, 0, acl_len); > - else > - nfs4_write_cached_acl(inode, pages, res.acl_data_offset, > - acl_len); > + nfs4_write_cached_acl(inode, pages, args.acl_len, > + res.acl_data_offset, res.acl_len); > if (buf) { > ret = -ERANGE; > if (acl_len > buflen) > -- > 1.7.11.4 I encountered 2 problems. 1) The if condition should be srclen >= pgbase + acl_len 2) There is a second _copy_from_pages which copies to the the acl to the passed buffer in __nfs4_get_acl_uncached(). I've attached a patch below. ----- >From ec84527625b3847a0623410d532253dd919e119a Mon Sep 17 00:00:00 2001 From: Sachin Prabhu <sprabhu@redhat.com> Date: Tue, 28 Aug 2012 15:01:13 +0100 Subject: [PATCH] NFSv4: Check for buffer overflow in __nfs4_get_acl_uncached Check for buffer overflow in __nfs4_get_acl_uncached when using _copy_from_pages to copy to the buffer passed in the argument. Also fix if condition in nfs4_write_cached_acl() from the previous post. Signed-off-by: Sachin Prabhu --- fs/nfs/nfs4proc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 580b0b1..cb8748d 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -3740,7 +3740,7 @@ static void nfs4_write_cached_acl(struct inode *inode, struct page **pages, struct nfs4_cached_acl *acl; size_t buflen = sizeof(*acl) + acl_len; - if (buflen <= PAGE_SIZE && srclen <= pgbase + acl_len) { + if (buflen <= PAGE_SIZE && srclen >= pgbase + acl_len) { acl = kmalloc(buflen, GFP_KERNEL); if (acl == NULL) goto out; @@ -3825,7 +3825,7 @@ static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu res.acl_data_offset, res.acl_len); if (buf) { ret = -ERANGE; - if (acl_len > buflen) + if (acl_len > buflen || args.acl_len < res.acl_data_offset + res.acl_len) goto out_free; _copy_from_pages(buf, pages, res.acl_data_offset, acl_len); -- 1.7.11.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] Avoid array overflow in __nfs4_get_acl_uncached 2012-08-28 14:09 ` Sachin Prabhu @ 2012-09-03 19:11 ` Myklebust, Trond 2012-09-06 14:46 ` Sachin Prabhu 0 siblings, 1 reply; 13+ messages in thread From: Myklebust, Trond @ 2012-09-03 19:11 UTC (permalink / raw) To: Sachin Prabhu; +Cc: Linux NFS mailing list T24gVHVlLCAyMDEyLTA4LTI4IGF0IDE1OjA5ICswMTAwLCBTYWNoaW4gUHJhYmh1IHdyb3RlOg0K PiA+IE9LLiBIb3cgYWJvdXQgdGhpcyBwYXRjaCwgd2hpY2ggYXBwbGllcyBvbiB0b3Agb2YgdGhl IG9uZSBJIHNlbnQgeW91Pw0KPiA+IDg8LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0NCj4gPiBGcm9tIDIyYzllMjQ3NTU2MGVk ZGE5MjVmOTcxYTJhMDJiYWM1ODUzNjQxNGIgTW9uIFNlcCAxNyAwMDowMDowMCAyMDAxDQo+ID4g RnJvbTogVHJvbmQgTXlrbGVidXN0IDxUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbT4NCj4gPiBE YXRlOiBTdW4sIDI2IEF1ZyAyMDEyIDExOjQ0OjQzIC0wNzAwDQo+ID4gU3ViamVjdDogW1BBVENI XSBORlN2NDogQWRkIGJ1ZmZlciBvdmVyZmxvdyBjaGVja2luZyB0byBuZnM0X3dyaXRlX2NhY2hl ZF9hY2wNCj4gPiANCj4gPiBDdXJyZW50bHksIHRoZSBidWZmZXIgb3ZlcmZsb3cgY2hlY2tpbmcg aXMgZG9uZSBpbmNvcnJlY3RseSBieSB0aGUNCj4gPiBjYWxsZXIuIE1vdmUgdGhlIG92ZXJmbG93 IGNoZWNraW5nIGludG8gbmZzNF93cml0ZV9jYWNoZWRfYWNsIGl0c2VsZg0KPiA+IGZvciByb2J1 c3RuZXNzLCBhbmQgZml4IHRoZSBvdmVyZmxvdyBjYWxjdWxhdGlvbiB0byB0YWtlIGludG8gYWNj b3VudA0KPiA+IHRoZSAncGdiYXNlJyBhcmd1bWVudCB0byBfY29weV9mcm9tX3BhZ2VzLg0KPiA+ IA0KPiA+IFNpZ25lZC1vZmYtYnk6IFRyb25kIE15a2xlYnVzdCA8VHJvbmQuTXlrbGVidXN0QG5l dGFwcC5jb20+DQo+ID4gLS0tDQo+ID4gIGZzL25mcy9uZnM0cHJvYy5jIHwgMTIgKysrKystLS0t LS0tDQo+ID4gIDEgZmlsZSBjaGFuZ2VkLCA1IGluc2VydGlvbnMoKyksIDcgZGVsZXRpb25zKC0p DQo+ID4gDQo+ID4gZGlmZiAtLWdpdCBhL2ZzL25mcy9uZnM0cHJvYy5jIGIvZnMvbmZzL25mczRw cm9jLmMNCj4gPiBpbmRleCA2NTRkYzM4Li5hZjRlYmMzIDEwMDY0NA0KPiA+IC0tLSBhL2ZzL25m cy9uZnM0cHJvYy5jDQo+ID4gKysrIGIvZnMvbmZzL25mczRwcm9jLmMNCj4gPiBAQCAtMzczNCwx MiArMzczNCwxMyBAQCBvdXQ6DQo+ID4gIAlyZXR1cm4gcmV0Ow0KPiA+ICB9DQo+ID4gIA0KPiA+ IC1zdGF0aWMgdm9pZCBuZnM0X3dyaXRlX2NhY2hlZF9hY2woc3RydWN0IGlub2RlICppbm9kZSwg c3RydWN0IHBhZ2UgKipwYWdlcywgc2l6ZV90IHBnYmFzZSwgc2l6ZV90IGFjbF9sZW4pDQo+ID4g K3N0YXRpYyB2b2lkIG5mczRfd3JpdGVfY2FjaGVkX2FjbChzdHJ1Y3QgaW5vZGUgKmlub2RlLCBz dHJ1Y3QgcGFnZSAqKnBhZ2VzLA0KPiA+ICsJCXNpemVfdCBzcmNsZW4sIHNpemVfdCBwZ2Jhc2Us IHNpemVfdCBhY2xfbGVuKQ0KPiA+ICB7DQo+ID4gIAlzdHJ1Y3QgbmZzNF9jYWNoZWRfYWNsICph Y2w7DQo+ID4gIAlzaXplX3QgYnVmbGVuID0gc2l6ZW9mKCphY2wpICsgYWNsX2xlbjsNCj4gPiAg DQo+ID4gLQlpZiAocGFnZXMgJiYgYnVmbGVuIDw9IFBBR0VfU0laRSkgew0KPiA+ICsJaWYgKGJ1 ZmxlbiA8PSBQQUdFX1NJWkUgJiYgc3JjbGVuIDw9IHBnYmFzZSArIGFjbF9sZW4pIHsNCj4gPiAg CQlhY2wgPSBrbWFsbG9jKGJ1ZmxlbiwgR0ZQX0tFUk5FTCk7DQo+ID4gIAkJaWYgKGFjbCA9PSBO VUxMKQ0KPiA+ICAJCQlnb3RvIG91dDsNCj4gPiBAQCAtMzgyMCwxMSArMzgyMSw4IEBAIHN0YXRp YyBzc2l6ZV90IF9fbmZzNF9nZXRfYWNsX3VuY2FjaGVkKHN0cnVjdCBpbm9kZSAqaW5vZGUsIHZv aWQgKmJ1Ziwgc2l6ZV90IGJ1DQo+ID4gIAkJZ290byBvdXRfZnJlZTsNCj4gPiAgDQo+ID4gIAlh Y2xfbGVuID0gcmVzLmFjbF9sZW47DQo+ID4gLQlpZiAoYWNsX2xlbiA+IGFyZ3MuYWNsX2xlbikN Cj4gPiAtCQluZnM0X3dyaXRlX2NhY2hlZF9hY2woaW5vZGUsIE5VTEwsIDAsIGFjbF9sZW4pOw0K PiA+IC0JZWxzZQ0KPiA+IC0JCW5mczRfd3JpdGVfY2FjaGVkX2FjbChpbm9kZSwgcGFnZXMsIHJl cy5hY2xfZGF0YV9vZmZzZXQsDQo+ID4gLQkJCQkgICAgICBhY2xfbGVuKTsNCj4gPiArCW5mczRf d3JpdGVfY2FjaGVkX2FjbChpbm9kZSwgcGFnZXMsIGFyZ3MuYWNsX2xlbiwNCj4gPiArCQkJcmVz LmFjbF9kYXRhX29mZnNldCwgcmVzLmFjbF9sZW4pOw0KPiA+ICAJaWYgKGJ1Zikgew0KPiA+ICAJ CXJldCA9IC1FUkFOR0U7DQo+ID4gIAkJaWYgKGFjbF9sZW4gPiBidWZsZW4pDQo+ID4gLS0gDQo+ ID4gMS43LjExLjQNCj4gDQo+IEkgZW5jb3VudGVyZWQgMiBwcm9ibGVtcy4gDQo+IDEpIFRoZSBp ZiBjb25kaXRpb24gc2hvdWxkIGJlIHNyY2xlbiA+PSBwZ2Jhc2UgKyBhY2xfbGVuDQo+IDIpIFRo ZXJlIGlzIGEgc2Vjb25kIF9jb3B5X2Zyb21fcGFnZXMgd2hpY2ggY29waWVzIHRvIHRoZSB0aGUg YWNsIHRvIHRoZQ0KPiBwYXNzZWQgYnVmZmVyIGluIF9fbmZzNF9nZXRfYWNsX3VuY2FjaGVkKCku DQoNClRoZSBzZWNvbmQgY29weSBmcm9tIHBhZ2VzIHNob3VsZCBhbHJlYWR5IGJlIGNvdmVyZWQg YnkgdGhlIGNoZWNrcyBpbg0KZGVjb2RlX2dldGFjbC4gQWxyaWdodCwgc2luY2UgdGhpcyBpcyBu b3Qgb2J2aW91cywgdGhlbiBjbGVhcmx5IHdlIG5lZWQNCnRvIG1ha2UgaXQgc28uIEhvdyBhYm91 dCB0aGUgZm9sbG93aW5nPw0KODwtLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0NCkZyb20gNTA0MDI0MDI0NWEwNDZiZDU4YzM4 MzgwNmIzZjE2MWVlOGI1ODIzYiBNb24gU2VwIDE3IDAwOjAwOjAwIDIwMDENCkZyb206IFRyb25k IE15a2xlYnVzdCA8VHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20+DQpEYXRlOiBTdW4sIDI2IEF1 ZyAyMDEyIDExOjQ0OjQzIC0wNzAwDQpTdWJqZWN0OiBbUEFUQ0hdIE5GU3Y0OiBGaXggYnVmZmVy IG92ZXJmbG93IGNoZWNraW5nIGluDQogX19uZnM0X2dldF9hY2xfdW5jYWNoZWQNCg0KUGFzcyB0 aGUgY2hlY2tzIG1hZGUgYnkgZGVjb2RlX2dldGFjbCBiYWNrIHRvIF9fbmZzNF9nZXRfYWNsX3Vu Y2FjaGVkDQpzbyB0aGF0IGl0IGtub3dzIGlmIHRoZSBhY2wgaGFzIGJlZW4gdHJ1bmNhdGVkLg0K DQpTaWduZWQtb2ZmLWJ5OiBUcm9uZCBNeWtsZWJ1c3QgPFRyb25kLk15a2xlYnVzdEBuZXRhcHAu Y29tPg0KLS0tDQogZnMvbmZzL25mczRwcm9jLmMgICAgICAgfCAzMSArKysrKysrKysrKystLS0t LS0tLS0tLS0tLS0tLS0tDQogZnMvbmZzL25mczR4ZHIuYyAgICAgICAgfCAxMiArKysrLS0tLS0t LS0NCiBpbmNsdWRlL2xpbnV4L25mc194ZHIuaCB8ICAyICstDQogMyBmaWxlcyBjaGFuZ2VkLCAx NyBpbnNlcnRpb25zKCspLCAyOCBkZWxldGlvbnMoLSkNCg0KZGlmZiAtLWdpdCBhL2ZzL25mcy9u ZnM0cHJvYy5jIGIvZnMvbmZzL25mczRwcm9jLmMNCmluZGV4IDY1NGRjMzguLjc0ZjVjMjYgMTAw NjQ0DQotLS0gYS9mcy9uZnMvbmZzNHByb2MuYw0KKysrIGIvZnMvbmZzL25mczRwcm9jLmMNCkBA IC0zNzM5LDcgKzM3MzksNyBAQCBzdGF0aWMgdm9pZCBuZnM0X3dyaXRlX2NhY2hlZF9hY2woc3Ry dWN0IGlub2RlICppbm9kZSwgc3RydWN0IHBhZ2UgKipwYWdlcywgc2l6ZQ0KIAlzdHJ1Y3QgbmZz NF9jYWNoZWRfYWNsICphY2w7DQogCXNpemVfdCBidWZsZW4gPSBzaXplb2YoKmFjbCkgKyBhY2xf bGVuOw0KIA0KLQlpZiAocGFnZXMgJiYgYnVmbGVuIDw9IFBBR0VfU0laRSkgew0KKwlpZiAoYnVm bGVuIDw9IFBBR0VfU0laRSkgew0KIAkJYWNsID0ga21hbGxvYyhidWZsZW4sIEdGUF9LRVJORUwp Ow0KIAkJaWYgKGFjbCA9PSBOVUxMKQ0KIAkJCWdvdG8gb3V0Ow0KQEAgLTM3ODQsNyArMzc4NCw2 IEBAIHN0YXRpYyBzc2l6ZV90IF9fbmZzNF9nZXRfYWNsX3VuY2FjaGVkKHN0cnVjdCBpbm9kZSAq aW5vZGUsIHZvaWQgKmJ1Ziwgc2l6ZV90IGJ1DQogCX07DQogCXVuc2lnbmVkIGludCBucGFnZXMg PSBESVZfUk9VTkRfVVAoYnVmbGVuLCBQQUdFX1NJWkUpOw0KIAlpbnQgcmV0ID0gLUVOT01FTSwg aTsNCi0Jc2l6ZV90IGFjbF9sZW4gPSAwOw0KIA0KIAkvKiBBcyBsb25nIGFzIHdlJ3JlIGRvaW5n IGEgcm91bmQgdHJpcCB0byB0aGUgc2VydmVyIGFueXdheSwNCiAJICogbGV0J3MgYmUgcHJlcGFy ZWQgZm9yIGEgcGFnZSBvZiBhY2wgZGF0YS4gKi8NCkBAIC0zODA3LDExICszODA2LDYgQEAgc3Rh dGljIHNzaXplX3QgX19uZnM0X2dldF9hY2xfdW5jYWNoZWQoc3RydWN0IGlub2RlICppbm9kZSwg dm9pZCAqYnVmLCBzaXplX3QgYnUNCiAJYXJncy5hY2xfbGVuID0gbnBhZ2VzICogUEFHRV9TSVpF Ow0KIAlhcmdzLmFjbF9wZ2Jhc2UgPSAwOw0KIA0KLQkvKiBMZXQgZGVjb2RlX2dldGZhY2wga25v dyBub3QgdG8gZmFpbCBpZiB0aGUgQUNMIGRhdGEgaXMgbGFyZ2VyIHRoYW4NCi0JICogdGhlIHBh Z2Ugd2Ugc2VuZCBhcyBhIGd1ZXNzICovDQotCWlmIChidWYgPT0gTlVMTCkNCi0JCXJlcy5hY2xf ZmxhZ3MgfD0gTkZTNF9BQ0xfTEVOX1JFUVVFU1Q7DQotDQogCWRwcmludGsoIiVzICBidWYgJXAg YnVmbGVuICV6dSBucGFnZXMgJWQgYXJncy5hY2xfbGVuICV6dVxuIiwNCiAJCV9fZnVuY19fLCBi dWYsIGJ1ZmxlbiwgbnBhZ2VzLCBhcmdzLmFjbF9sZW4pOw0KIAlyZXQgPSBuZnM0X2NhbGxfc3lu YyhORlNfU0VSVkVSKGlub2RlKS0+Y2xpZW50LCBORlNfU0VSVkVSKGlub2RlKSwNCkBAIC0zODE5 LDIwICszODEzLDE5IEBAIHN0YXRpYyBzc2l6ZV90IF9fbmZzNF9nZXRfYWNsX3VuY2FjaGVkKHN0 cnVjdCBpbm9kZSAqaW5vZGUsIHZvaWQgKmJ1Ziwgc2l6ZV90IGJ1DQogCWlmIChyZXQpDQogCQln b3RvIG91dF9mcmVlOw0KIA0KLQlhY2xfbGVuID0gcmVzLmFjbF9sZW47DQotCWlmIChhY2xfbGVu ID4gYXJncy5hY2xfbGVuKQ0KLQkJbmZzNF93cml0ZV9jYWNoZWRfYWNsKGlub2RlLCBOVUxMLCAw LCBhY2xfbGVuKTsNCi0JZWxzZQ0KLQkJbmZzNF93cml0ZV9jYWNoZWRfYWNsKGlub2RlLCBwYWdl cywgcmVzLmFjbF9kYXRhX29mZnNldCwNCi0JCQkJICAgICAgYWNsX2xlbik7DQotCWlmIChidWYp IHsNCisJLyogSGFuZGxlIHRoZSBjYXNlIHdoZXJlIHRoZSBwYXNzZWQtaW4gYnVmZmVyIGlzIHRv byBzaG9ydCAqLw0KKwlpZiAocmVzLmFjbF9mbGFncyAmIE5GUzRfQUNMX1RSVU5DKSB7DQorCQkv KiBEaWQgdGhlIHVzZXIgb25seSBpc3N1ZSBhIHJlcXVlc3QgZm9yIHRoZSBhY2wgbGVuZ3RoPyAq Lw0KKwkJaWYgKGJ1ZiA9PSBOVUxMKQ0KKwkJCWdvdG8gb3V0X29rOw0KIAkJcmV0ID0gLUVSQU5H RTsNCi0JCWlmIChhY2xfbGVuID4gYnVmbGVuKQ0KLQkJCWdvdG8gb3V0X2ZyZWU7DQotCQlfY29w eV9mcm9tX3BhZ2VzKGJ1ZiwgcGFnZXMsIHJlcy5hY2xfZGF0YV9vZmZzZXQsDQotCQkJCWFjbF9s ZW4pOw0KKwkJZ290byBvdXRfZnJlZTsNCiAJfQ0KLQlyZXQgPSBhY2xfbGVuOw0KKwluZnM0X3dy aXRlX2NhY2hlZF9hY2woaW5vZGUsIHBhZ2VzLCByZXMuYWNsX2RhdGFfb2Zmc2V0LCByZXMuYWNs X2xlbik7DQorCWlmIChidWYpDQorCQlfY29weV9mcm9tX3BhZ2VzKGJ1ZiwgcGFnZXMsIHJlcy5h Y2xfZGF0YV9vZmZzZXQsIHJlcy5hY2xfbGVuKTsNCitvdXRfb2s6DQorCXJldCA9IHJlcy5hY2xf bGVuOw0KIG91dF9mcmVlOg0KIAlmb3IgKGkgPSAwOyBpIDwgbnBhZ2VzOyBpKyspDQogCQlpZiAo cGFnZXNbaV0pDQpkaWZmIC0tZ2l0IGEvZnMvbmZzL25mczR4ZHIuYyBiL2ZzL25mcy9uZnM0eGRy LmMNCmluZGV4IDFiZmJkNjcuLjNlYmUwMjUgMTAwNjQ0DQotLS0gYS9mcy9uZnMvbmZzNHhkci5j DQorKysgYi9mcy9uZnMvbmZzNHhkci5jDQpAQCAtNTA3MywxNyArNTA3MywxMyBAQCBzdGF0aWMg aW50IGRlY29kZV9nZXRhY2woc3RydWN0IHhkcl9zdHJlYW0gKnhkciwgc3RydWN0IHJwY19ycXN0 ICpyZXEsDQogCQkgKiB2YXJpYWJsZSBsZW5ndGggYml0bWFwcy4qLw0KIAkJcmVzLT5hY2xfZGF0 YV9vZmZzZXQgPSB4ZHJfc3RyZWFtX3Bvcyh4ZHIpIC0gcGdfb2Zmc2V0Ow0KIA0KLQkJLyogV2Ug aWdub3JlICZzYXZlcCBhbmQgZG9uJ3QgZG8gY29uc2lzdGVuY3kgY2hlY2tzIG9uDQotCQkgKiB0 aGUgYXR0ciBsZW5ndGguICBMZXQgdXNlcnNwYWNlIGZpZ3VyZSBpdCBvdXQuLi4uICovDQogCQly ZXMtPmFjbF9sZW4gPSBhdHRybGVuOw0KLQkJaWYgKGF0dHJsZW4gPiAoeGRyLT5ud29yZHMgPDwg MikpIHsNCi0JCQlpZiAocmVzLT5hY2xfZmxhZ3MgJiBORlM0X0FDTF9MRU5fUkVRVUVTVCkgew0K LQkJCQkvKiBnZXR4YXR0ciBpbnRlcmZhY2UgY2FsbGVkIHdpdGggYSBOVUxMIGJ1ZiAqLw0KLQkJ CQlnb3RvIG91dDsNCi0JCQl9DQorCQkvKiBDaGVjayBmb3IgcmVjZWl2ZSBidWZmZXIgb3ZlcmZs b3cgKi8NCisJCWlmIChhdHRybGVuID4gKHhkci0+bndvcmRzIDw8IDIpIHx8DQorCQkgICAgYXR0 cmxlbiArIHBnX29mZnNldCA+IHhkci0+YnVmLT5wYWdlX2xlbikgew0KKwkJCXJlcy0+YWNsX2Zs YWdzIHw9IE5GUzRfQUNMX1RSVU5DOw0KIAkJCWRwcmludGsoIk5GUzogYWNsIHJlcGx5OiBhdHRy bGVuICV1ID4gcGFnZV9sZW4gJXVcbiIsDQogCQkJCQlhdHRybGVuLCB4ZHItPm53b3JkcyA8PCAy KTsNCi0JCQlyZXR1cm4gLUVJTlZBTDsNCiAJCX0NCiAJfSBlbHNlDQogCQlzdGF0dXMgPSAtRU9Q Tk9UU1VQUDsNCmRpZmYgLS1naXQgYS9pbmNsdWRlL2xpbnV4L25mc194ZHIuaCBiL2luY2x1ZGUv bGludXgvbmZzX3hkci5oDQppbmRleCBhYzdjOGFlLi5iZTljZjNjIDEwMDY0NA0KLS0tIGEvaW5j bHVkZS9saW51eC9uZnNfeGRyLmgNCisrKyBiL2luY2x1ZGUvbGludXgvbmZzX3hkci5oDQpAQCAt NjUyLDcgKzY1Miw3IEBAIHN0cnVjdCBuZnNfZ2V0YWNsYXJncyB7DQogfTsNCiANCiAvKiBnZXR4 YXR0ciBBQ0wgaW50ZXJmYWNlIGZsYWdzICovDQotI2RlZmluZSBORlM0X0FDTF9MRU5fUkVRVUVT VAkweDAwMDEJLyogemVybyBsZW5ndGggZ2V0eGF0dHIgYnVmZmVyICovDQorI2RlZmluZSBORlM0 X0FDTF9UUlVOQwkJMHgwMDAxCS8qIEFDTCB3YXMgdHJ1bmNhdGVkICovDQogc3RydWN0IG5mc19n ZXRhY2xyZXMgew0KIAlzaXplX3QJCQkJYWNsX2xlbjsNCiAJc2l6ZV90CQkJCWFjbF9kYXRhX29m ZnNldDsNCi0tIA0KMS43LjExLjQNCg0KDQotLSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZT IGNsaWVudCBtYWludGFpbmVyDQoNCk5ldEFwcA0KVHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20N Cnd3dy5uZXRhcHAuY29tDQoNCg== ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Avoid array overflow in __nfs4_get_acl_uncached 2012-09-03 19:11 ` Myklebust, Trond @ 2012-09-06 14:46 ` Sachin Prabhu 2012-09-06 14:53 ` Myklebust, Trond 0 siblings, 1 reply; 13+ messages in thread From: Sachin Prabhu @ 2012-09-06 14:46 UTC (permalink / raw) To: Myklebust, Trond; +Cc: Linux NFS mailing list On Mon, 2012-09-03 at 19:11 +0000, Myklebust, Trond wrote: > > I encountered 2 problems. > > 1) The if condition should be srclen >= pgbase + acl_len > > 2) There is a second _copy_from_pages which copies to the the acl to the > > passed buffer in __nfs4_get_acl_uncached(). > > The second copy from pages should already be covered by the checks in > decode_getacl. Alright, since this is not obvious, then clearly we need > to make it so. How about the following? I could reproduce the crash with the second _copy_from_pages using the patch to PyNFS from http://www.spinics.net/lists/linux-nfs/msg32359.html The patch below works fine for both cases apart from a small bug which I've pointed to below. > 8<------------------------------------------------------------------ > From 5040240245a046bd58c383806b3f161ee8b5823b Mon Sep 17 00:00:00 2001 > From: Trond Myklebust <Trond.Myklebust@netapp.com> > Date: Sun, 26 Aug 2012 11:44:43 -0700 > Subject: [PATCH] NFSv4: Fix buffer overflow checking in > __nfs4_get_acl_uncached > > Pass the checks made by decode_getacl back to __nfs4_get_acl_uncached > so that it knows if the acl has been truncated. > > Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com> > --- > fs/nfs/nfs4proc.c | 31 ++++++++++++------------------- > fs/nfs/nfs4xdr.c | 12 ++++-------- > include/linux/nfs_xdr.h | 2 +- > 3 files changed, 17 insertions(+), 28 deletions(-) > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index 654dc38..74f5c26 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -3739,7 +3739,7 @@ static void nfs4_write_cached_acl(struct inode *inode, struct page **pages, size > struct nfs4_cached_acl *acl; > size_t buflen = sizeof(*acl) + acl_len; > > - if (pages && buflen <= PAGE_SIZE) { > + if (buflen <= PAGE_SIZE) { > acl = kmalloc(buflen, GFP_KERNEL); > if (acl == NULL) > goto out; > @@ -3784,7 +3784,6 @@ static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu > }; > unsigned int npages = DIV_ROUND_UP(buflen, PAGE_SIZE); > int ret = -ENOMEM, i; > - size_t acl_len = 0; > > /* As long as we're doing a round trip to the server anyway, > * let's be prepared for a page of acl data. */ > @@ -3807,11 +3806,6 @@ static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu > args.acl_len = npages * PAGE_SIZE; > args.acl_pgbase = 0; > > - /* Let decode_getfacl know not to fail if the ACL data is larger than > - * the page we send as a guess */ > - if (buf == NULL) > - res.acl_flags |= NFS4_ACL_LEN_REQUEST; > - > dprintk("%s buf %p buflen %zu npages %d args.acl_len %zu\n", > __func__, buf, buflen, npages, args.acl_len); > ret = nfs4_call_sync(NFS_SERVER(inode)->client, NFS_SERVER(inode), > @@ -3819,20 +3813,19 @@ 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; > - if (acl_len > args.acl_len) > - nfs4_write_cached_acl(inode, NULL, 0, acl_len); > - else > - nfs4_write_cached_acl(inode, pages, res.acl_data_offset, > - acl_len); > - if (buf) { > + /* Handle the case where the passed-in buffer is too short */ > + if (res.acl_flags & NFS4_ACL_TRUNC) { > + /* Did the user only issue a request for the acl length? */ > + if (buf == NULL) > + goto out_ok; > ret = -ERANGE; > - if (acl_len > buflen) > - goto out_free; > - _copy_from_pages(buf, pages, res.acl_data_offset, > - acl_len); > + goto out_free; > } > - ret = acl_len; > + nfs4_write_cached_acl(inode, pages, res.acl_data_offset, res.acl_len); > + if (buf) > + _copy_from_pages(buf, pages, res.acl_data_offset, res.acl_len); > +out_ok: > + ret = res.acl_len; > out_free: > for (i = 0; i < npages; i++) > if (pages[i]) > diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c > index 1bfbd67..3ebe025 100644 > --- a/fs/nfs/nfs4xdr.c > +++ b/fs/nfs/nfs4xdr.c > @@ -5073,17 +5073,13 @@ static int decode_getacl(struct xdr_stream *xdr, struct rpc_rqst *req, > * variable length bitmaps.*/ > 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 > (xdr->nwords << 2)) { > - if (res->acl_flags & NFS4_ACL_LEN_REQUEST) { > - /* getxattr interface called with a NULL buf */ > - goto out; > - } > + /* Check for receive buffer overflow */ > + if (attrlen > (xdr->nwords << 2) || > + attrlen + pg_offset > xdr->buf->page_len) { Here we need to use res->acl_data_offset which points to the start of the ACL data instead of pg_offset which points to the start of the pages section of the xdr_buf. Once I changed this if condition, I was able to successfully test with my reproducer. Sachin Prabhu > + res->acl_flags |= NFS4_ACL_TRUNC; > dprintk("NFS: acl reply: attrlen %u > page_len %u\n", > attrlen, xdr->nwords << 2); > - return -EINVAL; > } > } else > status = -EOPNOTSUPP; > diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h > index ac7c8ae..be9cf3c 100644 > --- a/include/linux/nfs_xdr.h > +++ b/include/linux/nfs_xdr.h > @@ -652,7 +652,7 @@ struct nfs_getaclargs { > }; > > /* getxattr ACL interface flags */ > -#define NFS4_ACL_LEN_REQUEST 0x0001 /* zero length getxattr buffer */ > +#define NFS4_ACL_TRUNC 0x0001 /* ACL was truncated */ > struct nfs_getaclres { > size_t acl_len; > size_t acl_data_offset; > -- > 1.7.11.4 > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Avoid array overflow in __nfs4_get_acl_uncached 2012-09-06 14:46 ` Sachin Prabhu @ 2012-09-06 14:53 ` Myklebust, Trond 2012-09-06 15:05 ` Sachin Prabhu 0 siblings, 1 reply; 13+ messages in thread From: Myklebust, Trond @ 2012-09-06 14:53 UTC (permalink / raw) To: Sachin Prabhu; +Cc: Linux NFS mailing list T24gVGh1LCAyMDEyLTA5LTA2IGF0IDE1OjQ2ICswMTAwLCBTYWNoaW4gUHJhYmh1IHdyb3RlOg0K PiBPbiBNb24sIDIwMTItMDktMDMgYXQgMTk6MTEgKzAwMDAsIE15a2xlYnVzdCwgVHJvbmQgd3Jv dGU6DQo+ID4gPiBJIGVuY291bnRlcmVkIDIgcHJvYmxlbXMuIA0KPiA+ID4gMSkgVGhlIGlmIGNv bmRpdGlvbiBzaG91bGQgYmUgc3JjbGVuID49IHBnYmFzZSArIGFjbF9sZW4NCj4gPiA+IDIpIFRo ZXJlIGlzIGEgc2Vjb25kIF9jb3B5X2Zyb21fcGFnZXMgd2hpY2ggY29waWVzIHRvIHRoZSB0aGUg YWNsIHRvIHRoZQ0KPiA+ID4gcGFzc2VkIGJ1ZmZlciBpbiBfX25mczRfZ2V0X2FjbF91bmNhY2hl ZCgpLg0KPiA+IA0KPiA+IFRoZSBzZWNvbmQgY29weSBmcm9tIHBhZ2VzIHNob3VsZCBhbHJlYWR5 IGJlIGNvdmVyZWQgYnkgdGhlIGNoZWNrcyBpbg0KPiA+IGRlY29kZV9nZXRhY2wuIEFscmlnaHQs IHNpbmNlIHRoaXMgaXMgbm90IG9idmlvdXMsIHRoZW4gY2xlYXJseSB3ZSBuZWVkDQo+ID4gdG8g bWFrZSBpdCBzby4gSG93IGFib3V0IHRoZSBmb2xsb3dpbmc/DQo+IA0KPiBJIGNvdWxkIHJlcHJv ZHVjZSB0aGUgY3Jhc2ggd2l0aCB0aGUgc2Vjb25kIF9jb3B5X2Zyb21fcGFnZXMgdXNpbmcgdGhl DQo+IHBhdGNoIHRvIFB5TkZTIGZyb20NCj4gaHR0cDovL3d3dy5zcGluaWNzLm5ldC9saXN0cy9s aW51eC1uZnMvbXNnMzIzNTkuaHRtbA0KPiANCj4gVGhlIHBhdGNoIGJlbG93IHdvcmtzIGZpbmUg Zm9yIGJvdGggY2FzZXMgYXBhcnQgZnJvbSBhIHNtYWxsIGJ1ZyB3aGljaA0KPiBJJ3ZlIHBvaW50 ZWQgdG8gYmVsb3cuDQo+IA0KPiA+IDg8LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tDQo+ID4gRnJvbSA1MDQwMjQwMjQ1YTA0 NmJkNThjMzgzODA2YjNmMTYxZWU4YjU4MjNiIE1vbiBTZXAgMTcgMDA6MDA6MDAgMjAwMQ0KPiA+ IEZyb206IFRyb25kIE15a2xlYnVzdCA8VHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20+DQo+ID4g RGF0ZTogU3VuLCAyNiBBdWcgMjAxMiAxMTo0NDo0MyAtMDcwMA0KPiA+IFN1YmplY3Q6IFtQQVRD SF0gTkZTdjQ6IEZpeCBidWZmZXIgb3ZlcmZsb3cgY2hlY2tpbmcgaW4NCj4gPiAgX19uZnM0X2dl dF9hY2xfdW5jYWNoZWQNCg0KPiA+ICsJCS8qIENoZWNrIGZvciByZWNlaXZlIGJ1ZmZlciBvdmVy ZmxvdyAqLw0KPiA+ICsJCWlmIChhdHRybGVuID4gKHhkci0+bndvcmRzIDw8IDIpIHx8DQo+ID4g KwkJICAgIGF0dHJsZW4gKyBwZ19vZmZzZXQgPiB4ZHItPmJ1Zi0+cGFnZV9sZW4pIHsNCj4gDQo+ IEhlcmUgd2UgbmVlZCB0byB1c2UgcmVzLT5hY2xfZGF0YV9vZmZzZXQgd2hpY2ggcG9pbnRzIHRv IHRoZSBzdGFydCBvZg0KPiB0aGUgQUNMIGRhdGEgaW5zdGVhZCBvZiBwZ19vZmZzZXQgd2hpY2gg cG9pbnRzIHRvIHRoZSBzdGFydCBvZiB0aGUgcGFnZXMNCj4gc2VjdGlvbiBvZiB0aGUgeGRyX2J1 Zi4NCj4gDQo+IE9uY2UgSSBjaGFuZ2VkIHRoaXMgaWYgY29uZGl0aW9uLCBJIHdhcyBhYmxlIHRv IHN1Y2Nlc3NmdWxseSB0ZXN0IHdpdGgNCj4gbXkgcmVwcm9kdWNlci4NCg0KT29wcy4uLiBZZXMs IG9mIGNvdXJzZSB5b3UgYXJlIHJpZ2h0LiBJJ2xsIG1ha2UgdGhlIG1vZGlmaWNhdGlvbiBhbmQN CnB1c2ggb3V0IHRoZSBjaGFuZ2VkIHBhdGNoLg0KDQpUaGFua3MgdmVyeSBtdWNoIGZvciB0ZXN0 aW5nISBEbyB5b3Ugd2FudCBtZSB0byBhZGQgYSAiVGVzdGVkLWJ5OiIgbGluZT8NCg0KLS0gDQpU cm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFpbnRhaW5lcg0KDQpOZXRBcHANClRy b25kLk15a2xlYnVzdEBuZXRhcHAuY29tDQp3d3cubmV0YXBwLmNvbQ0KDQo= ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Avoid array overflow in __nfs4_get_acl_uncached 2012-09-06 14:53 ` Myklebust, Trond @ 2012-09-06 15:05 ` Sachin Prabhu 0 siblings, 0 replies; 13+ messages in thread From: Sachin Prabhu @ 2012-09-06 15:05 UTC (permalink / raw) To: Myklebust, Trond; +Cc: Linux NFS mailing list On Thu, 2012-09-06 at 14:53 +0000, Myklebust, Trond wrote: > On Thu, 2012-09-06 at 15:46 +0100, Sachin Prabhu wrote: > > On Mon, 2012-09-03 at 19:11 +0000, Myklebust, Trond wrote: > > > > I encountered 2 problems. > > > > 1) The if condition should be srclen >= pgbase + acl_len > > > > 2) There is a second _copy_from_pages which copies to the the acl to the > > > > passed buffer in __nfs4_get_acl_uncached(). > > > > > > The second copy from pages should already be covered by the checks in > > > decode_getacl. Alright, since this is not obvious, then clearly we need > > > to make it so. How about the following? > > > > I could reproduce the crash with the second _copy_from_pages using the > > patch to PyNFS from > > http://www.spinics.net/lists/linux-nfs/msg32359.html > > > > The patch below works fine for both cases apart from a small bug which > > I've pointed to below. > > > > > 8<------------------------------------------------------------------ > > > From 5040240245a046bd58c383806b3f161ee8b5823b Mon Sep 17 00:00:00 2001 > > > From: Trond Myklebust <Trond.Myklebust@netapp.com> > > > Date: Sun, 26 Aug 2012 11:44:43 -0700 > > > Subject: [PATCH] NFSv4: Fix buffer overflow checking in > > > __nfs4_get_acl_uncached > > > > + /* Check for receive buffer overflow */ > > > + if (attrlen > (xdr->nwords << 2) || > > > + attrlen + pg_offset > xdr->buf->page_len) { > > > > Here we need to use res->acl_data_offset which points to the start of > > the ACL data instead of pg_offset which points to the start of the pages > > section of the xdr_buf. > > > > Once I changed this if condition, I was able to successfully test with > > my reproducer. > > Oops... Yes, of course you are right. I'll make the modification and > push out the changed patch. > > Thanks very much for testing! Do you want me to add a "Tested-by:" line? > Fine by me. Sachin Prabhu ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2012-09-06 15:05 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-08-24 14:16 [PATCH] Avoid array overflow in __nfs4_get_acl_uncached Sachin Prabhu 2012-08-24 15:07 ` Myklebust, Trond 2012-08-24 21:31 ` Sachin Prabhu 2012-08-24 21:38 ` Myklebust, Trond 2012-08-24 21:51 ` Sachin Prabhu 2012-08-24 22:02 ` Myklebust, Trond 2012-08-25 23:31 ` Sachin Prabhu 2012-08-26 18:57 ` Myklebust, Trond 2012-08-28 14:09 ` Sachin Prabhu 2012-09-03 19:11 ` Myklebust, Trond 2012-09-06 14:46 ` Sachin Prabhu 2012-09-06 14:53 ` Myklebust, Trond 2012-09-06 15:05 ` Sachin Prabhu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).