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