Re: [PATCH] nfsd: return -EINVAL when namelen is 0

[Chuck Lever]

On Mon, Sep 09, 2024 at 11:33:04AM +0000, David Laight wrote:
> From: Scott Mayhew
> > Sent: 09 September 2024 12:24
> ...
> > > > > diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> > > > > index 67d8673a9391..69a3a84e159e 100644
> > > > > --- a/fs/nfsd/nfs4recover.c
> > > > > +++ b/fs/nfsd/nfs4recover.c
> > > > > @@ -809,6 +809,10 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
> > > > >  			ci = &cmsg->cm_u.cm_clntinfo;
> > > > >  			if (get_user(namelen, &ci->cc_name.cn_len))
> > > > >  				return -EFAULT;
> > > > > +			if (!namelen) {
> > > > > +				dprintk("%s: namelen should not be zero", __func__);
> > > > > +				return -EINVAL;
> > > > > +			}
> > > > >  			name.data = memdup_user(&ci->cc_name.cn_id, namelen);
> > >
> > > Don't you also want an upper bound sanity check?
> > > (or is cn_len only 8 bit?)
> > 
> > Yeah, actually it should probably be checking for namelen >
> > NFS4_OPAQUE_LIMIT.

Scott, can you send me a tested patch? I can squash that in.


> I suspect memdup_user() itself should have a third 'maxlen' argument.
> And probably one that is required to be a compile-time constant.
> 
> Oh, and is dprintk() rate-limited?

No dprintk call site is rate-limited, and they are everywhere in
this code. Since they are off by default, rate-limiting is not a
concern. Also, journald will rate-limit any output from the kernel
to prevent flooding the system lock.


> Not that the message looks very helpful.

The specific message is not helpful; suggestions welcome. But I
prefer having a warning of some kind rather than a silent failure
mode.


-- 
Chuck Lever