Re: [REPOST PATCH] nfs: fix port value parsing

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 2022-06-30 at 07:33 +0800, Ian Kent wrote:
> 
> On 29/6/22 23:33, Trond Myklebust wrote:
> > On Wed, 2022-06-29 at 09:02 +0800, Ian Kent wrote:
> > > 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@xxxxxxxxxx>
> > > > > ---
> > > > >    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.
> > My point is that 'sloppy' is only supposed to work to suppress the
> > error in the case where an option is not found by the parser. That
> > corresponds to the error ENOPARAM.
> 
> Well, yes, and that's why ENOPARAM isn't returned and shouldn't be.
> 
> And if the sloppy option is given it doesn't get to check the value
> 
> of the option, it just returns success which isn't right.
> 
> 
> > 
> > > I think that's a bit to general for this case.
> > > 
> > > This seemed like the most sensible way to fix it.
> > > 
> > Your patch works around just one symptom of the problem instead of
> > addressing the root cause.
> > 
> Ok, how do you recommend I fix this?
> 

Maybe I'm missing something, but why not this?

8<--------------------------------
diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
index 9a16897e8dc6..8f1f9b4af89d 100644
--- a/fs/nfs/fs_context.c
+++ b/fs/nfs/fs_context.c
@@ -484,7 +484,7 @@ static int nfs_fs_context_parse_param(struct
fs_context *fc,
 
 	opt = fs_parse(fc, nfs_fs_parameters, param, &result);
 	if (opt < 0)
-		return ctx->sloppy ? 1 : opt;
+		return (opt == -ENOPARAM && ctx->sloppy) ? 1 : opt;
 
 	if (fc->security)
 		ctx->has_sec_mnt_opts = 1;

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx






[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux