[PATCH 1/1] NFS guard against incorrect vers inputs from nfs-utils

[PATCH 1/1] NFS guard against incorrect vers inputs from nfs-utils

[Olga Kornievskaia]

It is possible that userland can pass to the kernel mismatching
inputs for the minorversion. like vers=4.1,minorversion=0. Instead
of making the kernel responposible for 'choosing' the minorversion,
make the userland always responsible for not sending a mismatch.

Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfs/super.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 29bacdc..90c0584 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -1207,7 +1207,7 @@ static int nfs_parse_mount_options(char *raw,
 				   struct nfs_parsed_mount_data *mnt)
 {
 	char *p, *string, *secdata;
-	int rc, sloppy = 0, invalid_option = 0;
+	int rc, sloppy = 0, invalid_option = 0, minorversion = -1;
 	unsigned short protofamily = AF_UNSPEC;
 	unsigned short mountfamily = AF_UNSPEC;
 
@@ -1419,6 +1419,7 @@ static int nfs_parse_mount_options(char *raw,
 			if (option > NFS4_MAX_MINOR_VERSION)
 				goto out_invalid_value;
 			mnt->minorversion = option;
+			minorversion = option;
 			break;
 
 		/*
@@ -1655,6 +1656,9 @@ static int nfs_parse_mount_options(char *raw,
 		}
 	}
 
+	if (minorversion >= 0 && minorversion != mnt->minorversion)
+		goto out_mountvers_mismatch;
+
 	return 1;
 
 out_mountproto_mismatch:
@@ -1685,6 +1689,10 @@ static int nfs_parse_mount_options(char *raw,
 	free_secdata(secdata);
 	printk(KERN_INFO "NFS: security options invalid: %d\n", rc);
 	return 0;
+out_mountvers_mismatch:
+	printk(KERN_INFO "NFS: mismatch versions supplied vers=4.%d and "
+			 "minorversion=%d\n", mnt->minorversion, minorversion);
+	return 0;
 }
 
 /*
-- 
1.8.3.1

Re: [PATCH 1/1] NFS guard against incorrect vers inputs from nfs-utils

[Steve Dickson]

Hey Olga,

On 02/22/2018 02:28 PM, Olga Kornievskaia wrote:
> It is possible that userland can pass to the kernel mismatching
> inputs for the minorversion. like vers=4.1,minorversion=0. Instead
> of making the kernel responposible for 'choosing' the minorversion,
> make the userland always responsible for not sending a mismatch.
I'm thinking this is probably more of mount problem...

mount -t nfs4 -o minorversion=0 server:/export /mnt

shouldn't this be a  v4.0 mount instead of a 4.2 mount?

steved. 

> 
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  fs/nfs/super.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index 29bacdc..90c0584 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -1207,7 +1207,7 @@ static int nfs_parse_mount_options(char *raw,
>  				   struct nfs_parsed_mount_data *mnt)
>  {
>  	char *p, *string, *secdata;
> -	int rc, sloppy = 0, invalid_option = 0;
> +	int rc, sloppy = 0, invalid_option = 0, minorversion = -1;
>  	unsigned short protofamily = AF_UNSPEC;
>  	unsigned short mountfamily = AF_UNSPEC;
>  
> @@ -1419,6 +1419,7 @@ static int nfs_parse_mount_options(char *raw,
>  			if (option > NFS4_MAX_MINOR_VERSION)
>  				goto out_invalid_value;
>  			mnt->minorversion = option;
> +			minorversion = option;
>  			break;
>  
>  		/*
> @@ -1655,6 +1656,9 @@ static int nfs_parse_mount_options(char *raw,
>  		}
>  	}
>  
> +	if (minorversion >= 0 && minorversion != mnt->minorversion)
> +		goto out_mountvers_mismatch;
> +
>  	return 1;
>  
>  out_mountproto_mismatch:
> @@ -1685,6 +1689,10 @@ static int nfs_parse_mount_options(char *raw,
>  	free_secdata(secdata);
>  	printk(KERN_INFO "NFS: security options invalid: %d\n", rc);
>  	return 0;
> +out_mountvers_mismatch:
> +	printk(KERN_INFO "NFS: mismatch versions supplied vers=4.%d and "
> +			 "minorversion=%d\n", mnt->minorversion, minorversion);
> +	return 0;
>  }
>  
>  /*
> 

Re: [PATCH 1/1] NFS guard against incorrect vers inputs from nfs-utils

[Olga Kornievskaia]

On Fri, Feb 23, 2018 at 11:24 AM, Steve Dickson <SteveD@redhat.com> wrote:
> Hey Olga,
>
> On 02/22/2018 02:28 PM, Olga Kornievskaia wrote:
>> It is possible that userland can pass to the kernel mismatching
>> inputs for the minorversion. like vers=4.1,minorversion=0. Instead
>> of making the kernel responposible for 'choosing' the minorversion,
>> make the userland always responsible for not sending a mismatch.
> I'm thinking this is probably more of mount problem...

Yes the problem is a broken user land sending incorrect arguments but
I still think the kernel needs to do sanity checking on arguments to
prevent this from happening again.

> mount -t nfs4 -o minorversion=0 server:/export /mnt
>
> shouldn't this be a  v4.0 mount instead of a 4.2 mount?

Yes I would think this should create a v4.0 mount.

>
> steved.
>
>>
>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>> ---
>>  fs/nfs/super.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
>> index 29bacdc..90c0584 100644
>> --- a/fs/nfs/super.c
>> +++ b/fs/nfs/super.c
>> @@ -1207,7 +1207,7 @@ static int nfs_parse_mount_options(char *raw,
>>                                  struct nfs_parsed_mount_data *mnt)
>>  {
>>       char *p, *string, *secdata;
>> -     int rc, sloppy = 0, invalid_option = 0;
>> +     int rc, sloppy = 0, invalid_option = 0, minorversion = -1;
>>       unsigned short protofamily = AF_UNSPEC;
>>       unsigned short mountfamily = AF_UNSPEC;
>>
>> @@ -1419,6 +1419,7 @@ static int nfs_parse_mount_options(char *raw,
>>                       if (option > NFS4_MAX_MINOR_VERSION)
>>                               goto out_invalid_value;
>>                       mnt->minorversion = option;
>> +                     minorversion = option;
>>                       break;
>>
>>               /*
>> @@ -1655,6 +1656,9 @@ static int nfs_parse_mount_options(char *raw,
>>               }
>>       }
>>
>> +     if (minorversion >= 0 && minorversion != mnt->minorversion)
>> +             goto out_mountvers_mismatch;
>> +
>>       return 1;
>>
>>  out_mountproto_mismatch:
>> @@ -1685,6 +1689,10 @@ static int nfs_parse_mount_options(char *raw,
>>       free_secdata(secdata);
>>       printk(KERN_INFO "NFS: security options invalid: %d\n", rc);
>>       return 0;
>> +out_mountvers_mismatch:
>> +     printk(KERN_INFO "NFS: mismatch versions supplied vers=4.%d and "
>> +                      "minorversion=%d\n", mnt->minorversion, minorversion);
>> +     return 0;
>>  }
>>
>>  /*
>>
> --
> 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

Re: [PATCH 1/1] NFS guard against incorrect vers inputs from nfs-utils

[Steve Dickson]

On 02/23/2018 12:05 PM, Olga Kornievskaia wrote:
> On Fri, Feb 23, 2018 at 11:24 AM, Steve Dickson <SteveD@redhat.com> wrote:
>> Hey Olga,
>>
>> On 02/22/2018 02:28 PM, Olga Kornievskaia wrote:
>>> It is possible that userland can pass to the kernel mismatching
>>> inputs for the minorversion. like vers=4.1,minorversion=0. Instead
>>> of making the kernel responposible for 'choosing' the minorversion,
>>> make the userland always responsible for not sending a mismatch.
>> I'm thinking this is probably more of mount problem...
> 
> Yes the problem is a broken user land sending incorrect arguments but
> I still think the kernel needs to do sanity checking on arguments to
> prevent this from happening again.
> 
>> mount -t nfs4 -o minorversion=0 server:/export /mnt
>>
>> shouldn't this be a  v4.0 mount instead of a 4.2 mount?
> 
> Yes I would think this should create a v4.0 mount.
Way back when... there was some talk about deprecating 
minorversion= flag... Since it is broken, maybe this is 
a good time to do it?

Thoughts?

steved.

Re: [PATCH 1/1] NFS guard against incorrect vers inputs from nfs-utils

[Trond Myklebust]

T24gRnJpLCAyMDE4LTAyLTIzIGF0IDEzOjQ1IC0wNTAwLCBTdGV2ZSBEaWNrc29uIHdyb3RlOg0K
PiANCj4gT24gMDIvMjMvMjAxOCAxMjowNSBQTSwgT2xnYSBLb3JuaWV2c2thaWEgd3JvdGU6DQo+
ID4gT24gRnJpLCBGZWIgMjMsIDIwMTggYXQgMTE6MjQgQU0sIFN0ZXZlIERpY2tzb24gPFN0ZXZl
REByZWRoYXQuY29tPg0KPiA+IHdyb3RlOg0KPiA+ID4gSGV5IE9sZ2EsDQo+ID4gPiANCj4gPiA+
IE9uIDAyLzIyLzIwMTggMDI6MjggUE0sIE9sZ2EgS29ybmlldnNrYWlhIHdyb3RlOg0KPiA+ID4g
PiBJdCBpcyBwb3NzaWJsZSB0aGF0IHVzZXJsYW5kIGNhbiBwYXNzIHRvIHRoZSBrZXJuZWwgbWlz
bWF0Y2hpbmcNCj4gPiA+ID4gaW5wdXRzIGZvciB0aGUgbWlub3J2ZXJzaW9uLiBsaWtlIHZlcnM9
NC4xLG1pbm9ydmVyc2lvbj0wLg0KPiA+ID4gPiBJbnN0ZWFkDQo+ID4gPiA+IG9mIG1ha2luZyB0
aGUga2VybmVsIHJlc3BvbnBvc2libGUgZm9yICdjaG9vc2luZycgdGhlDQo+ID4gPiA+IG1pbm9y
dmVyc2lvbiwNCj4gPiA+ID4gbWFrZSB0aGUgdXNlcmxhbmQgYWx3YXlzIHJlc3BvbnNpYmxlIGZv
ciBub3Qgc2VuZGluZyBhDQo+ID4gPiA+IG1pc21hdGNoLg0KPiA+ID4gDQo+ID4gPiBJJ20gdGhp
bmtpbmcgdGhpcyBpcyBwcm9iYWJseSBtb3JlIG9mIG1vdW50IHByb2JsZW0uLi4NCj4gPiANCj4g
PiBZZXMgdGhlIHByb2JsZW0gaXMgYSBicm9rZW4gdXNlciBsYW5kIHNlbmRpbmcgaW5jb3JyZWN0
IGFyZ3VtZW50cw0KPiA+IGJ1dA0KPiA+IEkgc3RpbGwgdGhpbmsgdGhlIGtlcm5lbCBuZWVkcyB0
byBkbyBzYW5pdHkgY2hlY2tpbmcgb24gYXJndW1lbnRzDQo+ID4gdG8NCj4gPiBwcmV2ZW50IHRo
aXMgZnJvbSBoYXBwZW5pbmcgYWdhaW4uDQo+ID4gDQo+ID4gPiBtb3VudCAtdCBuZnM0IC1vIG1p
bm9ydmVyc2lvbj0wIHNlcnZlcjovZXhwb3J0IC9tbnQNCj4gPiA+IA0KPiA+ID4gc2hvdWxkbid0
IHRoaXMgYmUgYSAgdjQuMCBtb3VudCBpbnN0ZWFkIG9mIGEgNC4yIG1vdW50Pw0KPiA+IA0KPiA+
IFllcyBJIHdvdWxkIHRoaW5rIHRoaXMgc2hvdWxkIGNyZWF0ZSBhIHY0LjAgbW91bnQuDQo+IA0K
PiBXYXkgYmFjayB3aGVuLi4uIHRoZXJlIHdhcyBzb21lIHRhbGsgYWJvdXQgZGVwcmVjYXRpbmcg
DQo+IG1pbm9ydmVyc2lvbj0gZmxhZy4uLiBTaW5jZSBpdCBpcyBicm9rZW4sIG1heWJlIHRoaXMg
aXMgDQo+IGEgZ29vZCB0aW1lIHRvIGRvIGl0Pw0KPiANCj4gVGhvdWdodHM/DQoNCkkndmUgbmV2
ZXIgbGlrZWQgdGhlIHNlcGFyYXRlIHY0LW9ubHkgJ21pbm9ydmVyc2lvbicga2V5d29yZCwgd2hp
Y2ggaXMNCndoeSBJIGludHJvZHVjZWQgdGhlICd2ZXJzaW9uPTQueCcgZm9ybWF0LiBNeSBwcmVm
ZXJlbmNlIGlzIHRoZXJlZm9yZQ0KdGhhdCB3ZSBjb250aW51ZSB0byBkZXByZWNhdGUgdXNlIG9m
ICdtaW5vcnZlcnNpb24nIHdpdGggYSB2aWV3IHRvDQpraWxsaW5nIGl0IG9mZiBjb21wbGV0ZWx5
Lg0KDQotLSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyLCBQ
cmltYXJ5RGF0YQ0KdHJvbmQubXlrbGVidXN0QHByaW1hcnlkYXRhLmNvbQ0K

Re: [PATCH 1/1] NFS guard against incorrect vers inputs from nfs-utils

[Olga Kornievskaia]

On Fri, Feb 23, 2018 at 2:09 PM, Trond Myklebust
<trondmy@primarydata.com> wrote:
> On Fri, 2018-02-23 at 13:45 -0500, Steve Dickson wrote:
>>
>> On 02/23/2018 12:05 PM, Olga Kornievskaia wrote:
>> > On Fri, Feb 23, 2018 at 11:24 AM, Steve Dickson <SteveD@redhat.com>
>> > wrote:
>> > > Hey Olga,
>> > >
>> > > On 02/22/2018 02:28 PM, Olga Kornievskaia wrote:
>> > > > It is possible that userland can pass to the kernel mismatching
>> > > > inputs for the minorversion. like vers=4.1,minorversion=0.
>> > > > Instead
>> > > > of making the kernel responposible for 'choosing' the
>> > > > minorversion,
>> > > > make the userland always responsible for not sending a
>> > > > mismatch.
>> > >
>> > > I'm thinking this is probably more of mount problem...
>> >
>> > Yes the problem is a broken user land sending incorrect arguments
>> > but
>> > I still think the kernel needs to do sanity checking on arguments
>> > to
>> > prevent this from happening again.
>> >
>> > > mount -t nfs4 -o minorversion=0 server:/export /mnt
>> > >
>> > > shouldn't this be a  v4.0 mount instead of a 4.2 mount?
>> >
>> > Yes I would think this should create a v4.0 mount.
>>
>> Way back when... there was some talk about deprecating
>> minorversion= flag... Since it is broken, maybe this is
>> a good time to do it?
>>
>> Thoughts?
>
> I've never liked the separate v4-only 'minorversion' keyword, which is
> why I introduced the 'version=4.x' format. My preference is therefore
> that we continue to deprecate use of 'minorversion' with a view to
> killing it off completely.

It all seems like a good idea except I wonder how widely used and
relied on "minorversion" option is. Steve you might have a good idea
about it.

>
> --
> Trond Myklebust
> Linux NFS client maintainer, PrimaryData
> trond.myklebust@primarydata.com

Re: [PATCH 1/1] NFS guard against incorrect vers inputs from nfs-utils

[Steve Dickson]

On 02/23/2018 02:28 PM, Olga Kornievskaia wrote:
> On Fri, Feb 23, 2018 at 2:09 PM, Trond Myklebust
> <trondmy@primarydata.com> wrote:
>> On Fri, 2018-02-23 at 13:45 -0500, Steve Dickson wrote:
>>>
>>> On 02/23/2018 12:05 PM, Olga Kornievskaia wrote:
>>>> On Fri, Feb 23, 2018 at 11:24 AM, Steve Dickson <SteveD@redhat.com>
>>>> wrote:
>>>>> Hey Olga,
>>>>>
>>>>> On 02/22/2018 02:28 PM, Olga Kornievskaia wrote:
>>>>>> It is possible that userland can pass to the kernel mismatching
>>>>>> inputs for the minorversion. like vers=4.1,minorversion=0.
>>>>>> Instead
>>>>>> of making the kernel responposible for 'choosing' the
>>>>>> minorversion,
>>>>>> make the userland always responsible for not sending a
>>>>>> mismatch.
>>>>>
>>>>> I'm thinking this is probably more of mount problem...
>>>>
>>>> Yes the problem is a broken user land sending incorrect arguments
>>>> but
>>>> I still think the kernel needs to do sanity checking on arguments
>>>> to
>>>> prevent this from happening again.
>>>>
>>>>> mount -t nfs4 -o minorversion=0 server:/export /mnt
>>>>>
>>>>> shouldn't this be a  v4.0 mount instead of a 4.2 mount?
>>>>
>>>> Yes I would think this should create a v4.0 mount.
>>>
>>> Way back when... there was some talk about deprecating
>>> minorversion= flag... Since it is broken, maybe this is
>>> a good time to do it?
>>>
>>> Thoughts?
>>
>> I've never liked the separate v4-only 'minorversion' keyword, which is
>> why I introduced the 'version=4.x' format. My preference is therefore
>> that we continue to deprecate use of 'minorversion' with a view to
>> killing it off completely.
> 
> It all seems like a good idea except I wonder how widely used and
> relied on "minorversion" option is. Steve you might have a good idea
> about it.
I have no idea... Actually I'm not sure how to deprecate a mount
option... I don't think that has every happen before... But 
I'm sure it will break something! ;-) 

steved.

> 
>>
>> --
>> Trond Myklebust
>> Linux NFS client maintainer, PrimaryData
>> trond.myklebust@primarydata.com