[PATCH] psi: allow unprivileged users with CAP_SYS_RESOURCE to write psi files

[PATCH] psi: allow unprivileged users with CAP_SYS_RESOURCE to write psi files

[Josh Hunt]

Currently only root can write files under /proc/pressure. Relax this to
allow tasks running as unprivileged users with CAP_SYS_RESOURCE to be
able to write to these files.

Signed-off-by: Josh Hunt <johunt@akamai.com>
---
 kernel/sched/psi.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index b1b00e9bd7ed..98ff7baf1ba8 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -1270,6 +1270,9 @@ static ssize_t psi_write(struct file *file, const char __user *user_buf,
 	if (!nbytes)
 		return -EINVAL;
 
+	if (!capable(CAP_SYS_RESOURCE))
+		return -EPERM;
+
 	buf_size = min(nbytes, sizeof(buf));
 	if (copy_from_user(buf, user_buf, buf_size))
 		return -EFAULT;
@@ -1353,9 +1356,9 @@ static int __init psi_proc_init(void)
 {
 	if (psi_enable) {
 		proc_mkdir("pressure", NULL);
-		proc_create("pressure/io", 0, NULL, &psi_io_proc_ops);
-		proc_create("pressure/memory", 0, NULL, &psi_memory_proc_ops);
-		proc_create("pressure/cpu", 0, NULL, &psi_cpu_proc_ops);
+		proc_create("pressure/io", 0666, NULL, &psi_io_proc_ops);
+		proc_create("pressure/memory", 0666, NULL, &psi_memory_proc_ops);
+		proc_create("pressure/cpu", 0666, NULL, &psi_cpu_proc_ops);
 	}
 	return 0;
 }
-- 
2.17.1

Re: [PATCH] psi: allow unprivileged users with CAP_SYS_RESOURCE to write psi files

[Eric W. Biederman]

Josh Hunt <johunt@akamai.com> writes:

> Currently only root can write files under /proc/pressure. Relax this to
> allow tasks running as unprivileged users with CAP_SYS_RESOURCE to be
> able to write to these files.

The test for CAP_SYS_RESOURCE really needs to be in open rather
than in write.

Otherwise a suid root executable could have stdout redirected
into these files.

Eric


> Signed-off-by: Josh Hunt <johunt@akamai.com>
> ---
>  kernel/sched/psi.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
> index b1b00e9bd7ed..98ff7baf1ba8 100644
> --- a/kernel/sched/psi.c
> +++ b/kernel/sched/psi.c
> @@ -1270,6 +1270,9 @@ static ssize_t psi_write(struct file *file, const char __user *user_buf,
>  	if (!nbytes)
>  		return -EINVAL;
>  
> +	if (!capable(CAP_SYS_RESOURCE))
> +		return -EPERM;
> +
>  	buf_size = min(nbytes, sizeof(buf));
>  	if (copy_from_user(buf, user_buf, buf_size))
>  		return -EFAULT;
> @@ -1353,9 +1356,9 @@ static int __init psi_proc_init(void)
>  {
>  	if (psi_enable) {
>  		proc_mkdir("pressure", NULL);
> -		proc_create("pressure/io", 0, NULL, &psi_io_proc_ops);
> -		proc_create("pressure/memory", 0, NULL, &psi_memory_proc_ops);
> -		proc_create("pressure/cpu", 0, NULL, &psi_cpu_proc_ops);
> +		proc_create("pressure/io", 0666, NULL, &psi_io_proc_ops);
> +		proc_create("pressure/memory", 0666, NULL, &psi_memory_proc_ops);
> +		proc_create("pressure/cpu", 0666, NULL, &psi_cpu_proc_ops);
>  	}
>  	return 0;
>  }

Re: [PATCH] psi: allow unprivileged users with CAP_SYS_RESOURCE to write psi files

[Peter Zijlstra]

On Wed, Mar 31, 2021 at 11:31:56PM -0400, Josh Hunt wrote:
> Currently only root can write files under /proc/pressure. Relax this to
> allow tasks running as unprivileged users with CAP_SYS_RESOURCE to be
> able to write to these files.
> 
> Signed-off-by: Josh Hunt <johunt@akamai.com>

I suppose that's ok, but lets also ask Johannes.

> ---
>  kernel/sched/psi.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
> index b1b00e9bd7ed..98ff7baf1ba8 100644
> --- a/kernel/sched/psi.c
> +++ b/kernel/sched/psi.c
> @@ -1270,6 +1270,9 @@ static ssize_t psi_write(struct file *file, const char __user *user_buf,
>  	if (!nbytes)
>  		return -EINVAL;
>  
> +	if (!capable(CAP_SYS_RESOURCE))
> +		return -EPERM;
> +
>  	buf_size = min(nbytes, sizeof(buf));
>  	if (copy_from_user(buf, user_buf, buf_size))
>  		return -EFAULT;
> @@ -1353,9 +1356,9 @@ static int __init psi_proc_init(void)
>  {
>  	if (psi_enable) {
>  		proc_mkdir("pressure", NULL);
> -		proc_create("pressure/io", 0, NULL, &psi_io_proc_ops);
> -		proc_create("pressure/memory", 0, NULL, &psi_memory_proc_ops);
> -		proc_create("pressure/cpu", 0, NULL, &psi_cpu_proc_ops);
> +		proc_create("pressure/io", 0666, NULL, &psi_io_proc_ops);
> +		proc_create("pressure/memory", 0666, NULL, &psi_memory_proc_ops);
> +		proc_create("pressure/cpu", 0666, NULL, &psi_cpu_proc_ops);
>  	}
>  	return 0;
>  }
> -- 
> 2.17.1
> 

Re: [PATCH] psi: allow unprivileged users with CAP_SYS_RESOURCE to write psi files

[Kees Cook]

On Wed, Mar 31, 2021 at 11:36:28PM -0500, Eric W. Biederman wrote:
> Josh Hunt <johunt@akamai.com> writes:
> 
> > Currently only root can write files under /proc/pressure. Relax this to
> > allow tasks running as unprivileged users with CAP_SYS_RESOURCE to be
> > able to write to these files.
> 
> The test for CAP_SYS_RESOURCE really needs to be in open rather
> than in write.
> 
> Otherwise a suid root executable could have stdout redirected
> into these files.

Right. Or check against f_cred. (See uses of kallsyms_show_value())
https://www.kernel.org/doc/html/latest/security/credentials.html#open-file-credentials

-Kees

> 
> Eric
> 
> 
> > Signed-off-by: Josh Hunt <johunt@akamai.com>
> > ---
> >  kernel/sched/psi.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
> > index b1b00e9bd7ed..98ff7baf1ba8 100644
> > --- a/kernel/sched/psi.c
> > +++ b/kernel/sched/psi.c
> > @@ -1270,6 +1270,9 @@ static ssize_t psi_write(struct file *file, const char __user *user_buf,
> >  	if (!nbytes)
> >  		return -EINVAL;
> >  
> > +	if (!capable(CAP_SYS_RESOURCE))
> > +		return -EPERM;
> > +
> >  	buf_size = min(nbytes, sizeof(buf));
> >  	if (copy_from_user(buf, user_buf, buf_size))
> >  		return -EFAULT;
> > @@ -1353,9 +1356,9 @@ static int __init psi_proc_init(void)
> >  {
> >  	if (psi_enable) {
> >  		proc_mkdir("pressure", NULL);
> > -		proc_create("pressure/io", 0, NULL, &psi_io_proc_ops);
> > -		proc_create("pressure/memory", 0, NULL, &psi_memory_proc_ops);
> > -		proc_create("pressure/cpu", 0, NULL, &psi_cpu_proc_ops);
> > +		proc_create("pressure/io", 0666, NULL, &psi_io_proc_ops);
> > +		proc_create("pressure/memory", 0666, NULL, &psi_memory_proc_ops);
> > +		proc_create("pressure/cpu", 0666, NULL, &psi_cpu_proc_ops);
> >  	}
> >  	return 0;
> >  }

-- 
Kees Cook

Re: [PATCH] psi: allow unprivileged users with CAP_SYS_RESOURCE to write psi files

[Eric W. Biederman]

Kees Cook <keescook@chromium.org> writes:

> On Wed, Mar 31, 2021 at 11:36:28PM -0500, Eric W. Biederman wrote:
>> Josh Hunt <johunt@akamai.com> writes:
>> 
>> > Currently only root can write files under /proc/pressure. Relax this to
>> > allow tasks running as unprivileged users with CAP_SYS_RESOURCE to be
>> > able to write to these files.
>> 
>> The test for CAP_SYS_RESOURCE really needs to be in open rather
>> than in write.
>> 
>> Otherwise a suid root executable could have stdout redirected
>> into these files.
>
> Right. Or check against f_cred. (See uses of kallsyms_show_value())
> https://www.kernel.org/doc/html/latest/security/credentials.html#open-file-credentials

We really want to limit checking against f_cred to those cases where we
break userspace by checking in open.  AKA the cases where we made the
mistake of putting the permission check in the wrong place and now can't
fix it.

Since this change is change the permissions that open uses already I
don't see any reason we can't perform a proper check in open.

Eric

Re: [PATCH] psi: allow unprivileged users with CAP_SYS_RESOURCE to write psi files

[Josh Hunt]

On 4/1/21 10:47 AM, Eric W. Biederman wrote:
> Kees Cook <keescook@chromium.org> writes:
> 
>> On Wed, Mar 31, 2021 at 11:36:28PM -0500, Eric W. Biederman wrote:
>>> Josh Hunt <johunt@akamai.com> writes:
>>>
>>>> Currently only root can write files under /proc/pressure. Relax this to
>>>> allow tasks running as unprivileged users with CAP_SYS_RESOURCE to be
>>>> able to write to these files.
>>>
>>> The test for CAP_SYS_RESOURCE really needs to be in open rather
>>> than in write.
>>>
>>> Otherwise a suid root executable could have stdout redirected
>>> into these files.
>>
>> Right. Or check against f_cred. (See uses of kallsyms_show_value())
>> https://urldefense.com/v3/__https://www.kernel.org/doc/html/latest/security/credentials.html*open-file-credentials__;Iw!!GjvTz_vk!B_aeVyHMG20VNUGx001EFKpeYlahLQHye7oS8sokXuZOhVDTtF_deDl71a_KYA$
> 
> We really want to limit checking against f_cred to those cases where we
> break userspace by checking in open.  AKA the cases where we made the
> mistake of putting the permission check in the wrong place and now can't
> fix it.
> 
> Since this change is change the permissions that open uses already I
> don't see any reason we can't perform a proper check in open.
> 
> Eric
> 

Thank you for the feedback. I will spin a v2 doing the check in open.

Josh

Re: [PATCH] psi: allow unprivileged users with CAP_SYS_RESOURCE to write psi files

[Johannes Weiner]

On Thu, Apr 01, 2021 at 08:47:33AM +0200, Peter Zijlstra wrote:
> On Wed, Mar 31, 2021 at 11:31:56PM -0400, Josh Hunt wrote:
> > Currently only root can write files under /proc/pressure. Relax this to
> > allow tasks running as unprivileged users with CAP_SYS_RESOURCE to be
> > able to write to these files.
> > 
> > Signed-off-by: Josh Hunt <johunt@akamai.com>
> 
> I suppose that's ok, but lets also ask Johannes.

The write creates a kthread that runs as SCHED_FIFO. For userspace
threads this is reserved for CAP_SYS_NICE tasks, but it's a kernel
thread and not arbitrary code, so I suppose CAP_SYS_RESOURCE is fine.

Acked-by: Johannes Weiner <hannes@cmpxchg.org>