> 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 ? >>> --- >>> 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
