On Tue, 2022-08-02 at 15:52 +0000, Chuck Lever III wrote: > > > On Aug 2, 2022, at 11:47 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > > > On Tue, 2022-08-02 at 14:10 +0000, Chuck Lever III wrote: > > > > > > > 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! > > > > > > > > > > Thank you. We should probably also consider sending this to stable > > kernels too. It's at least a DoS vector. > > Agreed, though it won't apply before 345b4159a075 ("lockd: Update > the NLMv4 TEST arguments decoder to use struct xdr_stream") > > How about: > > Cc: <stable@xxxxxxxxxxxxxxx> # 5.14 > > Works for me. We could probably spin up a version for earlier kernels too if necessary. > > > > --- > > > > 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 > > > > > > > > > > > > > -- > > Jeff Layton <jlayton@xxxxxxxxxx> > > -- > Chuck Lever > > > -- Jeff Layton <jlayton@xxxxxxxxxx>
