Re: [PATCH v6 2/5] fuse: Fail all requests with invalid uids or gids

[Eric W. Biederman]

Miklos Szeredi  writes:

> On Wed, Feb 21, 2018 at 9:29 PM, Eric W. Biederman
>  wrote:
>> Upon a cursory examinination the uid and gid of a fuse request are
>> necessary for correct operation.  Failing a fuse request where those
>> values are not reliable seems a straight forward and reliable means of
>> ensuring that fuse requests with bad data are not sent or processed.
>>
>> In most cases the vfs will avoid actions it suspects will cause
>> an inode write back of an inode with an invalid uid or gid.  But that does
>> not map precisely to what fuse is doing, so test for this and solve
>> this at the fuse level as well.
>>
>> Performing this work in fuse_req_init_context is cheap as the code is
>> already performing the translation here and only needs to check the
>> result of the translation to see if things are not representable in
>> a form the fuse server can handle.
>>
>> Signed-off-by: Eric W. Biederman 
>> ---
>>  fs/fuse/dev.c | 20 +---
>>  1 file changed, 13 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
>> index 0fb58f364fa6..216db3f51a31 100644
>> --- a/fs/fuse/dev.c
>> +++ b/fs/fuse/dev.c
>> @@ -112,11 +112,13 @@ static void __fuse_put_request(struct fuse_req *req)
>> refcount_dec(>count);
>>  }
>>
>> -static void fuse_req_init_context(struct fuse_conn *fc, struct fuse_req 
>> *req)
>> +static bool fuse_req_init_context(struct fuse_conn *fc, struct fuse_req 
>> *req)
>>  {
>> -   req->in.h.uid = from_kuid_munged(_user_ns, current_fsuid());
>> -   req->in.h.gid = from_kgid_munged(_user_ns, current_fsgid());
>> +   req->in.h.uid = from_kuid(_user_ns, current_fsuid());
>> +   req->in.h.gid = from_kgid(_user_ns, current_fsgid());
>> req->in.h.pid = pid_nr_ns(task_pid(current), fc->pid_ns);
>> +
>> +   return (req->in.h.uid != ((uid_t)-1)) && (req->in.h.gid != 
>> ((gid_t)-1));
>>  }
>>
>>  void fuse_set_initialized(struct fuse_conn *fc)
>> @@ -162,12 +164,13 @@ static struct fuse_req *__fuse_get_req(struct 
>> fuse_conn *fc, unsigned npages,
>> wake_up(>blocked_waitq);
>> goto out;
>> }
>> -
>> -   fuse_req_init_context(fc, req);
>> __set_bit(FR_WAITING, >flags);
>> if (for_background)
>> __set_bit(FR_BACKGROUND, >flags);
>> -
>> +   if (unlikely(!fuse_req_init_context(fc, req))) {
>> +   fuse_put_request(fc, req);
>> +   return ERR_PTR(-EOVERFLOW);
>> +   }
>> return req;
>>
>>   out:
>> @@ -256,9 +259,12 @@ struct fuse_req *fuse_get_req_nofail_nopages(struct 
>> fuse_conn *fc,
>> if (!req)
>> req = get_reserved_req(fc, file);
>>
>> -   fuse_req_init_context(fc, req);
>> __set_bit(FR_WAITING, >flags);
>> __clear_bit(FR_BACKGROUND, >flags);
>> +   if (unlikely(!fuse_req_init_context(fc, req))) {
>> +   fuse_put_request(fc, req);
>> +   return ERR_PTR(-EOVERFLOW);
>> +   }
>
> I think failing the "_nofail" variant is the wrong thing to do.  This
> is called to allocate a FLUSH request on close() and in readdirplus to
> allocate a FORGET request.  Failing the latter results in refcount
> leak in userspace.   Failing the former results in missing unlock on
> close() of posix locks.

Doh!  You are quite correct.

Modifying fuse_get_req_nofail_nopages to fail is a bug.

I am thinking the proper solution is to write:

static void fuse_req_init_context_nofail(struct fuse_req *req)
{
req->in.h.uid = 0;
req->in.h.gid = 0;
req->in.h.pid = 0;
}

And use that in the nofail case.  As it appears neither flush nor
the eviction of inodes is a user space triggered action and as such
user space identifiers are nonsense in those cases.

I will respin this patch shortly.

Eric

Re: [PATCH v6 2/5] fuse: Fail all requests with invalid uids or gids

[Eric W. Biederman]

Miklos Szeredi  writes:

> On Wed, Feb 21, 2018 at 9:29 PM, Eric W. Biederman
>  wrote:
>> Upon a cursory examinination the uid and gid of a fuse request are
>> necessary for correct operation.  Failing a fuse request where those
>> values are not reliable seems a straight forward and reliable means of
>> ensuring that fuse requests with bad data are not sent or processed.
>>
>> In most cases the vfs will avoid actions it suspects will cause
>> an inode write back of an inode with an invalid uid or gid.  But that does
>> not map precisely to what fuse is doing, so test for this and solve
>> this at the fuse level as well.
>>
>> Performing this work in fuse_req_init_context is cheap as the code is
>> already performing the translation here and only needs to check the
>> result of the translation to see if things are not representable in
>> a form the fuse server can handle.
>>
>> Signed-off-by: Eric W. Biederman 
>> ---
>>  fs/fuse/dev.c | 20 +---
>>  1 file changed, 13 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
>> index 0fb58f364fa6..216db3f51a31 100644
>> --- a/fs/fuse/dev.c
>> +++ b/fs/fuse/dev.c
>> @@ -112,11 +112,13 @@ static void __fuse_put_request(struct fuse_req *req)
>> refcount_dec(>count);
>>  }
>>
>> -static void fuse_req_init_context(struct fuse_conn *fc, struct fuse_req 
>> *req)
>> +static bool fuse_req_init_context(struct fuse_conn *fc, struct fuse_req 
>> *req)
>>  {
>> -   req->in.h.uid = from_kuid_munged(_user_ns, current_fsuid());
>> -   req->in.h.gid = from_kgid_munged(_user_ns, current_fsgid());
>> +   req->in.h.uid = from_kuid(_user_ns, current_fsuid());
>> +   req->in.h.gid = from_kgid(_user_ns, current_fsgid());
>> req->in.h.pid = pid_nr_ns(task_pid(current), fc->pid_ns);
>> +
>> +   return (req->in.h.uid != ((uid_t)-1)) && (req->in.h.gid != 
>> ((gid_t)-1));
>>  }
>>
>>  void fuse_set_initialized(struct fuse_conn *fc)
>> @@ -162,12 +164,13 @@ static struct fuse_req *__fuse_get_req(struct 
>> fuse_conn *fc, unsigned npages,
>> wake_up(>blocked_waitq);
>> goto out;
>> }
>> -
>> -   fuse_req_init_context(fc, req);
>> __set_bit(FR_WAITING, >flags);
>> if (for_background)
>> __set_bit(FR_BACKGROUND, >flags);
>> -
>> +   if (unlikely(!fuse_req_init_context(fc, req))) {
>> +   fuse_put_request(fc, req);
>> +   return ERR_PTR(-EOVERFLOW);
>> +   }
>> return req;
>>
>>   out:
>> @@ -256,9 +259,12 @@ struct fuse_req *fuse_get_req_nofail_nopages(struct 
>> fuse_conn *fc,
>> if (!req)
>> req = get_reserved_req(fc, file);
>>
>> -   fuse_req_init_context(fc, req);
>> __set_bit(FR_WAITING, >flags);
>> __clear_bit(FR_BACKGROUND, >flags);
>> +   if (unlikely(!fuse_req_init_context(fc, req))) {
>> +   fuse_put_request(fc, req);
>> +   return ERR_PTR(-EOVERFLOW);
>> +   }
>
> I think failing the "_nofail" variant is the wrong thing to do.  This
> is called to allocate a FLUSH request on close() and in readdirplus to
> allocate a FORGET request.  Failing the latter results in refcount
> leak in userspace.   Failing the former results in missing unlock on
> close() of posix locks.

Doh!  You are quite correct.

Modifying fuse_get_req_nofail_nopages to fail is a bug.

I am thinking the proper solution is to write:

static void fuse_req_init_context_nofail(struct fuse_req *req)
{
req->in.h.uid = 0;
req->in.h.gid = 0;
req->in.h.pid = 0;
}

And use that in the nofail case.  As it appears neither flush nor
the eviction of inodes is a user space triggered action and as such
user space identifiers are nonsense in those cases.

I will respin this patch shortly.

Eric

Re: [PATCH v6 2/5] fuse: Fail all requests with invalid uids or gids

[Miklos Szeredi]

On Wed, Feb 21, 2018 at 9:29 PM, Eric W. Biederman
 wrote:
> Upon a cursory examinination the uid and gid of a fuse request are
> necessary for correct operation.  Failing a fuse request where those
> values are not reliable seems a straight forward and reliable means of
> ensuring that fuse requests with bad data are not sent or processed.
>
> In most cases the vfs will avoid actions it suspects will cause
> an inode write back of an inode with an invalid uid or gid.  But that does
> not map precisely to what fuse is doing, so test for this and solve
> this at the fuse level as well.
>
> Performing this work in fuse_req_init_context is cheap as the code is
> already performing the translation here and only needs to check the
> result of the translation to see if things are not representable in
> a form the fuse server can handle.
>
> Signed-off-by: Eric W. Biederman 
> ---
>  fs/fuse/dev.c | 20 +---
>  1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index 0fb58f364fa6..216db3f51a31 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -112,11 +112,13 @@ static void __fuse_put_request(struct fuse_req *req)
> refcount_dec(>count);
>  }
>
> -static void fuse_req_init_context(struct fuse_conn *fc, struct fuse_req *req)
> +static bool fuse_req_init_context(struct fuse_conn *fc, struct fuse_req *req)
>  {
> -   req->in.h.uid = from_kuid_munged(_user_ns, current_fsuid());
> -   req->in.h.gid = from_kgid_munged(_user_ns, current_fsgid());
> +   req->in.h.uid = from_kuid(_user_ns, current_fsuid());
> +   req->in.h.gid = from_kgid(_user_ns, current_fsgid());
> req->in.h.pid = pid_nr_ns(task_pid(current), fc->pid_ns);
> +
> +   return (req->in.h.uid != ((uid_t)-1)) && (req->in.h.gid != 
> ((gid_t)-1));
>  }
>
>  void fuse_set_initialized(struct fuse_conn *fc)
> @@ -162,12 +164,13 @@ static struct fuse_req *__fuse_get_req(struct fuse_conn 
> *fc, unsigned npages,
> wake_up(>blocked_waitq);
> goto out;
> }
> -
> -   fuse_req_init_context(fc, req);
> __set_bit(FR_WAITING, >flags);
> if (for_background)
> __set_bit(FR_BACKGROUND, >flags);
> -
> +   if (unlikely(!fuse_req_init_context(fc, req))) {
> +   fuse_put_request(fc, req);
> +   return ERR_PTR(-EOVERFLOW);
> +   }
> return req;
>
>   out:
> @@ -256,9 +259,12 @@ struct fuse_req *fuse_get_req_nofail_nopages(struct 
> fuse_conn *fc,
> if (!req)
> req = get_reserved_req(fc, file);
>
> -   fuse_req_init_context(fc, req);
> __set_bit(FR_WAITING, >flags);
> __clear_bit(FR_BACKGROUND, >flags);
> +   if (unlikely(!fuse_req_init_context(fc, req))) {
> +   fuse_put_request(fc, req);
> +   return ERR_PTR(-EOVERFLOW);
> +   }

I think failing the "_nofail" variant is the wrong thing to do.  This
is called to allocate a FLUSH request on close() and in readdirplus to
allocate a FORGET request.  Failing the latter results in refcount
leak in userspace.   Failing the former results in missing unlock on
close() of posix locks.

Thanks,
Miklos

Re: [PATCH v6 2/5] fuse: Fail all requests with invalid uids or gids

[Miklos Szeredi]

On Wed, Feb 21, 2018 at 9:29 PM, Eric W. Biederman
 wrote:
> Upon a cursory examinination the uid and gid of a fuse request are
> necessary for correct operation.  Failing a fuse request where those
> values are not reliable seems a straight forward and reliable means of
> ensuring that fuse requests with bad data are not sent or processed.
>
> In most cases the vfs will avoid actions it suspects will cause
> an inode write back of an inode with an invalid uid or gid.  But that does
> not map precisely to what fuse is doing, so test for this and solve
> this at the fuse level as well.
>
> Performing this work in fuse_req_init_context is cheap as the code is
> already performing the translation here and only needs to check the
> result of the translation to see if things are not representable in
> a form the fuse server can handle.
>
> Signed-off-by: Eric W. Biederman 
> ---
>  fs/fuse/dev.c | 20 +---
>  1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index 0fb58f364fa6..216db3f51a31 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -112,11 +112,13 @@ static void __fuse_put_request(struct fuse_req *req)
> refcount_dec(>count);
>  }
>
> -static void fuse_req_init_context(struct fuse_conn *fc, struct fuse_req *req)
> +static bool fuse_req_init_context(struct fuse_conn *fc, struct fuse_req *req)
>  {
> -   req->in.h.uid = from_kuid_munged(_user_ns, current_fsuid());
> -   req->in.h.gid = from_kgid_munged(_user_ns, current_fsgid());
> +   req->in.h.uid = from_kuid(_user_ns, current_fsuid());
> +   req->in.h.gid = from_kgid(_user_ns, current_fsgid());
> req->in.h.pid = pid_nr_ns(task_pid(current), fc->pid_ns);
> +
> +   return (req->in.h.uid != ((uid_t)-1)) && (req->in.h.gid != 
> ((gid_t)-1));
>  }
>
>  void fuse_set_initialized(struct fuse_conn *fc)
> @@ -162,12 +164,13 @@ static struct fuse_req *__fuse_get_req(struct fuse_conn 
> *fc, unsigned npages,
> wake_up(>blocked_waitq);
> goto out;
> }
> -
> -   fuse_req_init_context(fc, req);
> __set_bit(FR_WAITING, >flags);
> if (for_background)
> __set_bit(FR_BACKGROUND, >flags);
> -
> +   if (unlikely(!fuse_req_init_context(fc, req))) {
> +   fuse_put_request(fc, req);
> +   return ERR_PTR(-EOVERFLOW);
> +   }
> return req;
>
>   out:
> @@ -256,9 +259,12 @@ struct fuse_req *fuse_get_req_nofail_nopages(struct 
> fuse_conn *fc,
> if (!req)
> req = get_reserved_req(fc, file);
>
> -   fuse_req_init_context(fc, req);
> __set_bit(FR_WAITING, >flags);
> __clear_bit(FR_BACKGROUND, >flags);
> +   if (unlikely(!fuse_req_init_context(fc, req))) {
> +   fuse_put_request(fc, req);
> +   return ERR_PTR(-EOVERFLOW);
> +   }

I think failing the "_nofail" variant is the wrong thing to do.  This
is called to allocate a FLUSH request on close() and in readdirplus to
allocate a FORGET request.  Failing the latter results in refcount
leak in userspace.   Failing the former results in missing unlock on
close() of posix locks.

Thanks,
Miklos

[PATCH v6 2/5] fuse: Fail all requests with invalid uids or gids

[Eric W. Biederman]

Upon a cursory examinination the uid and gid of a fuse request are
necessary for correct operation.  Failing a fuse request where those
values are not reliable seems a straight forward and reliable means of
ensuring that fuse requests with bad data are not sent or processed.

In most cases the vfs will avoid actions it suspects will cause
an inode write back of an inode with an invalid uid or gid.  But that does
not map precisely to what fuse is doing, so test for this and solve
this at the fuse level as well.

Performing this work in fuse_req_init_context is cheap as the code is
already performing the translation here and only needs to check the
result of the translation to see if things are not representable in
a form the fuse server can handle.

Signed-off-by: Eric W. Biederman 
---
 fs/fuse/dev.c | 20 +---
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 0fb58f364fa6..216db3f51a31 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -112,11 +112,13 @@ static void __fuse_put_request(struct fuse_req *req)
refcount_dec(>count);
 }
 
