NFSv41: fix NULL dereference in nfs40_setup_sequence
diff mbox

Message ID 0A7143CE-0592-444D-BA53-9CBB33E4373F@gmail.com
State New
Headers show

Commit Message

Vitaliy Gusev Oct. 31, 2016, 1:18 a.m. UTC
Hi.

During some tests I’ve seen nfs-client crashes. It was just reading file via NFSv4.1 protocol. 

The panic message is below,  fixing patch is attached.

———
"BUG: unable to handle kernel NULL pointer dereference at 0000000000000090
"IP: [<ffffffffb049eefc>] _raw_spin_lock+0xc/0x30
PGD 2a1c067 PUD 29e5067 PMD 0
Oops: 0002 [#1] SMP
CPU: 1 PID: 3573 Comm: kworker/1:0 Not tainted 4.8.0-26-generic #28-Ubuntu
Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 07/02/2015
Workqueue: rpciod rpc_async_schedule [sunrpc]
task: ffffa017cb534740 task.stack: ffffa01782b44000
RIP: 0010:[<ffffffffb049eefc>]  [<ffffffffb049eefc>] _raw_spin_lock+0xc/0x30N^
RSP: 0018:ffffa01782b47d48  EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffffa017c735d208 RCX: ffffa01783816c00
RDX: 0000000000000001 RSI: ffffa017c735d1e0 RDI: 0000000000000090
RBP: ffffa01782b47d78 R08: ffffa017cec58a00 R09: 0000000000000000
R10: 0000000000000000 R11: 000000b4098ed036 R12: ffffa01783816c00
R13: 0000000000000000 R14: 0000000000000090 R15: ffffa017c735d1e0
FS:  0000000000000000(0000) GS:ffffa017cec40000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000090 CR3: 0000000002a0c000 CR4: 00000000001406e0
Stack:
ffffffffc0836208 ffffa01783816c00 ffffffffc04c9070 ffffffffc04c9070
0000000000000000 ffffa01783816c40 ffffa01782b47d88 ffffffffc08362d0
ffffa01782b47d98 ffffffffc04c9089 ffffa01782b47e00 ffffffffc04cb6fd

Call Trace:
[<ffffffffc0836208>] ? nfs40_setup_sequence+0x48/0xe0 [nfsv4]
[<ffffffffc08362d0>] nfs4_open_confirm_prepare+0x30/0x40 [nfsv4]
[<ffffffffc04c9089>] rpc_prepare_task+0x19/0x20 [sunrpc]
[<ffffffffc04cb6fd>] __rpc_execute+0x8d/0x420 [sunrpc]
[<ffffffffc04cbaa2>] rpc_async_schedule+0x12/0x20 [sunrpc]
[<ffffffffafc9d61c>] process_one_work+0x1fc/0x4b0
[<ffffffffafc9d91b>] worker_thread+0x4b/0x500
[<ffffffffafca3c18>] kthread+0xd8/0xf0
[<ffffffffb049f29f>] ret_from_fork+0x1f/0x40
[<ffffffffafca3b40>] ? kthread_create_on_node+0x1e0/0x1e0
Code: 00 01 00 00 f0 0f c1 37 81 c6 00 01 00 00 40 84 f6 75 01 c3 55 48 89 e5 e8 e2 19 83 ff 5d c3 0f 1f 44 00 00 31 c0 ba 01 00 00 00 <f0> 0f b1 17 85 c0 75 01 c3 55 89 c6 48 89 e5 e8 c0 01 83 ff 66 
"RIP  [<ffffffffb049eefc>] _raw_spin_lock+0xc/0x30

———

From 1b5e4636c696a169f46b474de519f1d4d4f06277 Mon Sep 17 00:00:00 2001
From: Vitaliy Gusev <gusev.vitaliy@gmail.com>
Date: Mon, 31 Oct 2016 03:11:50 +0300
Subject: [PATCH] NFSv4.1: fix NULL dereference in nfs40_setup_sequence

If OPEN4 reply has set OPEN4_RESULT_CONFIRM flag in rflags, nfs4.1 client
calls pure NFSv4.0 function that shouldn't be used and this flags should
be ignored.

Signed-off-by: Vitaliy Gusev <gusev.vitaliy@gmail.com>
---
 fs/nfs/nfs4proc.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Trond Myklebust Oct. 31, 2016, 3:46 p.m. UTC | #1
DQo+IE9uIE9jdCAzMCwgMjAxNiwgYXQgMjE6MTgsIFZpdGFsaXkgR3VzZXYgPGd1c2V2LnZpdGFs
aXlAZ21haWwuY29tPiB3cm90ZToNCj4gDQo+IEhpLg0KPiANCj4gRHVyaW5nIHNvbWUgdGVzdHMg
SeKAmXZlIHNlZW4gbmZzLWNsaWVudCBjcmFzaGVzLiBJdCB3YXMganVzdCByZWFkaW5nIGZpbGUg
dmlhIE5GU3Y0LjEgcHJvdG9jb2wuIA0KPiANCj4gVGhlIHBhbmljIG1lc3NhZ2UgaXMgYmVsb3cs
ICBmaXhpbmcgcGF0Y2ggaXMgYXR0YWNoZWQuDQoNCldoeSBkb2VzIHRoaXMgbmVlZCB0byBiZSBm
aXhlZCBvbiB0aGUgY2xpZW50PyBJdCBsb29rcyBsaWtlIGEgc2VydmVyIGJ1Z+KApiBJbiBhbnkg
Y2FzZSwgdGhlIGZpeCB5b3UgcHJvcG9zZSBpcyBnb2luZyB0byBsZWF2ZSB0aGUgY2xpZW50IHdp
dGggYnJva2VuIG9wZW4gc3RhdGUuDQoNCj4gDQo+IOKAlOKAlOKAlA0KPiAiQlVHOiB1bmFibGUg
dG8gaGFuZGxlIGtlcm5lbCBOVUxMIHBvaW50ZXIgZGVyZWZlcmVuY2UgYXQgMDAwMDAwMDAwMDAw
MDA5MA0KPiAiSVA6IFs8ZmZmZmZmZmZiMDQ5ZWVmYz5dIF9yYXdfc3Bpbl9sb2NrKzB4Yy8weDMw
DQo+IFBHRCAyYTFjMDY3IFBVRCAyOWU1MDY3IFBNRCAwDQo+IE9vcHM6IDAwMDIgWyMxXSBTTVAN
Cj4gQ1BVOiAxIFBJRDogMzU3MyBDb21tOiBrd29ya2VyLzE6MCBOb3QgdGFpbnRlZCA0LjguMC0y
Ni1nZW5lcmljICMyOC1VYnVudHUNCj4gSGFyZHdhcmUgbmFtZTogVk13YXJlLCBJbmMuIFZNd2Fy
ZSBWaXJ0dWFsIFBsYXRmb3JtLzQ0MEJYIERlc2t0b3AgUmVmZXJlbmNlIFBsYXRmb3JtLCBCSU9T
IDYuMDAgMDcvMDIvMjAxNQ0KPiBXb3JrcXVldWU6IHJwY2lvZCBycGNfYXN5bmNfc2NoZWR1bGUg
W3N1bnJwY10NCj4gdGFzazogZmZmZmEwMTdjYjUzNDc0MCB0YXNrLnN0YWNrOiBmZmZmYTAxNzgy
YjQ0MDAwDQo+IFJJUDogMDAxMDpbPGZmZmZmZmZmYjA0OWVlZmM+XSAgWzxmZmZmZmZmZmIwNDll
ZWZjPl0gX3Jhd19zcGluX2xvY2srMHhjLzB4MzBOXg0KPiBSU1A6IDAwMTg6ZmZmZmEwMTc4MmI0
N2Q0OCAgRUZMQUdTOiAwMDAxMDI0Ng0KPiBSQVg6IDAwMDAwMDAwMDAwMDAwMDAgUkJYOiBmZmZm
YTAxN2M3MzVkMjA4IFJDWDogZmZmZmEwMTc4MzgxNmMwMA0KPiBSRFg6IDAwMDAwMDAwMDAwMDAw
MDEgUlNJOiBmZmZmYTAxN2M3MzVkMWUwIFJESTogMDAwMDAwMDAwMDAwMDA5MA0KPiBSQlA6IGZm
ZmZhMDE3ODJiNDdkNzggUjA4OiBmZmZmYTAxN2NlYzU4YTAwIFIwOTogMDAwMDAwMDAwMDAwMDAw
MA0KPiBSMTA6IDAwMDAwMDAwMDAwMDAwMDAgUjExOiAwMDAwMDBiNDA5OGVkMDM2IFIxMjogZmZm
ZmEwMTc4MzgxNmMwMA0KPiBSMTM6IDAwMDAwMDAwMDAwMDAwMDAgUjE0OiAwMDAwMDAwMDAwMDAw
MDkwIFIxNTogZmZmZmEwMTdjNzM1ZDFlMA0KPiBGUzogIDAwMDAwMDAwMDAwMDAwMDAoMDAwMCkg
R1M6ZmZmZmEwMTdjZWM0MDAwMCgwMDAwKSBrbmxHUzowMDAwMDAwMDAwMDAwMDAwDQo+IENTOiAg
MDAxMCBEUzogMDAwMCBFUzogMDAwMCBDUjA6IDAwMDAwMDAwODAwNTAwMzMNCj4gQ1IyOiAwMDAw
MDAwMDAwMDAwMDkwIENSMzogMDAwMDAwMDAwMmEwYzAwMCBDUjQ6IDAwMDAwMDAwMDAxNDA2ZTAN
Cj4gU3RhY2s6DQo+IGZmZmZmZmZmYzA4MzYyMDggZmZmZmEwMTc4MzgxNmMwMCBmZmZmZmZmZmMw
NGM5MDcwIGZmZmZmZmZmYzA0YzkwNzANCj4gMDAwMDAwMDAwMDAwMDAwMCBmZmZmYTAxNzgzODE2
YzQwIGZmZmZhMDE3ODJiNDdkODggZmZmZmZmZmZjMDgzNjJkMA0KPiBmZmZmYTAxNzgyYjQ3ZDk4
IGZmZmZmZmZmYzA0YzkwODkgZmZmZmEwMTc4MmI0N2UwMCBmZmZmZmZmZmMwNGNiNmZkDQo+IA0K
PiBDYWxsIFRyYWNlOg0KPiBbPGZmZmZmZmZmYzA4MzYyMDg+XSA/IG5mczQwX3NldHVwX3NlcXVl
bmNlKzB4NDgvMHhlMCBbbmZzdjRdDQo+IFs8ZmZmZmZmZmZjMDgzNjJkMD5dIG5mczRfb3Blbl9j
b25maXJtX3ByZXBhcmUrMHgzMC8weDQwIFtuZnN2NF0NCj4gWzxmZmZmZmZmZmMwNGM5MDg5Pl0g
cnBjX3ByZXBhcmVfdGFzaysweDE5LzB4MjAgW3N1bnJwY10NCj4gWzxmZmZmZmZmZmMwNGNiNmZk
Pl0gX19ycGNfZXhlY3V0ZSsweDhkLzB4NDIwIFtzdW5ycGNdDQo+IFs8ZmZmZmZmZmZjMDRjYmFh
Mj5dIHJwY19hc3luY19zY2hlZHVsZSsweDEyLzB4MjAgW3N1bnJwY10NCj4gWzxmZmZmZmZmZmFm
YzlkNjFjPl0gcHJvY2Vzc19vbmVfd29yaysweDFmYy8weDRiMA0KPiBbPGZmZmZmZmZmYWZjOWQ5
MWI+XSB3b3JrZXJfdGhyZWFkKzB4NGIvMHg1MDANCj4gWzxmZmZmZmZmZmFmY2EzYzE4Pl0ga3Ro
cmVhZCsweGQ4LzB4ZjANCj4gWzxmZmZmZmZmZmIwNDlmMjlmPl0gcmV0X2Zyb21fZm9yaysweDFm
LzB4NDANCj4gWzxmZmZmZmZmZmFmY2EzYjQwPl0gPyBrdGhyZWFkX2NyZWF0ZV9vbl9ub2RlKzB4
MWUwLzB4MWUwDQo+IENvZGU6IDAwIDAxIDAwIDAwIGYwIDBmIGMxIDM3IDgxIGM2IDAwIDAxIDAw
IDAwIDQwIDg0IGY2IDc1IDAxIGMzIDU1IDQ4IDg5IGU1IGU4IGUyIDE5IDgzIGZmIDVkIGMzIDBm
IDFmIDQ0IDAwIDAwIDMxIGMwIGJhIDAxIDAwIDAwIDAwIDxmMD4gMGYgYjEgMTcgODUgYzAgNzUg
MDEgYzMgNTUgODkgYzYgNDggODkgZTUgZTggYzAgMDEgODMgZmYgNjYgDQo+ICJSSVAgIFs8ZmZm
ZmZmZmZiMDQ5ZWVmYz5dIF9yYXdfc3Bpbl9sb2NrKzB4Yy8weDMwDQo+IA0KPiDigJTigJTigJQN
Cg0K

--
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
Vitaliy Gusev Oct. 31, 2016, 5:31 p.m. UTC | #2
> On 31 Oct 2016, at 18:46, Trond Myklebust <trondmy@primarydata.com> wrote:
> 
> 
>> On Oct 30, 2016, at 21:18, Vitaliy Gusev <gusev.vitaliy@gmail.com> wrote:
>> 
>> Hi.
>> 
>> During some tests I’ve seen nfs-client crashes. It was just reading file via NFSv4.1 protocol. 
>> 
>> The panic message is below,  fixing patch is attached.
> 
> Why does this need to be fixed on the client? It looks like a server bug… In any case, the fix you propose is going to leave the client with broken open state.

Do you like to get crash every time a remote side sends improper datas? I believe not.

I proposed just ignore the flag OPEN4_RESULT_CONFIRM  for nfs4.1+ clients.
RFC5661 has description that allows a client to do that:

   o  OPEN4_RESULT_CONFIRM is deprecated and MUST NOT be returned by an
       NFSv4.1 server.

———

Thanks,
Vitaliy Gusev

--
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 Oct. 31, 2016, 5:54 p.m. UTC | #3
> On Oct 31, 2016, at 13:31, Vitaliy Gusev <gusev.vitaliy@gmail.com> wrote:

> 

> 

>> On 31 Oct 2016, at 18:46, Trond Myklebust <trondmy@primarydata.com> wrote:

>> 

>> 

>>> On Oct 30, 2016, at 21:18, Vitaliy Gusev <gusev.vitaliy@gmail.com> wrote:

>>> 

>>> Hi.

>>> 

>>> During some tests I’ve seen nfs-client crashes. It was just reading file via NFSv4.1 protocol. 

>>> 

>>> The panic message is below,  fixing patch is attached.

>> 

>> Why does this need to be fixed on the client? It looks like a server bug… In any case, the fix you propose is going to leave the client with broken open state.

> 

> Do you like to get crash every time a remote side sends improper datas? I believe not.


There are a million other ways to screw a client over if your server is buggy or compromised.

> 

> I proposed just ignore the flag OPEN4_RESULT_CONFIRM  for nfs4.1+ clients.

> RFC5661 has description that allows a client to do that:

> 

>   o  OPEN4_RESULT_CONFIRM is deprecated and MUST NOT be returned by an

>       NFSv4.1 server.


I know what the spec says. The point is that the client will leak memory, and fail to handle the situation correctly when the server returns OPEN4_RESULT_CONFIRM with or with the patch that you are proposing.

The right thing to do here would rather be to print out a big fat warning to the user, and then possibly also to kill the mount.
Vitaliy Gusev Oct. 31, 2016, 8:30 p.m. UTC | #4
> On 31 Oct 2016, at 20:54, Trond Myklebust <trondmy@primarydata.com> wrote:
> 
>> 
>> On Oct 31, 2016, at 13:31, Vitaliy Gusev <gusev.vitaliy@gmail.com> wrote:
>> Do you like to get crash every time a remote side sends improper datas? I believe not.
> 
> There are a million other ways to screw a client over if your server is buggy or compromised.

I agree, but crashes are not right way to work with incorrect datas and that’s why I reported problem. 


>> 
>> I proposed just ignore the flag OPEN4_RESULT_CONFIRM  for nfs4.1+ clients.
>> RFC5661 has description that allows a client to do that:
>> 
>>  o  OPEN4_RESULT_CONFIRM is deprecated and MUST NOT be returned by an
>>      NFSv4.1 server.
> 
> I know what the spec says. The point is that the client will leak memory, and fail to handle the situation correctly when the server returns OPEN4_RESULT_CONFIRM with or with the patch that you are proposing.
> The right thing to do here would rather be to print out a big fat warning to the user, and then possibly also to kill the mount.

That’s a good point. What if return error for OPEN operation instead of kill the mount?

———
Thanks,
Vitaliy Gusev


---
Vitaliy Gusev


--
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

Patch
diff mbox

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index a9dec32..054ce67 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -2023,6 +2023,9 @@  static int _nfs4_proc_open_confirm(struct nfs4_opendata *data)
 	};
 	int status;
 
+	if (server->nfs_client->cl_mvops->minor_version != 0)
+		return 0;
+
 	nfs4_init_sequence(&data->c_arg.seq_args, &data->c_res.seq_res, 1);
 	kref_get(&data->kref);
 	data->rpc_done = 0;