2023-07-13 12:39:54

by Aleksa Sarai

[permalink] [raw]
Subject: [PATCH] procfs: block chmod on /proc/thread-self/comm

Due to an oversight in commit 1b3044e39a89 ("procfs: fix pthread
cross-thread naming if !PR_DUMPABLE") in switching from REG to NOD,
chmod operations on /proc/thread-self/comm were no longer blocked as
they are on almost all other procfs files.

A very similar situation with /proc/self/environ was used to as a root
exploit a long time ago, but procfs has SB_I_NOEXEC so this is simply a
correctness issue.

Ref: https://lwn.net/Articles/191954/
Ref: 6d76fa58b050 ("Don't allow chmod() on the /proc/<pid>/ files")
Fixes: 1b3044e39a89 ("procfs: fix pthread cross-thread naming if !PR_DUMPABLE")
Cc: [email protected] # v4.7+
Signed-off-by: Aleksa Sarai <[email protected]>
---
fs/proc/base.c | 3 ++-
tools/testing/selftests/nolibc/nolibc-test.c | 4 ++++
2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 05452c3b9872..7394229816f3 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -3583,7 +3583,8 @@ static int proc_tid_comm_permission(struct mnt_idmap *idmap,
}

static const struct inode_operations proc_tid_comm_inode_operations = {
- .permission = proc_tid_comm_permission,
+ .setattr = proc_setattr,
+ .permission = proc_tid_comm_permission,
};

/*
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
index 486334981e60..08f0969208eb 100644
--- a/tools/testing/selftests/nolibc/nolibc-test.c
+++ b/tools/testing/selftests/nolibc/nolibc-test.c
@@ -580,6 +580,10 @@ int run_syscall(int min, int max)
CASE_TEST(chmod_net); EXPECT_SYSZR(proc, chmod("/proc/self/net", 0555)); break;
CASE_TEST(chmod_self); EXPECT_SYSER(proc, chmod("/proc/self", 0555), -1, EPERM); break;
CASE_TEST(chown_self); EXPECT_SYSER(proc, chown("/proc/self", 0, 0), -1, EPERM); break;
+ CASE_TEST(chmod_self_comm); EXPECT_SYSER(proc, chmod("/proc/self/comm", 0777), -1, EPERM); break;
+ CASE_TEST(chmod_tid_comm); EXPECT_SYSER(proc, chmod("/proc/thread-self/comm", 0777), -1, EPERM); break;
+ CASE_TEST(chmod_self_environ);EXPECT_SYSER(proc, chmod("/proc/self/environ", 0777), -1, EPERM); break;
+ CASE_TEST(chmod_tid_environ); EXPECT_SYSER(proc, chmod("/proc/thread-self/environ", 0777), -1, EPERM); break;
CASE_TEST(chroot_root); EXPECT_SYSZR(euid0, chroot("/")); break;
CASE_TEST(chroot_blah); EXPECT_SYSER(1, chroot("/proc/self/blah"), -1, ENOENT); break;
CASE_TEST(chroot_exe); EXPECT_SYSER(proc, chroot("/proc/self/exe"), -1, ENOTDIR); break;
--
2.41.0



2023-07-13 12:59:53

by Willy Tarreau

[permalink] [raw]
Subject: Re: [PATCH] procfs: block chmod on /proc/thread-self/comm

+Cc Thomas Wei?schuh <[email protected]> as this seems quite related to
his finding about /proc/self/net:

https://lore.kernel.org/lkml/[email protected]/#b

On Thu, Jul 13, 2023 at 10:19:04PM +1000, Aleksa Sarai wrote:
> Due to an oversight in commit 1b3044e39a89 ("procfs: fix pthread
> cross-thread naming if !PR_DUMPABLE") in switching from REG to NOD,
> chmod operations on /proc/thread-self/comm were no longer blocked as
> they are on almost all other procfs files.
>
> A very similar situation with /proc/self/environ was used to as a root
> exploit a long time ago, but procfs has SB_I_NOEXEC so this is simply a
> correctness issue.
>
> Ref: https://lwn.net/Articles/191954/
> Ref: 6d76fa58b050 ("Don't allow chmod() on the /proc/<pid>/ files")
> Fixes: 1b3044e39a89 ("procfs: fix pthread cross-thread naming if !PR_DUMPABLE")
> Cc: [email protected] # v4.7+
> Signed-off-by: Aleksa Sarai <[email protected]>
> ---
> fs/proc/base.c | 3 ++-
> tools/testing/selftests/nolibc/nolibc-test.c | 4 ++++
> 2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 05452c3b9872..7394229816f3 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -3583,7 +3583,8 @@ static int proc_tid_comm_permission(struct mnt_idmap *idmap,
> }
>
> static const struct inode_operations proc_tid_comm_inode_operations = {
> - .permission = proc_tid_comm_permission,
> + .setattr = proc_setattr,
> + .permission = proc_tid_comm_permission,
> };
>
> /*
> diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> index 486334981e60..08f0969208eb 100644
> --- a/tools/testing/selftests/nolibc/nolibc-test.c
> +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> @@ -580,6 +580,10 @@ int run_syscall(int min, int max)
> CASE_TEST(chmod_net); EXPECT_SYSZR(proc, chmod("/proc/self/net", 0555)); break;
> CASE_TEST(chmod_self); EXPECT_SYSER(proc, chmod("/proc/self", 0555), -1, EPERM); break;
> CASE_TEST(chown_self); EXPECT_SYSER(proc, chown("/proc/self", 0, 0), -1, EPERM); break;
> + CASE_TEST(chmod_self_comm); EXPECT_SYSER(proc, chmod("/proc/self/comm", 0777), -1, EPERM); break;
> + CASE_TEST(chmod_tid_comm); EXPECT_SYSER(proc, chmod("/proc/thread-self/comm", 0777), -1, EPERM); break;
> + CASE_TEST(chmod_self_environ);EXPECT_SYSER(proc, chmod("/proc/self/environ", 0777), -1, EPERM); break;
> + CASE_TEST(chmod_tid_environ); EXPECT_SYSER(proc, chmod("/proc/thread-self/environ", 0777), -1, EPERM); break;
> CASE_TEST(chroot_root); EXPECT_SYSZR(euid0, chroot("/")); break;
> CASE_TEST(chroot_blah); EXPECT_SYSER(1, chroot("/proc/self/blah"), -1, ENOENT); break;
> CASE_TEST(chroot_exe); EXPECT_SYSER(proc, chroot("/proc/self/exe"), -1, ENOTDIR); break;
> --
> 2.41.0

2023-07-13 13:04:25

by Thomas Weißschuh

[permalink] [raw]
Subject: Re: [PATCH] procfs: block chmod on /proc/thread-self/comm

On 2023-07-13 22:19:04+1000, Aleksa Sarai wrote:
> Due to an oversight in commit 1b3044e39a89 ("procfs: fix pthread
> cross-thread naming if !PR_DUMPABLE") in switching from REG to NOD,
> chmod operations on /proc/thread-self/comm were no longer blocked as
> they are on almost all other procfs files.
>
> A very similar situation with /proc/self/environ was used to as a root
> exploit a long time ago, but procfs has SB_I_NOEXEC so this is simply a
> correctness issue.
>
> Ref: https://lwn.net/Articles/191954/
> Ref: 6d76fa58b050 ("Don't allow chmod() on the /proc/<pid>/ files")
> Fixes: 1b3044e39a89 ("procfs: fix pthread cross-thread naming if !PR_DUMPABLE")
> Cc: [email protected] # v4.7+
> Signed-off-by: Aleksa Sarai <[email protected]>
> ---
> fs/proc/base.c | 3 ++-
> tools/testing/selftests/nolibc/nolibc-test.c | 4 ++++
> 2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 05452c3b9872..7394229816f3 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -3583,7 +3583,8 @@ static int proc_tid_comm_permission(struct mnt_idmap *idmap,
> }
>
> static const struct inode_operations proc_tid_comm_inode_operations = {
> - .permission = proc_tid_comm_permission,
> + .setattr = proc_setattr,
> + .permission = proc_tid_comm_permission,
> };

Given that this seems to be a recurring theme a more systematic
aproach would help.

Something like the following (untested) patch:

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 05452c3b9872..b90f2e9cda66 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2649,6 +2649,7 @@ static struct dentry *proc_pident_instantiate(struct dentry *dentry,
set_nlink(inode, 2); /* Use getattr to fix if necessary */
if (p->iop)
inode->i_op = p->iop;
+ WARN_ON(!inode->i_op->setattr);
if (p->fop)
inode->i_fop = p->fop;
ei->op = p->op;

> /*
> diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> index 486334981e60..08f0969208eb 100644
> --- a/tools/testing/selftests/nolibc/nolibc-test.c
> +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> @@ -580,6 +580,10 @@ int run_syscall(int min, int max)
> CASE_TEST(chmod_net); EXPECT_SYSZR(proc, chmod("/proc/self/net", 0555)); break;
> CASE_TEST(chmod_self); EXPECT_SYSER(proc, chmod("/proc/self", 0555), -1, EPERM); break;
> CASE_TEST(chown_self); EXPECT_SYSER(proc, chown("/proc/self", 0, 0), -1, EPERM); break;
> + CASE_TEST(chmod_self_comm); EXPECT_SYSER(proc, chmod("/proc/self/comm", 0777), -1, EPERM); break;
> + CASE_TEST(chmod_tid_comm); EXPECT_SYSER(proc, chmod("/proc/thread-self/comm", 0777), -1, EPERM); break;
> + CASE_TEST(chmod_self_environ);EXPECT_SYSER(proc, chmod("/proc/self/environ", 0777), -1, EPERM); break;
> + CASE_TEST(chmod_tid_environ); EXPECT_SYSER(proc, chmod("/proc/thread-self/environ", 0777), -1, EPERM); break;

I'm not a big fan of this, it abuses the nolibc testsuite to test core
kernel functionality.
If this needs to be tested explicitly there is hopefully a better place.

Those existing tests focus on testing functionality provided by nolibc.
The test chmod_net just got removed because it suffered from the same
bug as /proc/thread-self/comm.

> CASE_TEST(chroot_root); EXPECT_SYSZR(euid0, chroot("/")); break;
> CASE_TEST(chroot_blah); EXPECT_SYSER(1, chroot("/proc/self/blah"), -1, ENOENT); break;
> CASE_TEST(chroot_exe); EXPECT_SYSER(proc, chroot("/proc/self/exe"), -1, ENOTDIR); break;
> --
> 2.41.0
>

2023-07-13 13:23:31

by Christian Brauner

[permalink] [raw]
Subject: Re: [PATCH] procfs: block chmod on /proc/thread-self/comm

On Thu, Jul 13, 2023 at 03:01:24PM +0200, Thomas Weißschuh wrote:
> On 2023-07-13 22:19:04+1000, Aleksa Sarai wrote:
> > Due to an oversight in commit 1b3044e39a89 ("procfs: fix pthread
> > cross-thread naming if !PR_DUMPABLE") in switching from REG to NOD,
> > chmod operations on /proc/thread-self/comm were no longer blocked as
> > they are on almost all other procfs files.
> >
> > A very similar situation with /proc/self/environ was used to as a root
> > exploit a long time ago, but procfs has SB_I_NOEXEC so this is simply a
> > correctness issue.
> >
> > Ref: https://lwn.net/Articles/191954/
> > Ref: 6d76fa58b050 ("Don't allow chmod() on the /proc/<pid>/ files")
> > Fixes: 1b3044e39a89 ("procfs: fix pthread cross-thread naming if !PR_DUMPABLE")
> > Cc: [email protected] # v4.7+
> > Signed-off-by: Aleksa Sarai <[email protected]>
> > ---
> > fs/proc/base.c | 3 ++-
> > tools/testing/selftests/nolibc/nolibc-test.c | 4 ++++
> > 2 files changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index 05452c3b9872..7394229816f3 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -3583,7 +3583,8 @@ static int proc_tid_comm_permission(struct mnt_idmap *idmap,
> > }
> >
> > static const struct inode_operations proc_tid_comm_inode_operations = {
> > - .permission = proc_tid_comm_permission,
> > + .setattr = proc_setattr,
> > + .permission = proc_tid_comm_permission,
> > };
>
> Given that this seems to be a recurring theme a more systematic
> aproach would help.
>
> Something like the following (untested) patch:
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 05452c3b9872..b90f2e9cda66 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2649,6 +2649,7 @@ static struct dentry *proc_pident_instantiate(struct dentry *dentry,
> set_nlink(inode, 2); /* Use getattr to fix if necessary */
> if (p->iop)
> inode->i_op = p->iop;
> + WARN_ON(!inode->i_op->setattr);

Hm, no. This is hacky.

To fix this properly we will need to wean off notify_change() from
falling back to simple_setattr() when no i_op->setattr() method is
defined. To do that we will have to go through every filesystem and port
all that rely on this fallback to set simple_setattr() explicitly as
their i_op->setattr() method.

Christoph and I just discussed this in relation to another patch.

This is a bugfix so it should be as minimal as possible for easy
backport.

2023-07-13 13:33:46

by Christian Brauner

[permalink] [raw]
Subject: Re: [PATCH] procfs: block chmod on /proc/thread-self/comm

> > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> > index 486334981e60..08f0969208eb 100644
> > --- a/tools/testing/selftests/nolibc/nolibc-test.c
> > +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> > @@ -580,6 +580,10 @@ int run_syscall(int min, int max)
> > CASE_TEST(chmod_net); EXPECT_SYSZR(proc, chmod("/proc/self/net", 0555)); break;
> > CASE_TEST(chmod_self); EXPECT_SYSER(proc, chmod("/proc/self", 0555), -1, EPERM); break;
> > CASE_TEST(chown_self); EXPECT_SYSER(proc, chown("/proc/self", 0, 0), -1, EPERM); break;
> > + CASE_TEST(chmod_self_comm); EXPECT_SYSER(proc, chmod("/proc/self/comm", 0777), -1, EPERM); break;
> > + CASE_TEST(chmod_tid_comm); EXPECT_SYSER(proc, chmod("/proc/thread-self/comm", 0777), -1, EPERM); break;
> > + CASE_TEST(chmod_self_environ);EXPECT_SYSER(proc, chmod("/proc/self/environ", 0777), -1, EPERM); break;
> > + CASE_TEST(chmod_tid_environ); EXPECT_SYSER(proc, chmod("/proc/thread-self/environ", 0777), -1, EPERM); break;

>
> I'm not a big fan of this, it abuses the nolibc testsuite to test core
> kernel functionality.

Yes, this should be dropped.
We need a minimal patch to fix this. This just makes backporting harder
and any test doesn't need to be backported.

2023-07-13 14:16:07

by Aleksa Sarai

[permalink] [raw]
Subject: Re: [PATCH] procfs: block chmod on /proc/thread-self/comm

On 2023-07-13, Christian Brauner <[email protected]> wrote:
> > > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> > > index 486334981e60..08f0969208eb 100644
> > > --- a/tools/testing/selftests/nolibc/nolibc-test.c
> > > +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> > > @@ -580,6 +580,10 @@ int run_syscall(int min, int max)
> > > CASE_TEST(chmod_net); EXPECT_SYSZR(proc, chmod("/proc/self/net", 0555)); break;
> > > CASE_TEST(chmod_self); EXPECT_SYSER(proc, chmod("/proc/self", 0555), -1, EPERM); break;
> > > CASE_TEST(chown_self); EXPECT_SYSER(proc, chown("/proc/self", 0, 0), -1, EPERM); break;
> > > + CASE_TEST(chmod_self_comm); EXPECT_SYSER(proc, chmod("/proc/self/comm", 0777), -1, EPERM); break;
> > > + CASE_TEST(chmod_tid_comm); EXPECT_SYSER(proc, chmod("/proc/thread-self/comm", 0777), -1, EPERM); break;
> > > + CASE_TEST(chmod_self_environ);EXPECT_SYSER(proc, chmod("/proc/self/environ", 0777), -1, EPERM); break;
> > > + CASE_TEST(chmod_tid_environ); EXPECT_SYSER(proc, chmod("/proc/thread-self/environ", 0777), -1, EPERM); break;
>
> >
> > I'm not a big fan of this, it abuses the nolibc testsuite to test core
> > kernel functionality.
>
> Yes, this should be dropped.
> We need a minimal patch to fix this. This just makes backporting harder
> and any test doesn't need to be backported.

Alright, I'll drop it in v2 (though I'm not sure why there are tests for
/proc/self and /proc/self/net then).