-static void fuse_req_init_context(struct fuse_conn *fc, struct fuse_req *req)
+static bool fuse_req_init_context(struct fuse_conn *fc, struct fuse_req *req)
 {
-   req->in.h.uid = from_kuid_munged(_user_ns, current_fsuid());
-   req->in.h.gid = from_kgid_munged(_user_ns, current_fsgid());
+   req->in.h.uid = from_kuid(_user_ns, current_fsuid());
+   req->in.h.gid = from_kgid(_user_ns, current_fsgid());
req->in.h.pid = pid_nr_ns(task_pid(current), fc->pid_ns);
+
+   return (req->in.h.uid != ((uid_t)-1)) && (req->in.h.gid != ((gid_t)-1));
 }
 
 void fuse_set_initialized(struct fuse_conn *fc)
@@ -162,12 +164,13 @@ static struct fuse_req *__fuse_get_req(struct fuse_conn 
*fc, unsigned npages,
wake_up(>blocked_waitq);
goto out;
}
-
-   fuse_req_init_context(fc, req);
__set_bit(FR_WAITING, >flags);
if (for_background)
__set_bit(FR_BACKGROUND, >flags);
-
+   if (unlikely(!fuse_req_init_context(fc, req))) {
+   fuse_put_request(fc, req);
+   return ERR_PTR(-EOVERFLOW);
+   }
return req;
 
  out:
