| Message ID | 20220801195726.154229-1-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] lockd: detect and reject lock arguments that overflow | expand |
Hi Jeff, Can't speak to everything, but applying this patch on my system fixes the issues I was having. Thanks for the quick patch! Jan On Mon, Aug 1, 2022 at 3:57 PM Jeff Layton <jlayton@kernel.org> 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@gmail.com> > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > 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 >
> On Aug 1, 2022, at 3:57 PM, Jeff Layton <jlayton@kernel.org> 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@gmail.com> > Signed-off-by: Jeff Layton <jlayton@kernel.org> 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
On Tue, 2022-08-02 at 14:10 +0000, Chuck Lever III wrote: > > > On Aug 1, 2022, at 3:57 PM, Jeff Layton <jlayton@kernel.org> 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@gmail.com> > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > 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. > > --- > > 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 > > >
> On Aug 2, 2022, at 11:47 AM, Jeff Layton <jlayton@kernel.org> 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@kernel.org> 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@gmail.com> >>> Signed-off-by: Jeff Layton <jlayton@kernel.org> >> >> 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@vger.kernel.org> # 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@kernel.org> -- Chuck Lever
On Tue, 2022-08-02 at 15:52 +0000, Chuck Lever III wrote: > > > On Aug 2, 2022, at 11:47 AM, Jeff Layton <jlayton@kernel.org> 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@kernel.org> 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@gmail.com> > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > > > > 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@vger.kernel.org> # 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@kernel.org> > > -- > Chuck Lever > > >
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; };
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@gmail.com> Signed-off-by: Jeff Layton <jlayton@kernel.org> --- 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