* Re: [PATCH] psi: allow unprivileged users with CAP_SYS_RESOURCE to write psi files
@ 2021-04-01 13:16 kernel test robot
0 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2021-04-01 13:16 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 2413 bytes --]
CC: kbuild-all(a)lists.01.org
In-Reply-To: <20210401033156.7262-1-johunt@akamai.com>
References: <20210401033156.7262-1-johunt@akamai.com>
TO: Josh Hunt <johunt@akamai.com>
TO: Ingo Molnar <mingo@redhat.com>
TO: Peter Zijlstra <peterz@infradead.org>
TO: linux-kernel(a)vger.kernel.org
CC: Josh Hunt <johunt@akamai.com>
Hi Josh,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tip/sched/core]
[also build test WARNING on v5.12-rc5 next-20210401]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Josh-Hunt/psi-allow-unprivileged-users-with-CAP_SYS_RESOURCE-to-write-psi-files/20210401-122010
base: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 0a2b65c03e9b47493e1442bf9c84badc60d9bffb
:::::: branch date: 9 hours ago
:::::: commit date: 9 hours ago
config: um-randconfig-m031-20210401 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
kernel/sched/psi.c:1359 psi_proc_init() warn: proc file '"pressure/io"' is world writable
kernel/sched/psi.c:1360 psi_proc_init() warn: proc file '"pressure/memory"' is world writable
kernel/sched/psi.c:1361 psi_proc_init() warn: proc file '"pressure/cpu"' is world writable
vim +1359 kernel/sched/psi.c
eb414681d5a07d Johannes Weiner 2018-10-26 1354
eb414681d5a07d Johannes Weiner 2018-10-26 1355 static int __init psi_proc_init(void)
eb414681d5a07d Johannes Weiner 2018-10-26 1356 {
3d817689a62cf7 Wang Long 2019-12-18 1357 if (psi_enable) {
eb414681d5a07d Johannes Weiner 2018-10-26 1358 proc_mkdir("pressure", NULL);
ee6d71e2ff4d9f Josh Hunt 2021-03-31 @1359 proc_create("pressure/io", 0666, NULL, &psi_io_proc_ops);
ee6d71e2ff4d9f Josh Hunt 2021-03-31 @1360 proc_create("pressure/memory", 0666, NULL, &psi_memory_proc_ops);
ee6d71e2ff4d9f Josh Hunt 2021-03-31 @1361 proc_create("pressure/cpu", 0666, NULL, &psi_cpu_proc_ops);
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 10289 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH] psi: allow unprivileged users with CAP_SYS_RESOURCE to write psi files
@ 2021-04-01 3:31 Josh Hunt
2021-04-01 4:36 ` Eric W. Biederman
2021-04-01 6:47 ` Peter Zijlstra
0 siblings, 2 replies; 8+ messages in thread
From: Josh Hunt @ 2021-04-01 3:31 UTC (permalink / raw)
To: Ingo Molnar, Peter Zijlstra, linux-kernel; +Cc: 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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] psi: allow unprivileged users with CAP_SYS_RESOURCE to write psi files
2021-04-01 3:31 Josh Hunt
@ 2021-04-01 4:36 ` Eric W. Biederman
2021-04-01 7:50 ` Kees Cook
2021-04-01 6:47 ` Peter Zijlstra
1 sibling, 1 reply; 8+ messages in thread
From: Eric W. Biederman @ 2021-04-01 4:36 UTC (permalink / raw)
To: Josh Hunt; +Cc: Ingo Molnar, Peter Zijlstra, linux-kernel
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;
> }
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] psi: allow unprivileged users with CAP_SYS_RESOURCE to write psi files
2021-04-01 4:36 ` Eric W. Biederman
@ 2021-04-01 7:50 ` Kees Cook
2021-04-01 17:47 ` Eric W. Biederman
0 siblings, 1 reply; 8+ messages in thread
From: Kees Cook @ 2021-04-01 7:50 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: Josh Hunt, Ingo Molnar, Peter Zijlstra, linux-kernel
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] psi: allow unprivileged users with CAP_SYS_RESOURCE to write psi files
2021-04-01 7:50 ` Kees Cook
@ 2021-04-01 17:47 ` Eric W. Biederman
2021-04-01 18:10 ` Josh Hunt
0 siblings, 1 reply; 8+ messages in thread
From: Eric W. Biederman @ 2021-04-01 17:47 UTC (permalink / raw)
To: Kees Cook; +Cc: Josh Hunt, Ingo Molnar, Peter Zijlstra, linux-kernel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] psi: allow unprivileged users with CAP_SYS_RESOURCE to write psi files
2021-04-01 17:47 ` Eric W. Biederman
@ 2021-04-01 18:10 ` Josh Hunt
0 siblings, 0 replies; 8+ messages in thread
From: Josh Hunt @ 2021-04-01 18:10 UTC (permalink / raw)
To: Eric W. Biederman, Kees Cook; +Cc: Ingo Molnar, Peter Zijlstra, linux-kernel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] psi: allow unprivileged users with CAP_SYS_RESOURCE to write psi files
2021-04-01 3:31 Josh Hunt
2021-04-01 4:36 ` Eric W. Biederman
@ 2021-04-01 6:47 ` Peter Zijlstra
2021-04-01 19:47 ` Johannes Weiner
1 sibling, 1 reply; 8+ messages in thread
From: Peter Zijlstra @ 2021-04-01 6:47 UTC (permalink / raw)
To: Josh Hunt; +Cc: Ingo Molnar, linux-kernel, hannes
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] psi: allow unprivileged users with CAP_SYS_RESOURCE to write psi files
2021-04-01 6:47 ` Peter Zijlstra
@ 2021-04-01 19:47 ` Johannes Weiner
0 siblings, 0 replies; 8+ messages in thread
From: Johannes Weiner @ 2021-04-01 19:47 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Josh Hunt, Ingo Molnar, linux-kernel
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>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-04-01 19:48 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-01 13:16 [PATCH] psi: allow unprivileged users with CAP_SYS_RESOURCE to write psi files kernel test robot
-- strict thread matches above, loose matches on Subject: below --
2021-04-01 3:31 Josh Hunt
2021-04-01 4:36 ` Eric W. Biederman
2021-04-01 7:50 ` Kees Cook
2021-04-01 17:47 ` Eric W. Biederman
2021-04-01 18:10 ` Josh Hunt
2021-04-01 6:47 ` Peter Zijlstra
2021-04-01 19:47 ` Johannes Weiner
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.