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 <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
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
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 <[email protected]> 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 <[email protected]>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]>
> Signed-off-by: Jeff Layton <[email protected]>
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 <[email protected]> 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 <[email protected]>
> > Signed-off-by: Jeff Layton <[email protected]>
>
> 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 <[email protected]>
> On Aug 2, 2022, at 11:47 AM, Jeff Layton <[email protected]> wrote:
>
> On Tue, 2022-08-02 at 14:10 +0000, Chuck Lever III wrote:
>>
>>> On Aug 1, 2022, at 3:57 PM, Jeff Layton <[email protected]> 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 <[email protected]>
>>> Signed-off-by: Jeff Layton <[email protected]>
>>
>> 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: <[email protected]> # 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 <[email protected]>
--
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 <[email protected]> wrote:
> >
> > On Tue, 2022-08-02 at 14:10 +0000, Chuck Lever III wrote:
> > >
> > > > On Aug 1, 2022, at 3:57 PM, Jeff Layton <[email protected]> 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 <[email protected]>
> > > > Signed-off-by: Jeff Layton <[email protected]>
> > >
> > > 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: <[email protected]> # 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 <[email protected]>
>
> --
> Chuck Lever
>
>
>
--
Jeff Layton <[email protected]>