@@ -256,9 +259,12 @@ struct fuse_req *fuse_get_req_nofail_nopages(struct 
fuse_conn *fc,
if (!req)
req = get_reserved_req(fc, file);
 
-   fuse_req_init_context(fc, req);
__set_bit(FR_WAITING, >flags);
__clear_bit(FR_BACKGROUND, >flags);
+   if (unlikely(!fuse_req_init_context(fc, req))) {
+   fuse_put_request(fc, req);
+   return ERR_PTR(-EOVERFLOW);
+   }
return req;
 }
 
-- 
2.14.1

[PATCH v6 2/5] fuse: Fail all requests with invalid uids or gids

[Eric W. Biederman]

Upon a cursory examinination the uid and gid of a fuse request are
necessary for correct operation.  Failing a fuse request where those
values are not reliable seems a straight forward and reliable means of
ensuring that fuse requests with bad data are not sent or processed.

In most cases the vfs will avoid actions it suspects will cause
an inode write back of an inode with an invalid uid or gid.  But that does
not map precisely to what fuse is doing, so test for this and solve
this at the fuse level as well.

Performing this work in fuse_req_init_context is cheap as the code is
already performing the translation here and only needs to check the
result of the translation to see if things are not representable in
a form the fuse server can handle.

Signed-off-by: Eric W. Biederman 
---
 fs/fuse/dev.c | 20 +---
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 0fb58f364fa6..216db3f51a31 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -112,11 +112,13 @@ static void __fuse_put_request(struct fuse_req *req)
refcount_dec(>count);
 }
 