--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

2023-07-13 14:18:50

by Christian Brauner

[permalink] [raw]
Subject: Re: [PATCH] procfs: block chmod on /proc/thread-self/comm

On Fri, Jul 14, 2023 at 12:00:51AM +1000, Aleksa Sarai wrote:
> On 2023-07-13, Christian Brauner <[email protected]> wrote:
> > > > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> > > > index 486334981e60..08f0969208eb 100644
> > > > --- a/tools/testing/selftests/nolibc/nolibc-test.c
> > > > +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> > > > @@ -580,6 +580,10 @@ int run_syscall(int min, int max)
> > > > CASE_TEST(chmod_net); EXPECT_SYSZR(proc, chmod("/proc/self/net", 0555)); break;
> > > > CASE_TEST(chmod_self); EXPECT_SYSER(proc, chmod("/proc/self", 0555), -1, EPERM); break;
> > > > CASE_TEST(chown_self); EXPECT_SYSER(proc, chown("/proc/self", 0, 0), -1, EPERM); break;
> > > > + CASE_TEST(chmod_self_comm); EXPECT_SYSER(proc, chmod("/proc/self/comm", 0777), -1, EPERM); break;
> > > > + CASE_TEST(chmod_tid_comm); EXPECT_SYSER(proc, chmod("/proc/thread-self/comm", 0777), -1, EPERM); break;
> > > > + CASE_TEST(chmod_self_environ);EXPECT_SYSER(proc, chmod("/proc/self/environ", 0777), -1, EPERM); break;
> > > > + CASE_TEST(chmod_tid_environ); EXPECT_SYSER(proc, chmod("/proc/thread-self/environ", 0777), -1, EPERM); break;
> >
> > >
> > > I'm not a big fan of this, it abuses the nolibc testsuite to test core
> > > kernel functionality.
> >
> > Yes, this should be dropped.
> > We need a minimal patch to fix this. This just makes backporting harder
> > and any test doesn't need to be backported.
>
> Alright, I'll drop it in v2 (though I'm not sure why there are tests for
> /proc/self and /proc/self/net then).

