2016-10-31 01:18:06

by Vitaliy Gusev

[permalink] [raw]
Subject: [PATCH] NFSv41: fix NULL dereference in nfs40_setup_sequence

Hi.

During some tests I=E2=80=99ve seen nfs-client crashes. It was just =
reading file via NFSv4.1 protocol.=20

The panic message is below, fixing patch is attached.

=E2=80=94=E2=80=94=E2=80=94
"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=20
"RIP [<ffffffffb049eefc>] _raw_spin_lock+0xc/0x30

=E2=80=94=E2=80=94=E2=80=94

=46rom 1b5e4636c696a169f46b474de519f1d4d4f06277 Mon Sep 17 00:00:00 2001
From: Vitaliy Gusev <[email protected]>
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 <[email protected]>
---
fs/nfs/nfs4proc.c | 3 +++
1 file changed, 3 insertions(+)

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;
=20
+ if (server->nfs_client->cl_mvops->minor_version !=3D 0)
+ return 0;
+
nfs4_init_sequence(&data->c_arg.seq_args, &data->c_res.seq_res, =
1);
kref_get(&data->kref);
data->rpc_done =3D 0;
--=20
2.6.4







2016-10-31 15:46:56

by Trond Myklebust

[permalink] [raw]
Subject: Re: [PATCH] NFSv41: fix NULL dereference in nfs40_setup_sequence

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


2016-10-31 17:31:11

by Vitaliy Gusev

[permalink] [raw]
Subject: Re: [PATCH] NFSv41: fix NULL dereference in nfs40_setup_sequence


> On 31 Oct 2016, at 18:46, Trond Myklebust <[email protected]> =
wrote:
>=20
>=20
>> On Oct 30, 2016, at 21:18, Vitaliy Gusev <[email protected]> =
wrote:
>>=20
>> Hi.
>>=20
>> During some tests I=E2=80=99ve seen nfs-client crashes. It was just =
reading file via NFSv4.1 protocol.=20
>>=20
>> The panic message is below, fixing patch is attached.
>=20
> Why does this need to be fixed on the client? It looks like a server =
bug=E2=80=A6 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.

=E2=80=94=E2=80=94=E2=80=94

Thanks,
Vitaliy Gusev


2016-10-31 17:54:53

by Trond Myklebust

[permalink] [raw]
Subject: Re: [PATCH] NFSv41: fix NULL dereference in nfs40_setup_sequence

