Skip to contentSkip to conversation

NFS: Avoid cross-structure casting

PATCH In review Kees Cook 2 participants 3 messages Apr 5, 2017 – Apr 20, 2017

Subsystems (2)
NFS client NFS server
lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH v2 1/1 NFS: Avoid cross-structure casting

When the call to nfs_devname() fails, the error path attempts to retain the error via the mnt variable, but this requires a cast across very different types (char * to struct vfsmount *), which the upcoming structure layout randomization plugin flags as being potentially dangerous in the face of randomization. This is a false positive, but what this code actually wants to do is retain the error value, so this patch explicitly sets it, instead of using what seems to be an unexpected cast.

Signed-off-by Kees Cook <[email protected]>

---
v2: duh, use ERR_CAST. thanks neilb!

---
 fs/nfs/namespace.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
fs/nfs/namespace.c +3 -2
index 786f17580582..8ca5d147124d 100644
@@ -259,9 +259,10 @@ struct vfsmount *nfs_do_submount(struct dentry *dentry, struct nfs_fh *fh,
 	if (page == NULL)
 		goto out;
 	devname = nfs_devname(dentry, page, PAGE_SIZE);
-	mnt = (struct vfsmount *)devname;
-	if (IS_ERR(devname))
+	if (IS_ERR(devname)) {
+		mnt = ERR_CAST(devname);
 		goto free_page;
+	}
 	mnt = nfs_do_clone_mount(NFS_SB(dentry->d_sb), devname, &mountdata);
 free_page:
 	free_page((unsigned long)page);
Signature · 6 lines
2.7.4


-- 
Kees Cook
Pixel Security
author
#1 lore raw
Quoted 10 lines · Kees Cook

When the call to nfs_devname() fails, the error path attempts to retain the error via the mnt variable, but this requires a cast across very different types (char * to struct vfsmount *), which the upcoming structure layout randomization plugin flags as being potentially dangerous in the face of randomization. This is a false positive, but what this code actually wants to do is retain the error value, so this patch explicitly sets it, instead of using what seems to be an unexpected cast.

Signed-off-by Kees Cook <[email protected]>

If I can get an Acked-by on this, I could push it via the gcc-plugin tree.

Thanks!

-Kees

Quoted 30 lines

---
v2: duh, use ERR_CAST. thanks neilb!

---
 fs/nfs/namespace.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
fs/nfs/namespace.c +3 -3
index 786f17580582..8ca5d147124d 100644
@@ -259,9 +259,10 @@ struct vfsmount *nfs_do_submount(struct dentry *dentry, struct nfs_fh *fh,
        if (page == NULL)
                goto out;
        devname = nfs_devname(dentry, page, PAGE_SIZE);
-       mnt = (struct vfsmount *)devname;
-       if (IS_ERR(devname))
+       if (IS_ERR(devname)) {
+               mnt = ERR_CAST(devname);
                goto free_page;
+       }
        mnt = nfs_do_clone_mount(NFS_SB(dentry->d_sb), devname, &mountdata);
 free_page:
        free_page((unsigned long)page);
--

2.7.4

--
Kees Cook
Pixel Security

Signature · 2 lines
Kees Cook
Pixel Security
Acked-by
#2 lore raw

T24gV2VkLCAyMDE3LTA0LTEyIGF0IDExOjU5IC0wNzAwLCBLZWVzIENvb2sgd3JvdGU6DQo+IE9u IFdlZCwgQXByIDUsIDIwMTcgYXQgODoyOSBBTSwgS2VlcyBDb29rIDxrZWVzY29va0BjaHJvbWl1 bS5vcmc+DQo+IHdyb3RlOg0KPiA+IFdoZW4gdGhlIGNhbGwgdG8gbmZzX2Rldm5hbWUoKSBmYWls cywgdGhlIGVycm9yIHBhdGggYXR0ZW1wdHMgdG8NCj4gPiByZXRhaW4NCj4gPiB0aGUgZXJyb3Ig dmlhIHRoZSBtbnQgdmFyaWFibGUsIGJ1dCB0aGlzIHJlcXVpcmVzIGEgY2FzdCBhY3Jvc3MNCj4g PiB2ZXJ5DQo+ID4gZGlmZmVyZW50IHR5cGVzIChjaGFyICogdG8gc3RydWN0IHZmc21vdW50ICop LCB3aGljaCB0aGUgdXBjb21pbmcNCj4gPiBzdHJ1Y3R1cmUgbGF5b3V0IHJhbmRvbWl6YXRpb24g cGx1Z2luIGZsYWdzIGFzIGJlaW5nIHBvdGVudGlhbGx5DQo+ID4gZGFuZ2Vyb3VzIGluIHRoZSBm YWNlIG9mIHJhbmRvbWl6YXRpb24uIFRoaXMgaXMgYSBmYWxzZSBwb3NpdGl2ZSwNCj4gPiBidXQN Cj4gPiB3aGF0IHRoaXMgY29kZSBhY3R1YWxseSB3YW50cyB0byBkbyBpcyByZXRhaW4gdGhlIGVy cm9yIHZhbHVlLCBzbw0KPiA+IHRoaXMNCj4gPiBwYXRjaCBleHBsaWNpdGx5IHNldHMgaXQsIGlu c3RlYWQgb2YgdXNpbmcgd2hhdCBzZWVtcyB0byBiZSBhbg0KPiA+IHVuZXhwZWN0ZWQgY2FzdC4N Cj4gPiANCj4gPiBTaWduZWQtb2ZmLWJ5OiBLZWVzIENvb2sgPGtlZXNjb29rQGNocm9taXVtLm9y Zz4NCj4gDQo+IElmIEkgY2FuIGdldCBhbiBBY2tlZC1ieSBvbiB0aGlzLCBJIGNvdWxkIHB1c2gg aXQgdmlhIHRoZSBnY2MtcGx1Z2luDQo+IHRyZWUuDQo+IA0KPiBUaGFua3MhDQo+IA0KPiAtS2Vl cw0KPiANCj4gPiAtLS0NCj4gPiB2MjogZHVoLCB1c2UgRVJSX0NBU1QuIHRoYW5rcyBuZWlsYiEN Cj4gPiAtLS0NCj4gPiDCoGZzL25mcy9uYW1lc3BhY2UuYyB8IDUgKysrLS0NCj4gPiDCoDEgZmls ZSBjaGFuZ2VkLCAzIGluc2VydGlvbnMoKyksIDIgZGVsZXRpb25zKC0pDQo+ID4gDQo+ID4gZGlm ZiAtLWdpdCBhL2ZzL25mcy9uYW1lc3BhY2UuYyBiL2ZzL25mcy9uYW1lc3BhY2UuYw0KPiA+IGlu ZGV4IDc4NmYxNzU4MDU4Mi4uOGNhNWQxNDcxMjRkIDEwMDY0NA0KPiA+IC0tLSBhL2ZzL25mcy9u YW1lc3BhY2UuYw0KPiA+ICsrKyBiL2ZzL25mcy9uYW1lc3BhY2UuYw0KPiA+IEBAIC0yNTksOSAr MjU5LDEwIEBAIHN0cnVjdCB2ZnNtb3VudCAqbmZzX2RvX3N1Ym1vdW50KHN0cnVjdCBkZW50cnkN Cj4gPiAqZGVudHJ5LCBzdHJ1Y3QgbmZzX2ZoICpmaCwNCj4gPiDCoMKgwqDCoMKgwqDCoMKgaWYg KHBhZ2UgPT0gTlVMTCkNCj4gPiDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoGdvdG8g b3V0Ow0KPiA+IMKgwqDCoMKgwqDCoMKgwqBkZXZuYW1lID0gbmZzX2Rldm5hbWUoZGVudHJ5LCBw YWdlLCBQQUdFX1NJWkUpOw0KPiA+IC3CoMKgwqDCoMKgwqDCoG1udCA9IChzdHJ1Y3QgdmZzbW91 bnQgKilkZXZuYW1lOw0KPiA+IC3CoMKgwqDCoMKgwqDCoGlmIChJU19FUlIoZGV2bmFtZSkpDQo+ ID4gK8KgwqDCoMKgwqDCoMKgaWYgKElTX0VSUihkZXZuYW1lKSkgew0KPiA+ICvCoMKgwqDCoMKg wqDCoMKgwqDCoMKgwqDCoMKgwqBtbnQgPSBFUlJfQ0FTVChkZXZuYW1lKTsNCj4gPiDCoMKgwqDC oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoGdvdG8gZnJlZV9wYWdlOw0KPiA+ICvCoMKgwqDCoMKg wqDCoH0NCj4gPiDCoMKgwqDCoMKgwqDCoMKgbW50ID0gbmZzX2RvX2Nsb25lX21vdW50KE5GU19T QihkZW50cnktPmRfc2IpLCBkZXZuYW1lLA0KPiA+ICZtb3VudGRhdGEpOw0KPiA+IMKgZnJlZV9w YWdlOg0KPiA+IMKgwqDCoMKgwqDCoMKgwqBmcmVlX3BhZ2UoKHVuc2lnbmVkIGxvbmcpcGFnZSk7 DQo+ID4gLS0NCj4gPiAyLjcuNA0KPiA+IA0KPiA+IA0KPiA+IC0tDQo+ID4gS2VlcyBDb29rDQo+ ID4gUGl4ZWwgU2VjdXJpdHkNCj4gDQoNCkFja2VkLWJ5OiBUcm9uZCBNeWtsZWJ1c3QgPHRyb25k Lm15a2xlYnVzdEBwcmltYXJ5ZGF0YS5jb20+DQoNCi0tIA0KVHJvbmQgTXlrbGVidXN0DQpMaW51 eCBORlMgY2xpZW50IG1haW50YWluZXIsIFByaW1hcnlEYXRhDQp0cm9uZC5teWtsZWJ1c3RAcHJp bWFyeWRhdGEuY29tDQo=