If you wanted to add tests as a separate patch they should very likely
go into tools/testing/selftets/proc.

2023-07-13 14:35:34

by Aleksa Sarai

[permalink] [raw]
Subject: Re: [PATCH] procfs: block chmod on /proc/thread-self/comm

On 2023-07-13, Willy Tarreau <[email protected]> wrote:
> +Cc Thomas Wei?schuh <[email protected]> as this seems quite related to
> his finding about /proc/self/net:
>
> https://lore.kernel.org/lkml/[email protected]/#b

Yeah I saw this patch and (along with an earlier discussion with
Christian on the topic of chmod on symlinks -- see [1]) lead us to find
that there were three other cases where this happens unintentionally:

* /proc/self (on the symlink itself)
* /proc/thread-self (on the symlink itself)
* /proc/thread-self/comm

The first two will be fixed by [1] so fixing them isn't necessary.

[1]: https://lore.kernel.org/linux-fsdevel/[email protected]/

--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

2023-07-13 14:36:24

by Willy Tarreau

[permalink] [raw]
Subject: Re: [PATCH] procfs: block chmod on /proc/thread-self/comm

On Fri, Jul 14, 2023 at 12:00:51AM +1000, Aleksa Sarai wrote:
> On 2023-07-13, Christian Brauner <[email protected]> wrote:
> > > > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> > > > index 486334981e60..08f0969208eb 100644
> > > > --- a/tools/testing/selftests/nolibc/nolibc-test.c
> > > > +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> > > > @@ -580,6 +580,10 @@ int run_syscall(int min, int max)
> > > > CASE_TEST(chmod_net); EXPECT_SYSZR(proc, chmod("/proc/self/net", 0555)); break;
> > > > CASE_TEST(chmod_self); EXPECT_SYSER(proc, chmod("/proc/self", 0555), -1, EPERM); break;
> > > > CASE_TEST(chown_self); EXPECT_SYSER(proc, chown("/proc/self", 0, 0), -1, EPERM); break;
> > > > + CASE_TEST(chmod_self_comm); EXPECT_SYSER(proc, chmod("/proc/self/comm", 0777), -1, EPERM); break;
> > > > + CASE_TEST(chmod_tid_comm); EXPECT_SYSER(proc, chmod("/proc/thread-self/comm", 0777), -1, EPERM); break;
> > > > + CASE_TEST(chmod_self_environ);EXPECT_SYSER(proc, chmod("/proc/self/environ", 0777), -1, EPERM); break;
> > > > + CASE_TEST(chmod_tid_environ); EXPECT_SYSER(proc, chmod("/proc/thread-self/environ", 0777), -1, EPERM); break;
> >
> > >
> > > I'm not a big fan of this, it abuses the nolibc testsuite to test core
> > > kernel functionality.
> >
> > Yes, this should be dropped.
> > We need a minimal patch to fix this. This just makes backporting harder
> > and any test doesn't need to be backported.
>
> Alright, I'll drop it in v2 (though I'm not sure why there are tests for
> /proc/self and /proc/self/net then).