DQo+IE9uIE9jdCAzMSwgMjAxNiwgYXQgMTM6MzEsIFZpdGFsaXkgR3VzZXYgPGd1c2V2LnZpdGFs
aXlAZ21haWwuY29tPiB3cm90ZToNCj4gDQo+IA0KPj4gT24gMzEgT2N0IDIwMTYsIGF0IDE4OjQ2
LCBUcm9uZCBNeWtsZWJ1c3QgPHRyb25kbXlAcHJpbWFyeWRhdGEuY29tPiB3cm90ZToNCj4+IA0K
Pj4gDQo+Pj4gT24gT2N0IDMwLCAyMDE2LCBhdCAyMToxOCwgVml0YWxpeSBHdXNldiA8Z3VzZXYu
dml0YWxpeUBnbWFpbC5jb20+IHdyb3RlOg0KPj4+IA0KPj4+IEhpLg0KPj4+IA0KPj4+IER1cmlu
ZyBzb21lIHRlc3RzIEnigJl2ZSBzZWVuIG5mcy1jbGllbnQgY3Jhc2hlcy4gSXQgd2FzIGp1c3Qg
cmVhZGluZyBmaWxlIHZpYSBORlN2NC4xIHByb3RvY29sLiANCj4+PiANCj4+PiBUaGUgcGFuaWMg
bWVzc2FnZSBpcyBiZWxvdywgIGZpeGluZyBwYXRjaCBpcyBhdHRhY2hlZC4NCj4+IA0KPj4gV2h5
IGRvZXMgdGhpcyBuZWVkIHRvIGJlIGZpeGVkIG9uIHRoZSBjbGllbnQ/IEl0IGxvb2tzIGxpa2Ug
YSBzZXJ2ZXIgYnVn4oCmIEluIGFueSBjYXNlLCB0aGUgZml4IHlvdSBwcm9wb3NlIGlzIGdvaW5n
IHRvIGxlYXZlIHRoZSBjbGllbnQgd2l0aCBicm9rZW4gb3BlbiBzdGF0ZS4NCj4gDQo+IERvIHlv
dSBsaWtlIHRvIGdldCBjcmFzaCBldmVyeSB0aW1lIGEgcmVtb3RlIHNpZGUgc2VuZHMgaW1wcm9w
ZXIgZGF0YXM/IEkgYmVsaWV2ZSBub3QuDQoNClRoZXJlIGFyZSBhIG1pbGxpb24gb3RoZXIgd2F5
cyB0byBzY3JldyBhIGNsaWVudCBvdmVyIGlmIHlvdXIgc2VydmVyIGlzIGJ1Z2d5IG9yIGNvbXBy
b21pc2VkLg0KDQo+IA0KPiBJIHByb3Bvc2VkIGp1c3QgaWdub3JlIHRoZSBmbGFnIE9QRU40X1JF
U1VMVF9DT05GSVJNICBmb3IgbmZzNC4xKyBjbGllbnRzLg0KPiBSRkM1NjYxIGhhcyBkZXNjcmlw
dGlvbiB0aGF0IGFsbG93cyBhIGNsaWVudCB0byBkbyB0aGF0Og0KPiANCj4gICBvICBPUEVONF9S
RVNVTFRfQ09ORklSTSBpcyBkZXByZWNhdGVkIGFuZCBNVVNUIE5PVCBiZSByZXR1cm5lZCBieSBh
bg0KPiAgICAgICBORlN2NC4xIHNlcnZlci4NCg0KSSBrbm93IHdoYXQgdGhlIHNwZWMgc2F5cy4g
VGhlIHBvaW50IGlzIHRoYXQgdGhlIGNsaWVudCB3aWxsIGxlYWsgbWVtb3J5LCBhbmQgZmFpbCB0
byBoYW5kbGUgdGhlIHNpdHVhdGlvbiBjb3JyZWN0bHkgd2hlbiB0aGUgc2VydmVyIHJldHVybnMg
T1BFTjRfUkVTVUxUX0NPTkZJUk0gd2l0aCBvciB3aXRoIHRoZSBwYXRjaCB0aGF0IHlvdSBhcmUg
cHJvcG9zaW5nLg0KDQpUaGUgcmlnaHQgdGhpbmcgdG8gZG8gaGVyZSB3b3VsZCByYXRoZXIgYmUg
dG8gcHJpbnQgb3V0IGEgYmlnIGZhdCB3YXJuaW5nIHRvIHRoZSB1c2VyLCBhbmQgdGhlbiBwb3Nz
aWJseSBhbHNvIHRvIGtpbGwgdGhlIG1vdW50Lg==


2016-10-31 20:30:41

by Vitaliy Gusev

[permalink] [raw]
Subject: Re: [PATCH] NFSv41: fix NULL dereference in nfs40_setup_sequence


> On 31 Oct 2016, at 20:54, Trond Myklebust <[email protected]> =
wrote:
>=20
>>=20
>> On Oct 31, 2016, at 13:31, Vitaliy Gusev <[email protected]> =
wrote:
>> Do you like to get crash every time a remote side sends improper =
datas? I believe not.
>=20
> 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=E2=80=99s why I reported problem.=20


>>=20
>> I proposed just ignore the flag OPEN4_RESULT_CONFIRM for nfs4.1+ =
clients.
>> RFC5661 has description that allows a client to do that:
>>=20
>> o OPEN4_RESULT_CONFIRM is deprecated and MUST NOT be returned by an
>> NFSv4.1 server.
>=20
> 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=E2=80=99s a good point. What if return error for OPEN operation =
instead of kill the mount?

=E2=80=94=E2=80=94=E2=80=94
Thanks,
Vitaliy Gusev


---
Vitaliy Gusev