Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i31e841b0:Fastmail
Message-ID: <ccd23a54-27b5-e65c-4a97-b169676c23bc@themaw.net>
Date:   Wed, 29 Jun 2022 09:02:48 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
Subject: Re: [REPOST PATCH] nfs: fix port value parsing
Content-Language: en-US
To:     Trond Myklebust <trondmy@hammerspace.com>,
        "Anna.Schumaker@Netapp.com" <Anna.Schumaker@Netapp.com>
Cc:     "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
        "SteveD@redhat.com" <SteveD@redhat.com>,
        "bcodding@redhat.com" <bcodding@redhat.com>,
        "dhowells@redhat.com" <dhowells@redhat.com>
References: <165637590710.37553.7481596265813355098.stgit@donald.themaw.net>
 <cadcb382d47ef037c5b713b099ae46640dfea37d.camel@hammerspace.com>
From:   Ian Kent <raven@themaw.net>
In-Reply-To: <cadcb382d47ef037c5b713b099ae46640dfea37d.camel@hammerspace.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 28/6/22 22:34, Trond Myklebust wrote:
> On Tue, 2022-06-28 at 08:25 +0800, Ian Kent wrote:
>> The valid values of nfs options port and mountport are 0 to
>> USHRT_MAX.
>>
>> The fs parser will return a fail for port values that are negative
>> and the sloppy option handling then returns success.
>>
>> But the sloppy option handling is meant to return success for invalid
>> options not valid options with invalid values.
>>
>> Parsing these values as s32 rather than u32 prevents the parser from
>> returning a parse fail allowing the later USHRT_MAX option check to
>> correctly return a fail in this case. The result check could be
>> changed
>> to use the int_32 union variant as well but leaving it as a uint_32
>> check avoids using two logical compares instead of one.
>>
>> Signed-off-by: Ian Kent <raven@themaw.net>
>> ---
>>   fs/nfs/fs_context.c |    4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
>> index 9a16897e8dc6..f4da1d2be616 100644
>> --- a/fs/nfs/fs_context.c
>> +++ b/fs/nfs/fs_context.c
>> @@ -156,14 +156,14 @@ static const struct fs_parameter_spec
>> nfs_fs_parameters[] = {
>>          fsparam_u32   ("minorversion",  Opt_minorversion),
>>          fsparam_string("mountaddr",     Opt_mountaddr),
>>          fsparam_string("mounthost",     Opt_mounthost),
>> -       fsparam_u32   ("mountport",     Opt_mountport),
>> +       fsparam_s32   ("mountport",     Opt_mountport),
>>          fsparam_string("mountproto",    Opt_mountproto),
>>          fsparam_u32   ("mountvers",     Opt_mountvers),
>>          fsparam_u32   ("namlen",        Opt_namelen),
>>          fsparam_u32   ("nconnect",      Opt_nconnect),
>>          fsparam_u32   ("max_connect",   Opt_max_connect),
>>          fsparam_string("nfsvers",       Opt_vers),
>> -       fsparam_u32   ("port",          Opt_port),
>> +       fsparam_s32   ("port",          Opt_port),
>>          fsparam_flag_no("posix",        Opt_posix),
>>          fsparam_string("proto",         Opt_proto),
>>          fsparam_flag_no("rdirplus",     Opt_rdirplus),
>>
>>
> Why don't we just check for the ENOPARAM return value from fs_parse()?

In this case I think the return will be EINVAL.

I think that's a bit to general for this case.

This seemed like the most sensible way to fix it.


Ian