> On Aug 1, 2022, at 3:57 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > lockd doesn't currently vet the start and length in nlm4 requests like > it should, and can end up generating lock requests with arguments that > overflow when passed to the filesystem. > > The NLM4 protocol uses unsigned 64-bit arguments for both start and > length, whereas struct file_lock tracks the start and end as loff_t > values. By the time we get around to calling nlm4svc_retrieve_args, > we've lost the information that would allow us to determine if there was > an overflow. > > Start tracking the actual start and len for NLM4 requests in the > nlm_lock. In nlm4svc_retrieve_args, vet these values to ensure they > won't cause an overflow, and return NLM4_FBIG if they do. > > Link: https://bugzilla.linux-nfs.org/show_bug.cgi?id=392 > Reported-by: Jan Kasiak <j.kasiak@xxxxxxxxx> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx> I've applied this one to my private tree for testing. Thanks Jeff! > --- > fs/lockd/svc4proc.c | 8 ++++++++ > fs/lockd/xdr4.c | 19 ++----------------- > include/linux/lockd/xdr.h | 2 ++ > 3 files changed, 12 insertions(+), 17 deletions(-) > > v2: record args as u64s in nlm_lock and check them in > nlm4svc_retrieve_args > > diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c > index 176b468a61c7..e5adb524a445 100644 > --- a/fs/lockd/svc4proc.c > +++ b/fs/lockd/svc4proc.c > @@ -32,6 +32,10 @@ nlm4svc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp, > if (!nlmsvc_ops) > return nlm_lck_denied_nolocks; > > + if (lock->lock_start > OFFSET_MAX || > + (lock->lock_len && ((lock->lock_len - 1) > (OFFSET_MAX - lock->lock_start)))) > + return nlm4_fbig; > + > /* Obtain host handle */ > if (!(host = nlmsvc_lookup_host(rqstp, lock->caller, lock->len)) > || (argp->monitor && nsm_monitor(host) < 0)) > @@ -50,6 +54,10 @@ nlm4svc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp, > /* Set up the missing parts of the file_lock structure */ > lock->fl.fl_file = file->f_file[mode]; > lock->fl.fl_pid = current->tgid; > + lock->fl.fl_start = (loff_t)lock->lock_start; > + lock->fl.fl_end = lock->lock_len ? > + (loff_t)(lock->lock_start + lock->lock_len - 1) : > + OFFSET_MAX; > lock->fl.fl_lmops = &nlmsvc_lock_operations; > nlmsvc_locks_init_private(&lock->fl, host, (pid_t)lock->svid); > if (!lock->fl.fl_owner) { > diff --git a/fs/lockd/xdr4.c b/fs/lockd/xdr4.c > index 856267c0864b..712fdfeb8ef0 100644 > --- a/fs/lockd/xdr4.c > +++ b/fs/lockd/xdr4.c > @@ -20,13 +20,6 @@ > > #include "svcxdr.h" > > -static inline loff_t > -s64_to_loff_t(__s64 offset) > -{ > - return (loff_t)offset; > -} > - > - > static inline s64 > loff_t_to_s64(loff_t offset) > { > @@ -70,8 +63,6 @@ static bool > svcxdr_decode_lock(struct xdr_stream *xdr, struct nlm_lock *lock) > { > struct file_lock *fl = &lock->fl; > - u64 len, start; > - s64 end; > > if (!svcxdr_decode_string(xdr, &lock->caller, &lock->len)) > return false; > @@ -81,20 +72,14 @@ svcxdr_decode_lock(struct xdr_stream *xdr, struct nlm_lock *lock) > return false; > if (xdr_stream_decode_u32(xdr, &lock->svid) < 0) > return false; > - if (xdr_stream_decode_u64(xdr, &start) < 0) > + if (xdr_stream_decode_u64(xdr, &lock->lock_start) < 0) > return false; > - if (xdr_stream_decode_u64(xdr, &len) < 0) > + if (xdr_stream_decode_u64(xdr, &lock->lock_len) < 0) > return false; > > locks_init_lock(fl); > fl->fl_flags = FL_POSIX; > fl->fl_type = F_RDLCK; > - end = start + len - 1; > - fl->fl_start = s64_to_loff_t(start); > - if (len == 0 || end < 0) > - fl->fl_end = OFFSET_MAX; > - else > - fl->fl_end = s64_to_loff_t(end); > > return true; > } > diff --git a/include/linux/lockd/xdr.h b/include/linux/lockd/xdr.h > index 398f70093cd3..67e4a2c5500b 100644 > --- a/include/linux/lockd/xdr.h > +++ b/include/linux/lockd/xdr.h > @@ -41,6 +41,8 @@ struct nlm_lock { > struct nfs_fh fh; > struct xdr_netobj oh; > u32 svid; > + u64 lock_start; > + u64 lock_len; > struct file_lock fl; > }; > > -- > 2.37.1 > -- Chuck Lever