-static void fuse_req_init_context(struct fuse_conn *fc, struct fuse_req *req)
+static bool fuse_req_init_context(struct fuse_conn *fc, struct fuse_req *req)
 {
-   req->in.h.uid = from_kuid_munged(_user_ns, current_fsuid());
-   req->in.h.gid = from_kgid_munged(_user_ns, current_fsgid());
+   req->in.h.uid = from_kuid(_user_ns, current_fsuid());
+   req->in.h.gid = from_kgid(_user_ns, current_fsgid());
req->in.h.pid = pid_nr_ns(task_pid(current), fc->pid_ns);
+
+   return (req->in.h.uid != ((uid_t)-1)) && (req->in.h.gid != ((gid_t)-1));
 }
 
 void fuse_set_initialized(struct fuse_conn *fc)
@@ -162,12 +164,13 @@ static struct fuse_req *__fuse_get_req(struct fuse_conn 
*fc, unsigned npages,
wake_up(>blocked_waitq);
goto out;
}
-
-   fuse_req_init_context(fc, req);
__set_bit(FR_WAITING, >flags);
if (for_background)
__set_bit(FR_BACKGROUND, >flags);
-
+   if (unlikely(!fuse_req_init_context(fc, req))) {
+   fuse_put_request(fc, req);
+   return ERR_PTR(-EOVERFLOW);
+   }
return req;
 
  out:
@@ -256,9 +259,12 @@ struct fuse_req *fuse_get_req_nofail_nopages(struct 
fuse_conn *fc,
if (!req)
req = get_reserved_req(fc, file);
 
-   fuse_req_init_context(fc, req);
__set_bit(FR_WAITING, >flags);
__clear_bit(FR_BACKGROUND, >flags);
+   if (unlikely(!fuse_req_init_context(fc, req))) {
+   fuse_put_request(fc, req);
+   return ERR_PTR(-EOVERFLOW);
+   }
return req;
 }
 
-- 
2.14.1