In fact the goal was to rely on existing entries that were certain to
return certain errors, as we are testing nolibc syscalls in limited
environments, such as not being able to create a new file due to another
syscall not being available yet. /proc is convenient to make a number
of syscalls fail. That's how the problem was detected by the way :-)

I personally don't mind that much that tests would be added, provided
they really test a new syscall+error combination each. As Thomas said,
here we already have other tests for chmod+EPERM so these ones do not
bring value here for the purpose of this specific test.

With that in mind, if there is some perceived value in adding such
tests, that's something we could discuss, either in this file as another
category or (preferably) in a separate one, because the framework makes
this easy. We could for example have a "proc-test" sub-project forked
from this one to run various tests on /proc file permissions. This would
respect a clean split, with nolibc-test assuming a valid kernel to test
a libc, and proc-test assuming a valid libc to test the kernel. Just an
idea.

Regards,
willy

2023-07-13 14:39:03

by Thomas Weißschuh

[permalink] [raw]
Subject: Re: [PATCH] procfs: block chmod on /proc/thread-self/comm

On 2023-07-14 00:00:51+1000, Aleksa Sarai wrote:
> On 2023-07-13, Christian Brauner <[email protected]> wrote:
> > > > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> > > > index 486334981e60..08f0969208eb 100644
> > > > --- a/tools/testing/selftests/nolibc/nolibc-test.c
> > > > +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> > > > @@ -580,6 +580,10 @@ int run_syscall(int min, int max)
> > > > CASE_TEST(chmod_net); EXPECT_SYSZR(proc, chmod("/proc/self/net", 0555)); break;
> > > > CASE_TEST(chmod_self); EXPECT_SYSER(proc, chmod("/proc/self", 0555), -1, EPERM); break;
> > > > CASE_TEST(chown_self); EXPECT_SYSER(proc, chown("/proc/self", 0, 0), -1, EPERM); break;
> > > > + CASE_TEST(chmod_self_comm); EXPECT_SYSER(proc, chmod("/proc/self/comm", 0777), -1, EPERM); break;
> > > > + CASE_TEST(chmod_tid_comm); EXPECT_SYSER(proc, chmod("/proc/thread-self/comm", 0777), -1, EPERM); break;
> > > > + CASE_TEST(chmod_self_environ);EXPECT_SYSER(proc, chmod("/proc/self/environ", 0777), -1, EPERM); break;
> > > > + CASE_TEST(chmod_tid_environ); EXPECT_SYSER(proc, chmod("/proc/thread-self/environ", 0777), -1, EPERM); break;
> >
> > >
> > > I'm not a big fan of this, it abuses the nolibc testsuite to test core
> > > kernel functionality.
> >
> > Yes, this should be dropped.
> > We need a minimal patch to fix this. This just makes backporting harder
> > and any test doesn't need to be backported.
>
> Alright, I'll drop it in v2 (though I'm not sure why there are tests for
> /proc/self and /proc/self/net then).

To test the functionality of the implementations of chown() and chmod()
in nolibc. procfs is used used as a test fixture to provide diverse file
and directories that are (nearly) always available.

The system under test is nolibc, not the kernel itself.

Thomas