* [PATCH v2] lockd: detect and reject lock arguments that overflow
@ 2022-08-01 19:57 Jeff Layton
2022-08-01 21:04 ` Jan Kasiak
2022-08-02 14:10 ` Chuck Lever III
0 siblings, 2 replies; 6+ messages in thread
From: Jeff Layton @ 2022-08-01 19:57 UTC (permalink / raw
To: chuck.lever; +Cc: bfields, linux-nfs, Jan Kasiak
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] lockd: detect and reject lock arguments that overflow
2022-08-01 19:57 [PATCH v2] lockd: detect and reject lock arguments that overflow Jeff Layton
@ 2022-08-01 21:04 ` Jan Kasiak
2022-08-02 14:10 ` Chuck Lever III
1 sibling, 0 replies; 6+ messages in thread
From: Jan Kasiak @ 2022-08-01 21:04 UTC (permalink / raw
To: Jeff Layton; +Cc: Chuck Lever III, bfields, Linux NFS Mailing List
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] lockd: detect and reject lock arguments that overflow
2022-08-01 19:57 [PATCH v2] lockd: detect and reject lock arguments that overflow Jeff Layton
2022-08-01 21:04 ` Jan Kasiak
@ 2022-08-02 14:10 ` Chuck Lever III
2022-08-02 15:47 ` Jeff Layton
1 sibling, 1 reply; 6+ messages in thread
From: Chuck Lever III @ 2022-08-02 14:10 UTC (permalink / raw
To: Jeff Layton; +Cc: Bruce Fields, Linux NFS Mailing List, Jan Kasiak
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] lockd: detect and reject lock arguments that overflow
2022-08-02 14:10 ` Chuck Lever III
@ 2022-08-02 15:47 ` Jeff Layton
2022-08-02 15:52 ` Chuck Lever III
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Layton @ 2022-08-02 15:47 UTC (permalink / raw
To: Chuck Lever III; +Cc: Bruce Fields, Linux NFS Mailing List, Jan Kasiak
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
>
>
>
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] lockd: detect and reject lock arguments that overflow
2022-08-02 15:47 ` Jeff Layton
@ 2022-08-02 15:52 ` Chuck Lever III
2022-08-02 15:54 ` Jeff Layton
0 siblings, 1 reply; 6+ messages in thread
From: Chuck Lever III @ 2022-08-02 15:52 UTC (permalink / raw
To: Jeff Layton; +Cc: Bruce Fields, Linux NFS Mailing List, Jan Kasiak
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] lockd: detect and reject lock arguments that overflow
2022-08-02 15:52 ` Chuck Lever III
@ 2022-08-02 15:54 ` Jeff Layton
0 siblings, 0 replies; 6+ messages in thread
From: Jeff Layton @ 2022-08-02 15:54 UTC (permalink / raw
To: Chuck Lever III; +Cc: Bruce Fields, Linux NFS Mailing List, Jan Kasiak
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
>
>
>
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-08-02 15:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-01 19:57 [PATCH v2] lockd: detect and reject lock arguments that overflow Jeff Layton
2022-08-01 21:04 ` Jan Kasiak
2022-08-02 14:10 ` Chuck Lever III
2022-08-02 15:47 ` Jeff Layton
2022-08-02 15:52 ` Chuck Lever III
2022-08-02 15:54 ` Jeff Layton
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.