* Re: [PATCHv3 bpf-next 6/7] selftests/bpf: Add uretprobe compat test
From: Jiri Olsa @ 2024-04-29 7:39 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
linux-kernel, linux-trace-kernel, linux-api, x86, bpf, Song Liu,
Yonghong Song, John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov (AMD), Ingo Molnar, Andy Lutomirski
In-Reply-To: <CAEf4BzYU-y+vptqXpuALYecJJgPt+CTcbo+=Q9QXnu4vNwem+g@mail.gmail.com>
On Fri, Apr 26, 2024 at 11:06:53AM -0700, Andrii Nakryiko wrote:
> On Sun, Apr 21, 2024 at 12:43 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding test that adds return uprobe inside 32 bit task
> > and verify the return uprobe and attached bpf programs
> > get properly executed.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> > tools/testing/selftests/bpf/.gitignore | 1 +
> > tools/testing/selftests/bpf/Makefile | 6 ++-
> > .../selftests/bpf/prog_tests/uprobe_syscall.c | 40 +++++++++++++++++++
> > .../bpf/progs/uprobe_syscall_compat.c | 13 ++++++
> > 4 files changed, 59 insertions(+), 1 deletion(-)
> > create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscall_compat.c
> >
> > diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
> > index f1aebabfb017..69d71223c0dd 100644
> > --- a/tools/testing/selftests/bpf/.gitignore
> > +++ b/tools/testing/selftests/bpf/.gitignore
> > @@ -45,6 +45,7 @@ test_cpp
> > /veristat
> > /sign-file
> > /uprobe_multi
> > +/uprobe_compat
> > *.ko
> > *.tmp
> > xskxceiver
> > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > index edc73f8f5aef..d170b63eca62 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -134,7 +134,7 @@ TEST_GEN_PROGS_EXTENDED = test_sock_addr test_skb_cgroup_id_user \
> > xskxceiver xdp_redirect_multi xdp_synproxy veristat xdp_hw_metadata \
> > xdp_features bpf_test_no_cfi.ko
> >
> > -TEST_GEN_FILES += liburandom_read.so urandom_read sign-file uprobe_multi
> > +TEST_GEN_FILES += liburandom_read.so urandom_read sign-file uprobe_multi uprobe_compat
>
> you need to add uprobe_compat to TRUNNER_EXTRA_FILES as well, no?
ah right
> > diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> > index 9233210a4c33..3770254d893b 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> > @@ -11,6 +11,7 @@
> > #include <sys/wait.h>
> > #include "uprobe_syscall.skel.h"
> > #include "uprobe_syscall_call.skel.h"
> > +#include "uprobe_syscall_compat.skel.h"
> >
> > __naked unsigned long uretprobe_regs_trigger(void)
> > {
> > @@ -291,6 +292,35 @@ static void test_uretprobe_syscall_call(void)
> > "read_trace_pipe_iter");
> > ASSERT_EQ(found, 0, "found");
> > }
> > +
> > +static void trace_pipe_compat_cb(const char *str, void *data)
> > +{
> > + if (strstr(str, "uretprobe compat") != NULL)
> > + (*(int *)data)++;
> > +}
> > +
> > +static void test_uretprobe_compat(void)
> > +{
> > + struct uprobe_syscall_compat *skel = NULL;
> > + int err, found = 0;
> > +
> > + skel = uprobe_syscall_compat__open_and_load();
> > + if (!ASSERT_OK_PTR(skel, "uprobe_syscall_compat__open_and_load"))
> > + goto cleanup;
> > +
> > + err = uprobe_syscall_compat__attach(skel);
> > + if (!ASSERT_OK(err, "uprobe_syscall_compat__attach"))
> > + goto cleanup;
> > +
> > + system("./uprobe_compat");
> > +
> > + ASSERT_OK(read_trace_pipe_iter(trace_pipe_compat_cb, &found, 1000),
> > + "read_trace_pipe_iter");
>
> why so complicated? can't you just set global variable that it was called
hm, we execute separate uprobe_compat (32bit) process that triggers the bpf
program, so we can't use global variable.. using the trace_pipe was the only
thing that was easy to do
jirka
>
> > + ASSERT_EQ(found, 1, "found");
> > +
> > +cleanup:
> > + uprobe_syscall_compat__destroy(skel);
> > +}
> > #else
> > static void test_uretprobe_regs_equal(void)
> > {
> > @@ -306,6 +336,11 @@ static void test_uretprobe_syscall_call(void)
> > {
> > test__skip();
> > }
> > +
> > +static void test_uretprobe_compat(void)
> > +{
> > + test__skip();
> > +}
> > #endif
> >
> > void test_uprobe_syscall(void)
> > @@ -320,3 +355,8 @@ void serial_test_uprobe_syscall_call(void)
> > {
> > test_uretprobe_syscall_call();
> > }
> > +
> > +void serial_test_uprobe_syscall_compat(void)
>
> and then no need for serial_test?
>
> > +{
> > + test_uretprobe_compat();
> > +}
> > diff --git a/tools/testing/selftests/bpf/progs/uprobe_syscall_compat.c b/tools/testing/selftests/bpf/progs/uprobe_syscall_compat.c
> > new file mode 100644
> > index 000000000000..f8adde7f08e2
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/uprobe_syscall_compat.c
> > @@ -0,0 +1,13 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <linux/bpf.h>
> > +#include <bpf/bpf_helpers.h>
> > +#include <bpf/bpf_tracing.h>
> > +
> > +char _license[] SEC("license") = "GPL";
> > +
> > +SEC("uretprobe.multi/./uprobe_compat:main")
> > +int uretprobe_compat(struct pt_regs *ctx)
> > +{
> > + bpf_printk("uretprobe compat\n");
> > + return 0;
> > +}
> > --
> > 2.44.0
> >
^ permalink raw reply
* Re: [PATCHv3 bpf-next 5/7] selftests/bpf: Add uretprobe syscall call from user space test
From: Jiri Olsa @ 2024-04-29 7:33 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
linux-kernel, linux-trace-kernel, linux-api, x86, bpf, Song Liu,
Yonghong Song, John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov (AMD), Ingo Molnar, Andy Lutomirski
In-Reply-To: <CAEf4BzbWr9s2HiWU=7=okwH7PR8LHGFj2marmaOxKW61BWKHGg@mail.gmail.com>
On Fri, Apr 26, 2024 at 11:03:29AM -0700, Andrii Nakryiko wrote:
> On Sun, Apr 21, 2024 at 12:43 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding test to verify that when called from outside of the
> > trampoline provided by kernel, the uretprobe syscall will cause
> > calling process to receive SIGILL signal and the attached bpf
> > program is no executed.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> > .../selftests/bpf/prog_tests/uprobe_syscall.c | 92 +++++++++++++++++++
> > .../selftests/bpf/progs/uprobe_syscall_call.c | 15 +++
> > 2 files changed, 107 insertions(+)
> > create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscall_call.c
> >
>
> See nits below, but overall LGTM
>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
>
> [...]
>
> > @@ -219,6 +301,11 @@ static void test_uretprobe_regs_change(void)
> > {
> > test__skip();
> > }
> > +
> > +static void test_uretprobe_syscall_call(void)
> > +{
> > + test__skip();
> > +}
> > #endif
> >
> > void test_uprobe_syscall(void)
> > @@ -228,3 +315,8 @@ void test_uprobe_syscall(void)
> > if (test__start_subtest("uretprobe_regs_change"))
> > test_uretprobe_regs_change();
> > }
> > +
> > +void serial_test_uprobe_syscall_call(void)
>
> does it need to be serial? non-serial are still run sequentially
> within a process (there is no multi-threading), it's more about some
> global effects on system.
plz see below
>
> > +{
> > + test_uretprobe_syscall_call();
> > +}
> > diff --git a/tools/testing/selftests/bpf/progs/uprobe_syscall_call.c b/tools/testing/selftests/bpf/progs/uprobe_syscall_call.c
> > new file mode 100644
> > index 000000000000..5ea03bb47198
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/uprobe_syscall_call.c
> > @@ -0,0 +1,15 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include "vmlinux.h"
> > +#include <bpf/bpf_helpers.h>
> > +#include <string.h>
> > +
> > +struct pt_regs regs;
> > +
> > +char _license[] SEC("license") = "GPL";
> > +
> > +SEC("uretprobe//proc/self/exe:uretprobe_syscall_call")
> > +int uretprobe(struct pt_regs *regs)
> > +{
> > + bpf_printk("uretprobe called");
>
> debugging leftover? we probably don't want to pollute trace_pipe from test
the reason for this is to make sure the bpf program was not executed,
the test makes sure the child gets killed with SIGILL and also that
the bpf program was not executed by checking the trace_pipe and
making sure nothing was received
the trace_pipe reading is also why it's serial
jirka
>
> > + return 0;
> > +}
> > --
> > 2.44.0
> >
^ permalink raw reply
* Re: [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: stsp @ 2024-04-29 1:12 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Aleksa Sarai, Serge E. Hallyn, linux-kernel, Stefan Metzmacher,
Eric Biederman, Alexander Viro, Andy Lutomirski,
Christian Brauner, Jan Kara, Jeff Layton, Chuck Lever,
Alexander Aring, David Laight, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <eae8e7e6-9c03-4c8e-ab61-cf7060d74d6d@yandex.ru>
29.04.2024 01:12, stsp пишет:
> freely pass device nodes around, then
> perhaps the ability to pass an r/o dir fd
> that can suddenly give creds, is probably
> not something new...
Actually there is probably something
new anyway. The process knows to close
all sensitive files before dropping privs.
But this may not be the case with dirs,
because dir_fd pretty much invalidates
itself when you drop privs: you'll get
EPERM from openat() if you no longer
have rights to open the file, and dir_fd
doesn't help.
Which is why someone may neglect
to close dir_fd before dropping privs,
but with O_CRED_ALLOW that would
be a mistake.
Or what about this scenario: receiver
gets this fd thinking its some useful and
harmless file fd that would be needed
even after priv drop. So he makes sure
with F_GETFL that this fd is O_RDONLY
and doesn't close it. But it appears to be
malicious O_CRED_ALLOW dir_fd, which
basically exposes many files for a write.
So I am concerned about the cases where
an fd was not closed before a priv drop,
because the process didn't realize the threat.
> But if the*whole point* of opening the fd was to capture privileges
> and preserve them across a privilege drop, and the program loads
> malicious code after dropping privs, then that's a risk that's taken
> intentionally.
If you opened an fd yourself, then yes.
You know what have you opened, and
you care to close sensitive fds before
dropping privs, or you take the risk.
But if you requested something from
another process and got some fd as
the result, the kernel doesn't guarantee
you got an fd to what you have requested.
You can get a malicious fd, which is not
"so" possible when you open an fd yourself.
So if you want to keep such an fd open,
you will probably at least make sure its
read-only, its not a device node (with fstat)
and so on.
All checks pass, and oops, O_CRED_ALLOW...
So why my patch makes O_CRED_ALLOW
non-passable? Because the receiver has
no way to see the potential harm within
such fd. So if he intended to keep an fd
open after some basic safety checks, he
will be tricked.
So while I think its a rather bad idea to
leave the received fds open after priv drop,
I don't completely exclude the possibility
that someone did that, together with a few
safety checks which would be tricked by
O_CRED_ALLOW.
^ permalink raw reply
* Re: [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: stsp @ 2024-04-28 22:12 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Aleksa Sarai, Serge E. Hallyn, linux-kernel, Stefan Metzmacher,
Eric Biederman, Alexander Viro, Andy Lutomirski,
Christian Brauner, Jan Kara, Jeff Layton, Chuck Lever,
Alexander Aring, David Laight, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <CALCETrXPgabERgWAru7PNz6A5rc6BTG9k2RRmjU71kQs4rSsPQ@mail.gmail.com>
29.04.2024 00:30, Andy Lutomirski пишет:
> On Sun, Apr 28, 2024 at 2:15 PM stsp <stsp2@yandex.ru> wrote:
>> But isn't that becoming a problem once
>> you are (maliciously) passed such fds via
>> exec() or SCM_RIGHTS? You may not know
>> about them (or about their creds), so you
>> won't close them. Or?
> Wait, who's the malicious party?
Someone who opens an fd with O_CRED_ALLOW
and passes it to an unsuspecting process. This
is at least how I understood the Christian Brauner's
point about "unsuspecting userspace".
> Anyone who can open a directory has,
> at the time they do so, permission to do so. If you send that fd to
> someone via SCM_RIGHTS, all you accomplish is that they now have the
> fd.
Normally yes.
But fd with O_CRED_ALLOW prevents the
receiver from fully dropping his privs, even
if he doesn't want to deal with it.
> In my scenario, the malicious party attacks an *existing* program that
> opens an fd for purposes that it doesn't think are dangerous. And
> then it gives the fd *to the malicious program* by whatever means
> (could be as simple as dropping privs then doing dlopen). Then the
> malicious program does OA2_INHERIT_CREDS and gets privileges it
> shouldn't have.
But what about an inverse scenario?
Malicious program passes an fd to the
"unaware" program, putting it under a
risk. That program probably never cared
about security, since it doesn't play with
privs. But suddenly it has privs, passed
out of nowhere (via exec() for example),
and someone who hacks it, takes them.
>>>> My solution was to close such fds on
>>>> exec and disallowing SCM_RIGHTS passage.
>>> I don't see what problem this solves.
>> That the process that received them,
>> doesn't know they have O_CRED_ALLOW
>> within. So it won't deduce to close them
>> in time.
> Hold on -- what exactly are you talking about? A process does
> recvmsg() and doesn't trust the party at the other end. Then it
> doesn't close the received fd. Then it does setuid(getuid()). Then
> it does dlopen or exec of an untrusted program.
>
> Okay, so the program now has a completely unknown fd. This is already
> part of the thread model. It could be a cred-capturing fd, it could
> be a device node, it could be a socket, it could be a memfd -- it
> could be just about anything. How do any of your proposals or my
> proposals cause an actual new problem here?
I am not actually sure how widely
does this spread. I.e. /dev/mem is
restricted these days, but if you can
freely pass device nodes around, then
perhaps the ability to pass an r/o dir fd
that can suddenly give creds, is probably
not something new...
But I really don't like to add to this
particular set of cases. I don't think
its safe, I just think its legacy, so while
it is done that way currently, doesn't
mean I can do the same thing and
call it "secure" just because something
like this was already possible.
Or is this actually completely safe?
Does it hurt to have O_CRED_ALLOW
non-passable?
>>> This is fundamental to the whole model. If I stick a FAT formatted USB
>>> drive in the system and mount it, then any process that can find its
>>> way to the mountpoint can write to it. And if I open a dirfd, any
>>> process with that dirfd can write it. This is old news and isn't a
>>> problem.
>> But IIRC O_DIRECTORY only allows O_RDONLY.
>> I even re-checked now, and O_DIRECTORY|O_RDWR
>> gives EISDIR. So is it actually true that
>> whoever has dir_fd, can write to it?
> If the filesystem grants that UID permission to write, then it can write.
Which to me sounds like owning an
O_DIRECTORY fd only gives you the
ability to skip the permission checks
of the outer path components, but not
the inner ones. So passing it w/o O_CRED_ALLOW
was quite safe and didn't give you any
new abilities.
^ permalink raw reply
* Re: [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: Andy Lutomirski @ 2024-04-28 21:30 UTC (permalink / raw)
To: stsp
Cc: Aleksa Sarai, Serge E. Hallyn, linux-kernel, Stefan Metzmacher,
Eric Biederman, Alexander Viro, Andy Lutomirski,
Christian Brauner, Jan Kara, Jeff Layton, Chuck Lever,
Alexander Aring, David Laight, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <33bbaf98-db4f-4ea6-9f34-d1bebf06c0aa@yandex.ru>
On Sun, Apr 28, 2024 at 2:15 PM stsp <stsp2@yandex.ru> wrote:
>
> 28.04.2024 23:19, Andy Lutomirski пишет:
> >> Doesn't this have the same problem
> >> that was pointed to me? Namely (explaining
> >> my impl first), that if someone puts the cred
> >> fd to an unaware process's fd table, such
> >> process can't fully drop its privs. He may not
> >> want to access these dirs, but once its hacked,
> >> the hacker will access these dirs with the
> >> creds came from an outside.
> > This is not a real problem. If I have a writable fd for /etc/shadow or
> > an fd for /dev/mem, etc, then I need close them to fully drop privs.
>
> But isn't that becoming a problem once
> you are (maliciously) passed such fds via
> exec() or SCM_RIGHTS? You may not know
> about them (or about their creds), so you
> won't close them. Or?
Wait, who's the malicious party? Anyone who can open a directory has,
at the time they do so, permission to do so. If you send that fd to
someone via SCM_RIGHTS, all you accomplish is that they now have the
fd.
In my scenario, the malicious party attacks an *existing* program that
opens an fd for purposes that it doesn't think are dangerous. And
then it gives the fd *to the malicious program* by whatever means
(could be as simple as dropping privs then doing dlopen). Then the
malicious program does OA2_INHERIT_CREDS and gets privileges it
shouldn't have.
But if the *whole point* of opening the fd was to capture privileges
and preserve them across a privilege drop, and the program loads
malicious code after dropping privs, then that's a risk that's taken
intentionally. This is like how, if you do curl
http://whatever.com/foo.sh | bash, you are granting all kinds of
permissions to unknown code.
> >> My solution was to close such fds on
> >> exec and disallowing SCM_RIGHTS passage.
> > I don't see what problem this solves.
>
> That the process that received them,
> doesn't know they have O_CRED_ALLOW
> within. So it won't deduce to close them
> in time.
Hold on -- what exactly are you talking about? A process does
recvmsg() and doesn't trust the party at the other end. Then it
doesn't close the received fd. Then it does setuid(getuid()). Then
it does dlopen or exec of an untrusted program.
Okay, so the program now has a completely unknown fd. This is already
part of the thread model. It could be a cred-capturing fd, it could
be a device node, it could be a socket, it could be a memfd -- it
could be just about anything. How do any of your proposals or my
proposals cause an actual new problem here?
> > This is fundamental to the whole model. If I stick a FAT formatted USB
> > drive in the system and mount it, then any process that can find its
> > way to the mountpoint can write to it. And if I open a dirfd, any
> > process with that dirfd can write it. This is old news and isn't a
> > problem.
>
> But IIRC O_DIRECTORY only allows O_RDONLY.
> I even re-checked now, and O_DIRECTORY|O_RDWR
> gives EISDIR. So is it actually true that
> whoever has dir_fd, can write to it?
If the filesystem grants that UID permission to write, then it can write.
^ permalink raw reply
* Re: [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: stsp @ 2024-04-28 21:14 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Aleksa Sarai, Serge E. Hallyn, linux-kernel, Stefan Metzmacher,
Eric Biederman, Alexander Viro, Andy Lutomirski,
Christian Brauner, Jan Kara, Jeff Layton, Chuck Lever,
Alexander Aring, David Laight, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <CALCETrVioWt0HUt9K1vzzuxo=Hs89AjLDUjz823s4Lwn_Y0dJw@mail.gmail.com>
28.04.2024 23:19, Andy Lutomirski пишет:
>> Doesn't this have the same problem
>> that was pointed to me? Namely (explaining
>> my impl first), that if someone puts the cred
>> fd to an unaware process's fd table, such
>> process can't fully drop its privs. He may not
>> want to access these dirs, but once its hacked,
>> the hacker will access these dirs with the
>> creds came from an outside.
> This is not a real problem. If I have a writable fd for /etc/shadow or
> an fd for /dev/mem, etc, then I need close them to fully drop privs.
But isn't that becoming a problem once
you are (maliciously) passed such fds via
exec() or SCM_RIGHTS? You may not know
about them (or about their creds), so you
won't close them. Or?
> The problem is that, in current kernels, directory fds don’t allow
> access using their f_cred, and changing that means that existing
> software that does not intend to create a capability will start to do
> so.
Which is what I am trying to do. :)
>> My solution was to close such fds on
>> exec and disallowing SCM_RIGHTS passage.
> I don't see what problem this solves.
That the process that received them,
doesn't know they have O_CRED_ALLOW
within. So it won't deduce to close them
in time.
Again, this is not the only possible solution.
The receiver can indicate its will to receive
them anyway, and the kernel can check if
such transaction is safe. But it was simpler
to just disallow, who needs to pass those?
>> SCM_RIGHTS can be allowed in the future,
>> but the receiver will need to use some
>> new flag to indicate that he is willing to
>> get such an fd. Passage via exec() can
>> probably never be allowed however.
>>
>> If I understand your model correctly, you
>> put a magic sub-tree to the fs scope of some
>> unaware process.
> Only if I want that process to have access!
Who is "I" in that context?
Can it be some malicious entity?
>> He may not want to access
>> it, but once hacked, the hacker will access
>> it with the creds from an outside.
>> And, unlike in my impl, in yours there is
>> probably no way to prevent that?
> This is fundamental to the whole model. If I stick a FAT formatted USB
> drive in the system and mount it, then any process that can find its
> way to the mountpoint can write to it. And if I open a dirfd, any
> process with that dirfd can write it. This is old news and isn't a
> problem.
But IIRC O_DIRECTORY only allows O_RDONLY.
I even re-checked now, and O_DIRECTORY|O_RDWR
gives EISDIR. So is it actually true that
whoever has dir_fd, can write to it?
>> In short: my impl confines the hassle within
>> the single process. It can be extended, and
>> then the receiver will need to explicitly allow
>> adding such fds to his fd table.
>> But your idea seems to inherently require
>> 2 processes, and there is probably no way
>> for the second process to say "ok, I allow
>> such sub-tree in my fs scope".
> A process could use my proposal to open_tree directory, make a whole
> new mountns that is otherwise empty, switch to the mountns, mount the
> directory, then change its UID and drop all privs, and still have
> access to that one directory.
Ok, if that requires actions that can't
be done after priv drop, then it can
indeed fully drop privs w/o mounting
anything, if he doesn't want such access.
Then the only security-related difference
with my approach is that mine guarantees
nothing new can be accessed, no matter
who passes what. Currently nothing can
be passed at all, but if it can - my approach
would still guarantee only the same creds
can be passed, not a new ones.
Can the same restriction be applied in
your case?
^ permalink raw reply
* Re: [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: Andy Lutomirski @ 2024-04-28 20:19 UTC (permalink / raw)
To: stsp
Cc: Aleksa Sarai, Serge E. Hallyn, linux-kernel, Stefan Metzmacher,
Eric Biederman, Alexander Viro, Andy Lutomirski,
Christian Brauner, Jan Kara, Jeff Layton, Chuck Lever,
Alexander Aring, David Laight, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <8e186307-bed2-4b5c-9bc6-bdc70171cc93@yandex.ru>
> On Apr 28, 2024, at 10:39 AM, stsp <stsp2@yandex.ru> wrote:
>
> 28.04.2024 19:41, Andy Lutomirski пишет:
>>>> On Apr 26, 2024, at 6:39 AM, Stas Sergeev <stsp2@yandex.ru> wrote:
>>> This patch-set implements the OA2_CRED_INHERIT flag for openat2() syscall.
>>> It is needed to perform an open operation with the creds that were in
>>> effect when the dir_fd was opened, if the dir was opened with O_CRED_ALLOW
>>> flag. This allows the process to pre-open some dirs and switch eUID
>>> (and other UIDs/GIDs) to the less-privileged user, while still retaining
>>> the possibility to open/create files within the pre-opened directory set.
>>>
>> Then two different things could be done:
>>
>> 1. The subtree could be used unmounted or via /proc magic links. This
>> would be for programs that are aware of this interface.
>>
>> 2. The subtree could be mounted, and accessed through the mount would
>> use the captured creds.
> Doesn't this have the same problem
> that was pointed to me? Namely (explaining
> my impl first), that if someone puts the cred
> fd to an unaware process's fd table, such
> process can't fully drop its privs. He may not
> want to access these dirs, but once its hacked,
> the hacker will access these dirs with the
> creds came from an outside.
This is not a real problem. If I have a writable fd for /etc/shadow or
an fd for /dev/mem, etc, then I need close them to fully drop privs.
The problem is that, in current kernels, directory fds don’t allow
access using their f_cred, and changing that means that existing
software that does not intend to create a capability will start to do
so.
> My solution was to close such fds on
> exec and disallowing SCM_RIGHTS passage.
I don't see what problem this solves.
> SCM_RIGHTS can be allowed in the future,
> but the receiver will need to use some
> new flag to indicate that he is willing to
> get such an fd. Passage via exec() can
> probably never be allowed however.
>
> If I understand your model correctly, you
> put a magic sub-tree to the fs scope of some
> unaware process.
Only if I want that process to have access!
> He may not want to access
> it, but once hacked, the hacker will access
> it with the creds from an outside.
> And, unlike in my impl, in yours there is
> probably no way to prevent that?
This is fundamental to the whole model. If I stick a FAT formatted USB
drive in the system and mount it, then any process that can find its
way to the mountpoint can write to it. And if I open a dirfd, any
process with that dirfd can write it. This is old news and isn't a
problem.
>
> In short: my impl confines the hassle within
> the single process. It can be extended, and
> then the receiver will need to explicitly allow
> adding such fds to his fd table.
> But your idea seems to inherently require
> 2 processes, and there is probably no way
> for the second process to say "ok, I allow
> such sub-tree in my fs scope".
A process could use my proposal to open_tree directory, make a whole
new mountns that is otherwise empty, switch to the mountns, mount the
directory, then change its UID and drop all privs, and still have
access to that one directory.
But even right now, a process could unshare userns and mountns, map
its uid as some nonzero number, rearrange its mountns so it only has
access to that one directory, drop all privs, and seccomp itself, and
only have access to that directory, as it still has the same UID.
Take your pick.
^ permalink raw reply
* Re: [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: stsp @ 2024-04-28 19:15 UTC (permalink / raw)
To: Andy Lutomirski, Aleksa Sarai, Serge E. Hallyn
Cc: linux-kernel, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Christian Brauner, Jan Kara, Jeff Layton,
Chuck Lever, Alexander Aring, David Laight, linux-fsdevel,
linux-api, Paolo Bonzini, Christian Göttsche
In-Reply-To: <8e186307-bed2-4b5c-9bc6-bdc70171cc93@yandex.ru>
28.04.2024 20:39, stsp пишет:
> In short: my impl confines the hassle within
> the single process. It can be extended, and
> then the receiver will need to explicitly allow
> adding such fds to his fd table.
> But your idea seems to inherently require
> 2 processes, and there is probably no way
> for the second process to say "ok, I allow
> such sub-tree in my fs scope".
There is probably 1 more detail I had
to make explicit: if my model is extended
to allow SCM_RIGHTS passage by the use
of a new flag on a receiver's side, the kernel
will be able to check that the receiver
currently has the same creds as the sender.
Please note that my model is needed for the
process that does setuid() to a less-privileged
user. He would need to receive the cred fd
_before_ such setuid, he would need to use
the special flag to receive it, and the kernel
will also need to check that he has the same
creds anyway, so no escalation can ever happen.
Only that sequence of events makes the
SCM_RIGHTS passage safe, AFAICT. But if
you don't allow SCM_RIGHTS, as my current
impl does, then you are sure the process
can raise the creds only to his own ones.
Now talking about your sub-tree idea, would
it be possible to check that the process had
initially enough creds to access it w/o inheriting
anything? That part looks important to me,
i.e. the process must not access anything
that he couldn't access before dropping privs.
But I am not sure if your idea even includes
the priv drop.
^ permalink raw reply
* Re: [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: stsp @ 2024-04-28 17:39 UTC (permalink / raw)
To: Andy Lutomirski, Aleksa Sarai, Serge E. Hallyn
Cc: linux-kernel, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Christian Brauner, Jan Kara, Jeff Layton,
Chuck Lever, Alexander Aring, David Laight, linux-fsdevel,
linux-api, Paolo Bonzini, Christian Göttsche
In-Reply-To: <CALCETrUL3zXAX94CpcQYwj1omwO+=-1Li+J7Bw2kpAw4d7nsyw@mail.gmail.com>
28.04.2024 19:41, Andy Lutomirski пишет:
>> On Apr 26, 2024, at 6:39 AM, Stas Sergeev <stsp2@yandex.ru> wrote:
>> This patch-set implements the OA2_CRED_INHERIT flag for openat2() syscall.
>> It is needed to perform an open operation with the creds that were in
>> effect when the dir_fd was opened, if the dir was opened with O_CRED_ALLOW
>> flag. This allows the process to pre-open some dirs and switch eUID
>> (and other UIDs/GIDs) to the less-privileged user, while still retaining
>> the possibility to open/create files within the pre-opened directory set.
>>
> Then two different things could be done:
>
> 1. The subtree could be used unmounted or via /proc magic links. This
> would be for programs that are aware of this interface.
>
> 2. The subtree could be mounted, and accessed through the mount would
> use the captured creds.
Doesn't this have the same problem
that was pointed to me? Namely (explaining
my impl first), that if someone puts the cred
fd to an unaware process's fd table, such
process can't fully drop its privs. He may not
want to access these dirs, but once its hacked,
the hacker will access these dirs with the
creds came from an outside.
My solution was to close such fds on
exec and disallowing SCM_RIGHTS passage.
SCM_RIGHTS can be allowed in the future,
but the receiver will need to use some
new flag to indicate that he is willing to
get such an fd. Passage via exec() can
probably never be allowed however.
If I understand your model correctly, you
put a magic sub-tree to the fs scope of some
unaware process. He may not want to access
it, but once hacked, the hacker will access
it with the creds from an outside.
And, unlike in my impl, in yours there is
probably no way to prevent that?
In short: my impl confines the hassle within
the single process. It can be extended, and
then the receiver will need to explicitly allow
adding such fds to his fd table.
But your idea seems to inherently require
2 processes, and there is probably no way
for the second process to say "ok, I allow
such sub-tree in my fs scope". And even if
he could, in my impl he can just close the
cred fd, while in yours it seems to persist.
Sorry if I misunderstood your idea.
^ permalink raw reply
* Re: [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: Andy Lutomirski @ 2024-04-28 16:41 UTC (permalink / raw)
To: Stas Sergeev, Aleksa Sarai, Serge E. Hallyn
Cc: linux-kernel, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Christian Brauner, Jan Kara, Jeff Layton,
Chuck Lever, Alexander Aring, David Laight, linux-fsdevel,
linux-api, Paolo Bonzini, Christian Göttsche
In-Reply-To: <20240426133310.1159976-1-stsp2@yandex.ru>
> On Apr 26, 2024, at 6:39 AM, Stas Sergeev <stsp2@yandex.ru> wrote:
> This patch-set implements the OA2_CRED_INHERIT flag for openat2() syscall.
> It is needed to perform an open operation with the creds that were in
> effect when the dir_fd was opened, if the dir was opened with O_CRED_ALLOW
> flag. This allows the process to pre-open some dirs and switch eUID
> (and other UIDs/GIDs) to the less-privileged user, while still retaining
> the possibility to open/create files within the pre-opened directory set.
>
I’ve been contemplating this, and I want to propose a different solution.
First, the problem Stas is solving is quite narrow and doesn’t
actually need kernel support: if I want to write a user program that
sandboxes itself, I have at least three solutions already. I can make
a userns and a mountns; I can use landlock; and I can have a separate
process that brokers filesystem access using SCM_RIGHTS.
But what if I want to run a container, where the container can access
a specific host directory, and the contained application is not aware
of the exact technology being used? I recently started using
containers in anger in a production setting, and “anger” was
definitely the right word: binding part of a filesystem in is
*miserable*. Getting the DAC rules right is nasty. LSMs are worse.
Podman’s “bind,relabel” feature is IMO utterly disgusting. I think I
actually gave up on making one of my use cases work on a Fedora
system.
Here’s what I wanted to do, logically, in production: pick a host
directory, pick a host *principal* (UID, GID, label, etc), and have
the *entire container* access the directory as that principal. This is
what happens automatically if I run the whole container as a userns
with only a single UID mapped, but I don’t really want to do that for
a whole variety and of reasons.
So maybe reimagining Stas’ feature a bit can actually solve this
problem. Instead of a special dirfd, what if there was a special
subtree (in the sense of open_tree) that captures a set of creds and
does all opens inside the subtree using those creds?
This isn’t a fully formed proposal, but I *think* it should be
generally fairly safe for even an unprivileged user to clone a subtree
with a specific flag set to do this. Maybe a capability would be
needed (CAP_CAPTURE_CREDS?), but it would be nice to allow delegating
this to a daemon if a privilege is needed, and getting the API right
might be a bit tricky.
Then two different things could be done:
1. The subtree could be used unmounted or via /proc magic links. This
would be for programs that are aware of this interface.
2. The subtree could be mounted, and accessed through the mount would
use the captured creds.
(Hmm. What would a new open_tree() pointing at this special subtree do?)
With all this done, if userspace wired it up, a container user could
do something like:
—bind-capture-creds source=dest
And the contained program would access source *as the user who started
the container*, and this would just work without relabeling or
fiddling with owner uids or gids or ACLs, and it would continue to
work even if the container has multiple dynamically allocated subuids
mapped (e.g. one for “root” and one for the actual application).
Bonus points for the ability to revoke the creds in an already opened
subtree. Or even for the creds to automatically revoke themselves when
the opener exits (or maybe when a specific cred-pinning fd goes away).
(This should work for single files as well as for directories.)
New LSM hooks or extensions of existing hooks might be needed to make
LSMs comfortable with this.
What do you all think?
^ permalink raw reply
* Re: [PATCH v2] usb: gadget: f_uac2: Expose all string descriptors through configfs.
From: Pavel Hofman @ 2024-04-28 11:49 UTC (permalink / raw)
To: Chris Wulff, linux-usb@vger.kernel.org
Cc: Greg Kroah-Hartman, James Gruber, Lee Jones,
linux-kernel@vger.kernel.org, linux-api@vger.kernel.org,
Takashi Iwai
In-Reply-To: <CO1PR17MB541942196F9A2F73CF8B5B14E1152@CO1PR17MB5419.namprd17.prod.outlook.com>
On 27. 04. 24 18:27, Chris Wulff wrote:
>> From: Pavel Hofman <pavel.hofman@ivitera.com>
>>>>> + p_it_name playback input terminal name
>>>>> + p_ot_name playback output terminal name
>>>>> + p_fu_name playback function unit name
>>>>> + p_alt0_name playback alt mode 0 name
>>>>> + p_alt1_name playback alt mode 1 name
>>>>
>>>> Nacked-by: Pavel Hofman <pavel.hofman@ivitera.com>
> ...
>> If the params in the upper level were to stand as defaults for the
>> altsettings (and for the existing altsetting 1 if no specific altset
>> subdir configs were given), maybe the naming xxx_alt1_xxx could become a
>> bit confusing. E.g. p_altx_name or p_alt_non0_name?
>
> I've been prototyping this a bit to see how it will work. My current configfs
> structure looks something like:
>
> (all existing properties)
> c_it_name
> c_it_ch_name
> c_fu_name
> c_ot_name
> p_it_name
> p_it_ch_name
> p_fu_name
> p_ot_name
> num_alt_modes (settable to 2..5 in my prototype)
>
> alt.0
> c_alt_name
> p_alt_name
> alt.1 (for alt.1, alt.2, etc.)
> c_alt_name
> p_alt_name
> c_ssize
> p_ssize
> (Additional properties here for other things that are settable for each alt mode,
> but the only one I've implemented in my prototype so far is sample size.)
>
Hats off to your speed, that's amazing. IMO this is a perfect config
layout, logical, extensible, easy to generate manually as well as with a
script.
> This brings up a few questions:
>
> Is a property for setting the number of alt modes preferred, or being able to
> make directories like some other things (eg, "mkdir alt.5" would add alt mode 5)?
> The former is what I started with, but I am leaning towards the latter as it is a bit
> more flexible (you could create alt modes of any index, though I'm not entirely
> sure why you'd want to.) It does involve a bit more dynamic memory allocation,
> but nothing crazy.
I am not sure the arbitrary index of alt mode would be useful (is it
even allowed in USB specs?). But I may not have understood your question
properly.
The num_alt_modes property - can the number be perhaps aquired from the
number of created directories? Or would that number of alt modes be
created automatically (all same with default values), and the properties
in alt.X dirs would subsequently only modify their respective values?
>
> And second, should the alt.x directories go back to the defaults if you remove
> and re-create them? I'm assuming it makes sense to do that, just putting it out
> there since my current prototype doesn't work that way.
IIUC just creating the alt.X directory would create the alt X mode, with
default properties from the top-level configs or with the source-code
defaults.
Thanks a lot,
Pavel.
^ permalink raw reply
* Re: RFC: Restricting userspace interfaces for CXL fabric management
From: Sirius @ 2024-04-28 4:25 UTC (permalink / raw)
To: Dan Williams
Cc: Greg KH, Harold Johnson, Jonathan Cameron, linux-cxl,
Sreenivas Bagalkote, Brett Henning, Sumanesh Samanta,
linux-kernel, Davidlohr Bueso, Dave Jiang, Alison Schofield,
Vishal Verma, Ira Weiny, linuxarm, linux-api, Lorenzo Pieralisi,
Natu, Mahesh
In-Reply-To: <662d263dd17c7_b6e0294ab@dwillia2-mobl3.amr.corp.intel.com.notmuch>
In days of yore (Sat, 27 Apr 2024), Dan Williams thus quoth:
> Greg KH wrote:
> [..]
> > So while innovating at the hardware level is fine, follow the ways that
> > everyone has done this for other specification types (USB, PCI, etc.)
> > and just allow vendor drivers to provide the information. Don't do this
> > in crazy userspace drivers which will circumvent the whole reason we
> > have standard kernel/user apis in the first place for these types of
> > things.
>
> Right, standard kernel/user apis is the requirement.
>
> The suggestion of opaque vendor passthrough tunnels, and every vendor
> ships their custom tool to do what should be common flows, is where this
> discussion went off the rails.
One aspect of this is Fabric Management (thinking CXL3 here). It is not
desirable that every vendor of CXL hardware require their own
(proprietary) fabric management software. From a user perspective, that is
absolutely horrible. Users can, and will, mix and match CXL hardware
according to their needs (or wallet), and them having to run multiple
fabric management solutions (which in worst case are conflicting with each
other) to manage things is .. suboptimal.
By all means - innovate - but do it in such a way that interoperability
and manageability is the priority. Special sauce vendor lock-in is a
surefire way to kill CXL where it stands - don't do it.
--
Kind regards,
/S
^ permalink raw reply
* Re: [PATCH v2] usb: gadget: f_uac2: Expose all string descriptors through configfs.
From: Chris Wulff @ 2024-04-27 16:27 UTC (permalink / raw)
To: Pavel Hofman, linux-usb@vger.kernel.org
Cc: Greg Kroah-Hartman, James Gruber, Lee Jones,
linux-kernel@vger.kernel.org, linux-api@vger.kernel.org
In-Reply-To: <9b40e148-f3eb-f8f5-bf2d-37a0a0629417@ivitera.com>
>From: Pavel Hofman <pavel.hofman@ivitera.com>
>>>> + p_it_name playback input terminal name
>>>> + p_ot_name playback output terminal name
>>>> + p_fu_name playback function unit name
>>>> + p_alt0_name playback alt mode 0 name
>>>> + p_alt1_name playback alt mode 1 name
>>>
>>> Nacked-by: Pavel Hofman <pavel.hofman@ivitera.com>
...
>If the params in the upper level were to stand as defaults for the
>altsettings (and for the existing altsetting 1 if no specific altset
>subdir configs were given), maybe the naming xxx_alt1_xxx could become a
>bit confusing. E.g. p_altx_name or p_alt_non0_name?
I've been prototyping this a bit to see how it will work. My current configfs
structure looks something like:
(all existing properties)
c_it_name
c_it_ch_name
c_fu_name
c_ot_name
p_it_name
p_it_ch_name
p_fu_name
p_ot_name
num_alt_modes (settable to 2..5 in my prototype)
alt.0
c_alt_name
p_alt_name
alt.1 (for alt.1, alt.2, etc.)
c_alt_name
p_alt_name
c_ssize
p_ssize
(Additional properties here for other things that are settable for each alt mode,
but the only one I've implemented in my prototype so far is sample size.)
This brings up a few questions:
Is a property for setting the number of alt modes preferred, or being able to
make directories like some other things (eg, "mkdir alt.5" would add alt mode 5)?
The former is what I started with, but I am leaning towards the latter as it is a bit
more flexible (you could create alt modes of any index, though I'm not entirely
sure why you'd want to.) It does involve a bit more dynamic memory allocation,
but nothing crazy.
And second, should the alt.x directories go back to the defaults if you remove
and re-create them? I'm assuming it makes sense to do that, just putting it out
there since my current prototype doesn't work that way.
I appreciate your thoughts on this.
-- Chris Wulff
^ permalink raw reply
* Re: RFC: Restricting userspace interfaces for CXL fabric management
From: Dan Williams @ 2024-04-27 16:22 UTC (permalink / raw)
To: Greg KH, Harold Johnson
Cc: Jonathan Cameron, Dan Williams, linux-cxl, Sreenivas Bagalkote,
Brett Henning, Sumanesh Samanta, linux-kernel, Davidlohr Bueso,
Dave Jiang, Alison Schofield, Vishal Verma, Ira Weiny, linuxarm,
linux-api, Lorenzo Pieralisi, Natu, Mahesh
In-Reply-To: <2024042708-outscore-dreadful-2c21@gregkh>
Greg KH wrote:
[..]
> So while innovating at the hardware level is fine, follow the ways that
> everyone has done this for other specification types (USB, PCI, etc.)
> and just allow vendor drivers to provide the information. Don't do this
> in crazy userspace drivers which will circumvent the whole reason we
> have standard kernel/user apis in the first place for these types of
> things.
Right, standard kernel/user apis is the requirement.
The suggestion of opaque vendor passthrough tunnels, and every vendor
ships their custom tool to do what should be common flows, is where this
discussion went off the rails.
^ permalink raw reply
* [PATCH v6 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: Stas Sergeev @ 2024-04-27 11:24 UTC (permalink / raw)
To: linux-kernel
Cc: Stas Sergeev, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Christian Brauner, Jan Kara, Jeff Layton,
Chuck Lever, Alexander Aring, David Laight, linux-fsdevel,
linux-api, Paolo Bonzini, Christian Göttsche
This patch-set implements the OA2_CRED_INHERIT flag for openat2() syscall.
It is needed to perform an open operation with the creds that were in
effect when the dir_fd was opened, if the dir was opened with O_CRED_ALLOW
flag. This allows the process to pre-open some dirs and switch eUID
(and other UIDs/GIDs) to the less-privileged user, while still retaining
the possibility to open/create files within the pre-opened directory set.
The sand-boxing is security-oriented: symlinks leading outside of a
sand-box are rejected. /proc magic links are rejected. fds opened with
O_CRED_ALLOW are always closed on exec() and cannot be passed via unix
socket.
The more detailed description (including security considerations)
is available in the log messages of individual patches.
Changes in v6:
- it appears open flags bit 23 is already taken on parisc, and bit 24
is taken on alpha. Move O_CRED_ALLOW to bit 25.
- added selftests for both O_CRED_ALLOW and O_CRED_INHERIT additions
Changes in v5:
- rename OA2_INHERIT_CRED to OA2_CRED_INHERIT
- add an "opt-in" flag O_CRED_ALLOW as was suggested by many reviewers
- stop using 64bit types, as suggested by
Christian Brauner <brauner@kernel.org>
- add BUILD_BUG_ON() for VALID_OPENAT2_FLAGS, based on Christian Brauner's
comments
- fixed problems reported by patch-testing bot
- made O_CRED_ALLOW fds not passable via unix sockets and exec(),
based on Christian Brauner's comments
Changes in v4:
- add optimizations suggested by David Laight <David.Laight@ACULAB.COM>
- move security checks to build_open_flags()
- force RESOLVE_NO_MAGICLINKS as suggested by Andy Lutomirski <luto@kernel.org>
Changes in v3:
- partially revert v2 changes to avoid overriding capabilities.
Only the bare minimum is overridden: fsuid, fsgid and group_info.
Document the fact the full cred override is unwanted, as it may
represent an unneeded security risk.
Changes in v2:
- capture full struct cred instead of just fsuid/fsgid.
Suggested by Stefan Metzmacher <metze@samba.org>
CC: Stefan Metzmacher <metze@samba.org>
CC: Eric Biederman <ebiederm@xmission.com>
CC: Alexander Viro <viro@zeniv.linux.org.uk>
CC: Andy Lutomirski <luto@kernel.org>
CC: Christian Brauner <brauner@kernel.org>
CC: Jan Kara <jack@suse.cz>
CC: Jeff Layton <jlayton@kernel.org>
CC: Chuck Lever <chuck.lever@oracle.com>
CC: Alexander Aring <alex.aring@gmail.com>
CC: David Laight <David.Laight@ACULAB.COM>
CC: linux-fsdevel@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: linux-api@vger.kernel.org
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Christian Göttsche <cgzones@googlemail.com>
--
2.44.0
^ permalink raw reply
* [PATCH v6 2/3] open: add O_CRED_ALLOW flag
From: Stas Sergeev @ 2024-04-27 11:24 UTC (permalink / raw)
To: linux-kernel
Cc: Stas Sergeev, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Christian Brauner, Jan Kara, Jeff Layton,
Chuck Lever, Alexander Aring, David Laight, linux-fsdevel,
linux-api, Paolo Bonzini, Christian Göttsche, Arnd Bergmann,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jens Axboe, Kuniyuki Iwashima, Pavel Begunkov, linux-arch, netdev
In-Reply-To: <20240427112451.1609471-1-stsp2@yandex.ru>
This flag prevents an fd from being passed via unix socket, and
makes it to be always closed on exec().
Selftest is added to check for both properties.
It is needed for the subsequent OA2_CRED_INHERIT addition, to work
as an "opt-in" for the new cred-inherit functionality. Without using
O_CRED_ALLOW when opening dir fd, OA2_CRED_INHERIT is going to return
EPERM.
Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
CC: Eric Biederman <ebiederm@xmission.com>
CC: Alexander Viro <viro@zeniv.linux.org.uk>
CC: Christian Brauner <brauner@kernel.org>
CC: Jan Kara <jack@suse.cz>
CC: Andy Lutomirski <luto@kernel.org>
CC: David Laight <David.Laight@ACULAB.COM>
CC: Arnd Bergmann <arnd@arndb.de>
CC: "David S. Miller" <davem@davemloft.net>
CC: Eric Dumazet <edumazet@google.com>
CC: Jakub Kicinski <kuba@kernel.org>
CC: Paolo Abeni <pabeni@redhat.com>
CC: Jens Axboe <axboe@kernel.dk>
CC: Kuniyuki Iwashima <kuniyu@amazon.com>
CC: Pavel Begunkov <asml.silence@gmail.com>
CC: linux-arch@vger.kernel.org
CC: netdev@vger.kernel.org
CC: linux-fsdevel@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: linux-api@vger.kernel.org
---
fs/fcntl.c | 2 +-
fs/file.c | 15 +--
include/linux/fcntl.h | 2 +-
include/uapi/asm-generic/fcntl.h | 5 +
net/core/scm.c | 5 +
tools/testing/selftests/core/Makefile | 2 +-
tools/testing/selftests/core/cred_allow.c | 139 ++++++++++++++++++++++
7 files changed, 160 insertions(+), 10 deletions(-)
create mode 100644 tools/testing/selftests/core/cred_allow.c
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 54cc85d3338e..78c96b1293c2 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -1039,7 +1039,7 @@ static int __init fcntl_init(void)
* Exceptions: O_NONBLOCK is a two bit define on parisc; O_NDELAY
* is defined as O_NONBLOCK on some platforms and not on others.
*/
- BUILD_BUG_ON(21 - 1 /* for O_RDONLY being 0 */ !=
+ BUILD_BUG_ON(22 - 1 /* for O_RDONLY being 0 */ !=
HWEIGHT32(
(VALID_OPEN_FLAGS & ~(O_NONBLOCK | O_NDELAY)) |
__FMODE_EXEC | __FMODE_NONOTIFY));
diff --git a/fs/file.c b/fs/file.c
index 3b683b9101d8..2a09d5276676 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -827,22 +827,23 @@ void do_close_on_exec(struct files_struct *files)
/* exec unshares first */
spin_lock(&files->file_lock);
for (i = 0; ; i++) {
+ int j;
unsigned long set;
unsigned fd = i * BITS_PER_LONG;
fdt = files_fdtable(files);
if (fd >= fdt->max_fds)
break;
set = fdt->close_on_exec[i];
- if (!set)
- continue;
fdt->close_on_exec[i] = 0;
- for ( ; set ; fd++, set >>= 1) {
- struct file *file;
- if (!(set & 1))
- continue;
- file = fdt->fd[fd];
+ for (j = 0; j < BITS_PER_LONG; j++, fd++, set >>= 1) {
+ struct file *file = fdt->fd[fd];
if (!file)
continue;
+ /* Close all cred-allow files. */
+ if (file->f_flags & O_CRED_ALLOW)
+ set |= 1;
+ if (!(set & 1))
+ continue;
rcu_assign_pointer(fdt->fd[fd], NULL);
__put_unused_fd(files, fd);
spin_unlock(&files->file_lock);
diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
index a332e79b3207..e074ee9c1e36 100644
--- a/include/linux/fcntl.h
+++ b/include/linux/fcntl.h
@@ -10,7 +10,7 @@
(O_RDONLY | O_WRONLY | O_RDWR | O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC | \
O_APPEND | O_NDELAY | O_NONBLOCK | __O_SYNC | O_DSYNC | \
FASYNC | O_DIRECT | O_LARGEFILE | O_DIRECTORY | O_NOFOLLOW | \
- O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE)
+ O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE | O_CRED_ALLOW)
/* List of all valid flags for the how->resolve argument: */
#define VALID_RESOLVE_FLAGS \
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index 80f37a0d40d7..9244c54bb933 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -89,6 +89,11 @@
#define __O_TMPFILE 020000000
#endif
+#ifndef O_CRED_ALLOW
+/* On parisc bit 23 is taken. On alpha bit 24 is also taken. Try bit 25. */
+#define O_CRED_ALLOW 0200000000
+#endif
+
/* a horrid kludge trying to make sure that this will fail on old kernels */
#define O_TMPFILE (__O_TMPFILE | O_DIRECTORY)
diff --git a/net/core/scm.c b/net/core/scm.c
index 9cd4b0a01cd6..f54fb0ee9727 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -111,6 +111,11 @@ static int scm_fp_copy(struct cmsghdr *cmsg, struct scm_fp_list **fplp)
fput(file);
return -EINVAL;
}
+ /* don't allow files with creds */
+ if (file->f_flags & O_CRED_ALLOW) {
+ fput(file);
+ return -EPERM;
+ }
if (unix_get_socket(file))
fpl->count_unix++;
diff --git a/tools/testing/selftests/core/Makefile b/tools/testing/selftests/core/Makefile
index ce262d097269..347a5a9d3f29 100644
--- a/tools/testing/selftests/core/Makefile
+++ b/tools/testing/selftests/core/Makefile
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: GPL-2.0-only
CFLAGS += -g $(KHDR_INCLUDES)
-TEST_GEN_PROGS := close_range_test
+TEST_GEN_PROGS := close_range_test cred_allow
include ../lib.mk
diff --git a/tools/testing/selftests/core/cred_allow.c b/tools/testing/selftests/core/cred_allow.c
new file mode 100644
index 000000000000..07d533207a2c
--- /dev/null
+++ b/tools/testing/selftests/core/cred_allow.c
@@ -0,0 +1,139 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <errno.h>
+#include <fcntl.h>
+#include <sched.h>
+#include <stdio.h>
+#include <stdbool.h>
+#include <sys/stat.h>
+#include <sys/syscall.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <sys/socket.h>
+#include <time.h>
+#include <unistd.h>
+#include <string.h>
+
+#include "../kselftest.h"
+
+#ifndef O_CRED_ALLOW
+#define O_CRED_ALLOW 0x2000000
+#endif
+
+enum { FD_NORM, FD_CE, FD_CA, FD_MAX };
+
+static int is_opened(int n)
+{
+ char buf[256];
+
+ snprintf(buf, sizeof(buf), "/proc/self/fd/%d", n);
+ return (access(buf, F_OK) == 0);
+}
+
+/* Sends an FD on a UNIX socket. Returns 0 on success or -errno. */
+static int send_fd(int usock, int fd_tx)
+{
+ union {
+ /* Aligned ancillary data buffer. */
+ char buf[CMSG_SPACE(sizeof(fd_tx))];
+ struct cmsghdr _align;
+ } cmsg_tx = {};
+ char data_tx = '.';
+ struct iovec io = {
+ .iov_base = &data_tx,
+ .iov_len = sizeof(data_tx),
+ };
+ struct msghdr msg = {
+ .msg_iov = &io,
+ .msg_iovlen = 1,
+ .msg_control = &cmsg_tx.buf,
+ .msg_controllen = sizeof(cmsg_tx.buf),
+ };
+ struct cmsghdr *cmsg = CMSG_FIRSTHDR(&msg);
+
+ cmsg->cmsg_len = CMSG_LEN(sizeof(fd_tx));
+ cmsg->cmsg_level = SOL_SOCKET;
+ cmsg->cmsg_type = SCM_RIGHTS;
+ memcpy(CMSG_DATA(cmsg), &fd_tx, sizeof(fd_tx));
+
+ if (sendmsg(usock, &msg, 0) < 0)
+ return -errno;
+ return 0;
+}
+
+int main(int argc, char *argv[], char *env[])
+{
+ int status;
+ int err;
+ pid_t pid;
+ int socket_fds[2];
+ int fds[FD_MAX];
+#define NFD(n) ((n) + 3)
+#define FD_OK(n) (NFD(n) == fds[n] && is_opened(fds[n]))
+
+ if (argc > 1 && strcmp(argv[1], "--child") == 0) {
+ int nfd = 0;
+ ksft_print_msg("we are child\n");
+ ksft_set_plan(3);
+ nfd += is_opened(NFD(FD_NORM));
+ ksft_test_result(nfd == 1, "normal fd opened\n");
+ nfd += is_opened(NFD(FD_CE));
+ ksft_test_result(nfd == 1, "O_CLOEXEC fd closed\n");
+ nfd += is_opened(NFD(FD_CA));
+ ksft_test_result(nfd == 1, "O_CRED_ALLOW fd closed\n");
+ /* exit with non-zero status propagates to parent's failure */
+ ksft_finished();
+ return 0;
+ }
+
+ ksft_set_plan(7);
+
+ fds[FD_NORM] = open("/proc/self/exe", O_RDONLY);
+ fds[FD_CE] = open("/proc/self/exe", O_RDONLY | O_CLOEXEC);
+ fds[FD_CA] = open("/proc/self/exe", O_RDONLY | O_CRED_ALLOW);
+ ksft_test_result(FD_OK(FD_NORM), "regular open\n");
+ ksft_test_result(FD_OK(FD_CE), "O_CLOEXEC open\n");
+ ksft_test_result(FD_OK(FD_CA), "O_CRED_ALLOW open\n");
+
+ err = socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, socket_fds);
+ if (err) {
+ ksft_perror("socketpair() failed");
+ ksft_exit_fail_msg("socketpair\n");
+ return 1;
+ }
+ err = send_fd(socket_fds[0], fds[FD_NORM]);
+ ksft_test_result(err == 0, "normal fd sent\n");
+ err = send_fd(socket_fds[0], fds[FD_CE]);
+ ksft_test_result(err == 0, "O_CLOEXEC fd sent\n");
+ err = send_fd(socket_fds[0], fds[FD_CA]);
+ ksft_test_result(err == -EPERM, "O_CRED_ALLOW fd not sent, EPERM\n");
+ close(socket_fds[0]);
+ close(socket_fds[1]);
+
+ pid = fork();
+ if (pid < 0) {
+ ksft_perror("fork() failed");
+ ksft_exit_fail_msg("fork\n");
+ return 1;
+ }
+
+ if (pid == 0) {
+ char *cargv[] = {"cred_allow", "--child", NULL};
+
+ execve("/proc/self/exe", cargv, env);
+ ksft_perror("execve() failed");
+ ksft_exit_fail_msg("execve\n");
+ return 1;
+ }
+
+ if (waitpid(pid, &status, 0) != pid) {
+ ksft_perror("waitpid() failed");
+ ksft_exit_fail_msg("waitpid\n");
+ return 1;
+ }
+ ksft_print_msg("back to parent\n");
+
+ ksft_test_result(status == 0, "child success\n");
+
+ ksft_finished();
+ return 0;
+}
--
2.44.0
^ permalink raw reply related
* [PATCH v6 3/3] openat2: add OA2_CRED_INHERIT flag
From: Stas Sergeev @ 2024-04-27 11:24 UTC (permalink / raw)
To: linux-kernel
Cc: Stas Sergeev, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Christian Brauner, Jan Kara, Jeff Layton,
Chuck Lever, Alexander Aring, David Laight, linux-fsdevel,
linux-api, Paolo Bonzini, Christian Göttsche
In-Reply-To: <20240427112451.1609471-1-stsp2@yandex.ru>
This flag performs the open operation with the fs credentials
(fsuid, fsgid, group_info) that were in effect when dir_fd was opened.
dir_fd must be opened with O_CRED_ALLOW, or EPERM is returned.
Selftests are added to check for these properties as well as for
the invalid flag combinations.
This allows the process to pre-open some directories and then
change eUID (and all other UIDs/GIDs) to a less-privileged user,
retaining the ability to open/create files within these directories.
Design goal:
The idea is to provide a very light-weight sandboxing, where the
process, without the use of any heavy-weight techniques like chroot
within namespaces, can restrict the access to the set of pre-opened
directories.
This patch is just a first step to such sandboxing. If things go
well, in the future the same extension can be added to more syscalls.
These should include at least unlinkat(), renameat2() and the
not-yet-upstreamed setxattrat().
Security considerations:
- Only the bare minimal set of credentials is overridden:
fsuid, fsgid and group_info. The rest, for example capabilities,
are not overridden to avoid unneeded security risks.
- To avoid sandboxing escape, this patch makes sure the restricted
lookup modes are used. Namely, RESOLVE_BENEATH or RESOLVE_IN_ROOT.
- Magic /proc symlinks are discarded, as suggested by
Andy Lutomirski <luto@kernel.org>
- O_CRED_ALLOW fds cannot be passed via unix socket and are always
closed on exec() to prevent "unsuspecting userspace" from not being
able to fully drop privs.
Use cases:
Virtual machines that deal with untrusted code, can use that
instead of a more heavy-weighted approaches.
Currently the approach is being tested on a dosemu2 VM.
Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
CC: Stefan Metzmacher <metze@samba.org>
CC: Eric Biederman <ebiederm@xmission.com>
CC: Alexander Viro <viro@zeniv.linux.org.uk>
CC: Andy Lutomirski <luto@kernel.org>
CC: Christian Brauner <brauner@kernel.org>
CC: Jan Kara <jack@suse.cz>
CC: Jeff Layton <jlayton@kernel.org>
CC: Chuck Lever <chuck.lever@oracle.com>
CC: Alexander Aring <alex.aring@gmail.com>
CC: linux-fsdevel@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Christian Göttsche <cgzones@googlemail.com>
---
fs/fcntl.c | 2 +
fs/namei.c | 56 +++++++++-
fs/open.c | 10 +-
include/linux/fcntl.h | 2 +
include/uapi/linux/openat2.h | 2 +
tools/testing/selftests/openat2/Makefile | 2 +-
.../testing/selftests/openat2/cred_inherit.c | 105 ++++++++++++++++++
.../testing/selftests/openat2/openat2_test.c | 12 +-
8 files changed, 186 insertions(+), 5 deletions(-)
create mode 100644 tools/testing/selftests/openat2/cred_inherit.c
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 78c96b1293c2..283c2e65fc2c 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -1043,6 +1043,8 @@ static int __init fcntl_init(void)
HWEIGHT32(
(VALID_OPEN_FLAGS & ~(O_NONBLOCK | O_NDELAY)) |
__FMODE_EXEC | __FMODE_NONOTIFY));
+ BUILD_BUG_ON(HWEIGHT32(VALID_OPENAT2_FLAGS) !=
+ HWEIGHT32(VALID_OPEN_FLAGS) + 1);
fasync_cache = kmem_cache_create("fasync_cache",
sizeof(struct fasync_struct), 0,
diff --git a/fs/namei.c b/fs/namei.c
index dd50345f7260..aa5dcf57851b 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3776,6 +3776,43 @@ static int do_o_path(struct nameidata *nd, unsigned flags, struct file *file)
return error;
}
+static const struct cred *openat2_init_creds(int dfd)
+{
+ struct cred *cred;
+ struct fd f;
+
+ if (dfd == AT_FDCWD)
+ return ERR_PTR(-EINVAL);
+
+ f = fdget_raw(dfd);
+ if (!f.file)
+ return ERR_PTR(-EBADF);
+
+ cred = ERR_PTR(-EPERM);
+ if (!(f.file->f_flags & O_CRED_ALLOW))
+ goto done;
+
+ cred = prepare_creds();
+ if (!cred) {
+ cred = ERR_PTR(-ENOMEM);
+ goto done;
+ }
+
+ cred->fsuid = f.file->f_cred->fsuid;
+ cred->fsgid = f.file->f_cred->fsgid;
+ cred->group_info = get_group_info(f.file->f_cred->group_info);
+
+done:
+ fdput(f);
+ return cred;
+}
+
+static void openat2_done_creds(const struct cred *cred)
+{
+ put_group_info(cred->group_info);
+ put_cred(cred);
+}
+
static struct file *path_openat(struct nameidata *nd,
const struct open_flags *op, unsigned flags)
{
@@ -3793,18 +3830,33 @@ static struct file *path_openat(struct nameidata *nd,
error = do_o_path(nd, flags, file);
} else {
const char *s;
+ const struct cred *old_cred = NULL, *cred = NULL;
- file = alloc_empty_file(open_flags, current_cred());
- if (IS_ERR(file))
+ if (open_flags & OA2_CRED_INHERIT) {
+ cred = openat2_init_creds(nd->dfd);
+ if (IS_ERR(cred))
+ return ERR_CAST(cred);
+ }
+ file = alloc_empty_file(open_flags, cred ?: current_cred());
+ if (IS_ERR(file)) {
+ if (cred)
+ openat2_done_creds(cred);
return file;
+ }
s = path_init(nd, flags);
+ if (cred)
+ old_cred = override_creds(cred);
while (!(error = link_path_walk(s, nd)) &&
(s = open_last_lookups(nd, file, op)) != NULL)
;
if (!error)
error = do_open(nd, file, op);
+ if (old_cred)
+ revert_creds(old_cred);
terminate_walk(nd);
+ if (cred)
+ openat2_done_creds(cred);
}
if (likely(!error)) {
if (likely(file->f_mode & FMODE_OPENED))
diff --git a/fs/open.c b/fs/open.c
index ee8460c83c77..dd4fab536135 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1225,7 +1225,7 @@ inline int build_open_flags(const struct open_how *how, struct open_flags *op)
* values before calling build_open_flags(), but openat2(2) checks all
* of its arguments.
*/
- if (flags & ~VALID_OPEN_FLAGS)
+ if (flags & ~VALID_OPENAT2_FLAGS)
return -EINVAL;
if (how->resolve & ~VALID_RESOLVE_FLAGS)
return -EINVAL;
@@ -1326,6 +1326,14 @@ inline int build_open_flags(const struct open_how *how, struct open_flags *op)
lookup_flags |= LOOKUP_CACHED;
}
+ if (flags & OA2_CRED_INHERIT) {
+ /* Inherit creds only with scoped look-up modes. */
+ if (!(lookup_flags & LOOKUP_IS_SCOPED))
+ return -EPERM;
+ /* Reject /proc "magic" links if inheriting creds. */
+ lookup_flags |= LOOKUP_NO_MAGICLINKS;
+ }
+
op->lookup_flags = lookup_flags;
return 0;
}
diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
index e074ee9c1e36..33b9c7ad056b 100644
--- a/include/linux/fcntl.h
+++ b/include/linux/fcntl.h
@@ -12,6 +12,8 @@
FASYNC | O_DIRECT | O_LARGEFILE | O_DIRECTORY | O_NOFOLLOW | \
O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE | O_CRED_ALLOW)
+#define VALID_OPENAT2_FLAGS (VALID_OPEN_FLAGS | OA2_CRED_INHERIT)
+
/* List of all valid flags for the how->resolve argument: */
#define VALID_RESOLVE_FLAGS \
(RESOLVE_NO_XDEV | RESOLVE_NO_MAGICLINKS | RESOLVE_NO_SYMLINKS | \
diff --git a/include/uapi/linux/openat2.h b/include/uapi/linux/openat2.h
index a5feb7604948..f803558ad62f 100644
--- a/include/uapi/linux/openat2.h
+++ b/include/uapi/linux/openat2.h
@@ -40,4 +40,6 @@ struct open_how {
return -EAGAIN if that's not
possible. */
+#define OA2_CRED_INHERIT (1UL << 28)
+
#endif /* _UAPI_LINUX_OPENAT2_H */
diff --git a/tools/testing/selftests/openat2/Makefile b/tools/testing/selftests/openat2/Makefile
index 254d676a2689..a1f4b5395f82 100644
--- a/tools/testing/selftests/openat2/Makefile
+++ b/tools/testing/selftests/openat2/Makefile
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: GPL-2.0-or-later
CFLAGS += -Wall -O2 -g -fsanitize=address -fsanitize=undefined -static-libasan
-TEST_GEN_PROGS := openat2_test resolve_test rename_attack_test
+TEST_GEN_PROGS := openat2_test resolve_test rename_attack_test cred_inherit
include ../lib.mk
diff --git a/tools/testing/selftests/openat2/cred_inherit.c b/tools/testing/selftests/openat2/cred_inherit.c
new file mode 100644
index 000000000000..550a06763ac7
--- /dev/null
+++ b/tools/testing/selftests/openat2/cred_inherit.c
@@ -0,0 +1,105 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <errno.h>
+#include <fcntl.h>
+#include <sched.h>
+#include <stdio.h>
+#include <stdbool.h>
+#include <sys/stat.h>
+#include <sys/syscall.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <sys/socket.h>
+#include <time.h>
+#include <unistd.h>
+#include <string.h>
+
+#include "../kselftest.h"
+#include "helpers.h"
+
+#ifndef O_CRED_ALLOW
+#define O_CRED_ALLOW 0x2000000
+#endif
+
+#ifndef OA2_CRED_INHERIT
+#define OA2_CRED_INHERIT (1UL << 28)
+#endif
+
+enum { FD_NORM, FD_NCA, FD_DIR, FD_DCA, FD_MAX };
+
+int main(int argc, char *argv[], char *env[])
+{
+ struct open_how how1 = {
+ .flags = O_RDONLY,
+ .resolve = RESOLVE_BENEATH,
+ };
+ struct open_how how2 = {
+ .flags = O_RDONLY | OA2_CRED_INHERIT,
+ .resolve = RESOLVE_BENEATH,
+ };
+ int size = sizeof(struct open_how);
+ int i;
+ int fd;
+ int err;
+ int fds[FD_MAX];
+#define NFD(n) ((n) + 3)
+#define FD_OK(n) (NFD(n) == fds[n])
+
+ if (!openat2_supported) {
+ ksft_print_msg("openat2(2) unsupported\n");
+ return 0;
+ }
+
+ ksft_set_plan(14);
+
+ fds[FD_NORM] = open("/proc/self/maps", O_RDONLY);
+ fds[FD_NCA] = open("/proc/self/maps", O_RDONLY | O_CRED_ALLOW);
+ fds[FD_DIR] = open("/proc/self", O_RDONLY | O_DIRECTORY);
+ fds[FD_DCA] = open("/proc/self", O_RDONLY | O_DIRECTORY | O_CRED_ALLOW);
+ ksft_test_result(FD_OK(FD_NORM), "file open\n");
+ ksft_test_result(FD_OK(FD_NCA), "file open with O_CRED_ALLOW\n");
+ ksft_test_result(FD_OK(FD_DIR), "directory open\n");
+ ksft_test_result(FD_OK(FD_DCA), "directory open with O_CRED_ALLOW\n");
+
+ err = fchdir(fds[FD_DIR]);
+ if (err) {
+ ksft_perror("fchdir() failed");
+ ksft_exit_fail_msg("fchdir\n");
+ return 1;
+ }
+ fd = syscall(SYS_openat2, AT_FDCWD, "maps", &how1, size);
+ ksft_test_result(fd != -1, "AT_FDCWD success\n");
+ close(fd);
+ /* OA2_CRED_INHERIT fails with AT_FDCWD */
+ fd = syscall(SYS_openat2, AT_FDCWD, "maps", &how2, size);
+ ksft_test_result(fd == -1 && errno == EINVAL, "AT_FDCWD EINVAL\n");
+
+ fd = syscall(SYS_openat2, fds[FD_NORM], "maps", &how1, size);
+ ksft_test_result(fd == -1 && errno == ENOTDIR, "regilar file ENOTDIR\n");
+ /* No O_CRED_ALLOW -> EPERM */
+ fd = syscall(SYS_openat2, fds[FD_NORM], "maps", &how2, size);
+ ksft_test_result(fd == -1 && errno == EPERM, "regilar file EPERM\n");
+
+ fd = syscall(SYS_openat2, fds[FD_NCA], "maps", &how1, size);
+ ksft_test_result(fd == -1 && errno == ENOTDIR, "regilar file ENOTDIR\n");
+ fd = syscall(SYS_openat2, fds[FD_NCA], "maps", &how2, size);
+ ksft_test_result(fd == -1 && errno == ENOTDIR, "regilar file ENOTDIR\n");
+
+ fd = syscall(SYS_openat2, fds[FD_DIR], "maps", &how1, size);
+ ksft_test_result(fd != -1, "dir fd success\n");
+ close(fd);
+ /* No O_CRED_ALLOW -> EPERM */
+ fd = syscall(SYS_openat2, fds[FD_DIR], "maps", &how2, size);
+ ksft_test_result(fd == -1 && errno == EPERM, "dir fd EPERM\n");
+
+ fd = syscall(SYS_openat2, fds[FD_DCA], "maps", &how1, size);
+ ksft_test_result(fd != -1, "dir O_CRED_ALLOW fd success\n");
+ close(fd);
+ fd = syscall(SYS_openat2, fds[FD_DCA], "maps", &how2, size);
+ ksft_test_result(fd != -1, "dir O_CRED_ALLOW fd O_CRED_INHERIT success\n");
+ close(fd);
+
+ for (i = 0; i < FD_MAX; i++)
+ close(fds[i]);
+ ksft_finished();
+ return 0;
+}
diff --git a/tools/testing/selftests/openat2/openat2_test.c b/tools/testing/selftests/openat2/openat2_test.c
index 9024754530b2..5095288fe1ac 100644
--- a/tools/testing/selftests/openat2/openat2_test.c
+++ b/tools/testing/selftests/openat2/openat2_test.c
@@ -28,6 +28,10 @@
#define O_LARGEFILE 0x8000
#endif
+#ifndef OA2_CRED_INHERIT
+#define OA2_CRED_INHERIT (1UL << 28)
+#endif
+
struct open_how_ext {
struct open_how inner;
uint32_t extra1;
@@ -159,7 +163,7 @@ struct flag_test {
int err;
};
-#define NUM_OPENAT2_FLAG_TESTS 25
+#define NUM_OPENAT2_FLAG_TESTS 27
void test_openat2_flags(void)
{
@@ -233,6 +237,12 @@ void test_openat2_flags(void)
{ .name = "invalid how.resolve and O_PATH",
.how.flags = O_PATH,
.how.resolve = 0x1337, .err = -EINVAL },
+ { .name = "invalid how.resolve and OA2_CRED_INHERIT",
+ .how.flags = OA2_CRED_INHERIT,
+ .how.resolve = 0, .err = -EPERM },
+ { .name = "invalid AT_FDCWD and OA2_CRED_INHERIT",
+ .how.flags = OA2_CRED_INHERIT,
+ .how.resolve = 0x08, .err = -EINVAL },
/* currently unknown upper 32 bit rejected. */
{ .name = "currently unknown bit (1 << 63)",
--
2.44.0
^ permalink raw reply related
* [PATCH v6 1/3] fs: reorganize path_openat()
From: Stas Sergeev @ 2024-04-27 11:24 UTC (permalink / raw)
To: linux-kernel
Cc: Stas Sergeev, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Christian Brauner, Jan Kara, Jeff Layton,
Chuck Lever, Alexander Aring, David Laight, linux-fsdevel,
linux-api, Paolo Bonzini, Christian Göttsche
In-Reply-To: <20240427112451.1609471-1-stsp2@yandex.ru>
This patch moves the call to alloc_empty_file() down the if branches.
That changes is needed for the next patch, which adds a cred override
for alloc_empty_file(). The cred override is only needed in one branch,
i.e. it is not needed for O_PATH and O_TMPFILE..
No functional changes are intended by that patch.
Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
CC: Eric Biederman <ebiederm@xmission.com>
CC: Alexander Viro <viro@zeniv.linux.org.uk>
CC: Christian Brauner <brauner@kernel.org>
CC: Jan Kara <jack@suse.cz>
CC: Andy Lutomirski <luto@kernel.org>
CC: David Laight <David.Laight@ACULAB.COM>
CC: linux-fsdevel@vger.kernel.org
CC: linux-kernel@vger.kernel.org
---
fs/namei.c | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index c5b2a25be7d0..dd50345f7260 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3781,17 +3781,24 @@ static struct file *path_openat(struct nameidata *nd,
{
struct file *file;
int error;
+ int open_flags = op->open_flag;
- file = alloc_empty_file(op->open_flag, current_cred());
- if (IS_ERR(file))
- return file;
-
- if (unlikely(file->f_flags & __O_TMPFILE)) {
- error = do_tmpfile(nd, flags, op, file);
- } else if (unlikely(file->f_flags & O_PATH)) {
- error = do_o_path(nd, flags, file);
+ if (unlikely(open_flags & (__O_TMPFILE | O_PATH))) {
+ file = alloc_empty_file(open_flags, current_cred());
+ if (IS_ERR(file))
+ return file;
+ if (open_flags & __O_TMPFILE)
+ error = do_tmpfile(nd, flags, op, file);
+ else
+ error = do_o_path(nd, flags, file);
} else {
- const char *s = path_init(nd, flags);
+ const char *s;
+
+ file = alloc_empty_file(open_flags, current_cred());
+ if (IS_ERR(file))
+ return file;
+
+ s = path_init(nd, flags);
while (!(error = link_path_walk(s, nd)) &&
(s = open_last_lookups(nd, file, op)) != NULL)
;
--
2.44.0
^ permalink raw reply related
* Re: RFC: Restricting userspace interfaces for CXL fabric management
From: Greg KH @ 2024-04-27 11:12 UTC (permalink / raw)
To: Harold Johnson
Cc: Jonathan Cameron, Dan Williams, linux-cxl, Sreenivas Bagalkote,
Brett Henning, Sumanesh Samanta, linux-kernel, Davidlohr Bueso,
Dave Jiang, Alison Schofield, Vishal Verma, Ira Weiny, linuxarm,
linux-api, Lorenzo Pieralisi, Natu, Mahesh
In-Reply-To: <6351024b5d6206c4e9a8cd98d1a09d43@mail.gmail.com>
On Fri, Apr 26, 2024 at 02:25:29PM -0500, Harold Johnson wrote:
> A few examples:
> a) Temperature monitoring of a component or internal chip die
> temperatures. Could CXL define a standard OpCode to gather temperatures,
> yes it could; but is this really part of CXL? Then how many temperature
> elements and what does each element mean? This enters into the
> implementation and therefore is vendor specific. Unless the CXL spec
> starts to define the implementation, something along the lines of "thou
> shall have an average die temperature, rather than specific temperatures
> across a die", etc.
>
> b) Error counters, metrics, internal counters, etc. Could CXL define a
> set of common error counters, absolutely. PCIe has done some of this.
> However, a specific implementation may have counters and error reporting
> that are meaningful only to a specific design and a specific
> implementation rather than a "least common denominator" approach of a
> standard body.
>
> c) Performance counters, metric, indicators, etc. Performance can be very
> implementation specific and tweaking performance is likely to be
> implementation specific. Yes, generic and a least common denominator
> elements could be created, but are likely to limiting in realizing the
> maximum performance of an implementation.
>
> d) Logs, errors and debug information. In addition to spec defined
> logging of CXL topology errors, specific designs will have logs, crash
> dumps, debug data that is very specific to a implementation. There are
> likely to be cases where a product that conforms to a specification like
> CXL, may have features that don't directly have anything to do with CXL,
> but where a standards based management interface can be used to configure,
> manage, and collect data for a non-CXL feature.
All of the above should be able to be handled by vendor-specific KERNEL
drivers that feed the needed information to the proper user/kernel apis
that the kernel already provides.
So while innovating at the hardware level is fine, follow the ways that
everyone has done this for other specification types (USB, PCI, etc.)
and just allow vendor drivers to provide the information. Don't do this
in crazy userspace drivers which will circumvent the whole reason we
have standard kernel/user apis in the first place for these types of
things.
> e) Innovation. I believe that innovation should be encouraged. There may
> be designs that support CXL, but that also incorporate unique and
> innovative features or functions that might service a niche market. The
> AI space is ripe for innovation and perhaps specialized features that may
> not make sense for the overall CXL specification.
>
> I think that in most cases Vendor specific opcodes are not used to
> circumvent the standards, but are used when the standards group has no
> interested in driving into the standard certain features that are clearly
> either implementation specific or are vendor specific additions that have
> a specific appeal to a select class of customer, but yet are not relevant
> to a specific standard.
Then fight this out in the specification groups, which are highly
political, and do not push that into the kernel space please. Again,
this is nothing new, we have all done this for specs for decades now,
allow vendor additions to the spec and handle that in the kernel and all
should be ok, right?
Or am I missing something obvious here where we would NOT want to do
what all other specs have done?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v5 2/3] open: add O_CRED_ALLOW flag
From: kernel test robot @ 2024-04-27 2:12 UTC (permalink / raw)
To: Stas Sergeev, linux-kernel
Cc: oe-kbuild-all, Stas Sergeev, Stefan Metzmacher, Eric Biederman,
Alexander Viro, Andy Lutomirski, Christian Brauner, Jan Kara,
Jeff Layton, Chuck Lever, Alexander Aring, David Laight,
linux-fsdevel, linux-api, Paolo Bonzini, Christian Göttsche,
Arnd Bergmann, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jens Axboe, Kuniyuki Iwashima, Pavel Begunkov, linux-arch, netdev
In-Reply-To: <20240426133310.1159976-3-stsp2@yandex.ru>
Hi Stas,
kernel test robot noticed the following build errors:
[auto build test ERROR on linus/master]
[also build test ERROR on v6.9-rc5 next-20240426]
[cannot apply to arnd-asm-generic/master]
[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#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Stas-Sergeev/fs-reorganize-path_openat/20240426-214030
base: linus/master
patch link: https://lore.kernel.org/r/20240426133310.1159976-3-stsp2%40yandex.ru
patch subject: [PATCH v5 2/3] open: add O_CRED_ALLOW flag
config: parisc-allnoconfig (https://download.01.org/0day-ci/archive/20240427/202404270923.bAeBIJt1-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240427/202404270923.bAeBIJt1-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202404270923.bAeBIJt1-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from <command-line>:
fs/fcntl.c: In function 'fcntl_init':
>> include/linux/compiler_types.h:449:45: error: call to '__compiletime_assert_297' declared with attribute error: BUILD_BUG_ON failed: 22 - 1 != HWEIGHT32( (VALID_OPEN_FLAGS & ~(O_NONBLOCK | O_NDELAY)) | __FMODE_EXEC | __FMODE_NONOTIFY)
449 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^
include/linux/compiler_types.h:430:25: note: in definition of macro '__compiletime_assert'
430 | prefix ## suffix(); \
| ^~~~~~
include/linux/compiler_types.h:449:9: note: in expansion of macro '_compiletime_assert'
449 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
| ^~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:50:9: note: in expansion of macro 'BUILD_BUG_ON_MSG'
50 | BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
| ^~~~~~~~~~~~~~~~
fs/fcntl.c:1042:9: note: in expansion of macro 'BUILD_BUG_ON'
1042 | BUILD_BUG_ON(22 - 1 /* for O_RDONLY being 0 */ !=
| ^~~~~~~~~~~~
vim +/__compiletime_assert_297 +449 include/linux/compiler_types.h
eb5c2d4b45e3d2 Will Deacon 2020-07-21 435
eb5c2d4b45e3d2 Will Deacon 2020-07-21 436 #define _compiletime_assert(condition, msg, prefix, suffix) \
eb5c2d4b45e3d2 Will Deacon 2020-07-21 437 __compiletime_assert(condition, msg, prefix, suffix)
eb5c2d4b45e3d2 Will Deacon 2020-07-21 438
eb5c2d4b45e3d2 Will Deacon 2020-07-21 439 /**
eb5c2d4b45e3d2 Will Deacon 2020-07-21 440 * compiletime_assert - break build and emit msg if condition is false
eb5c2d4b45e3d2 Will Deacon 2020-07-21 441 * @condition: a compile-time constant condition to check
eb5c2d4b45e3d2 Will Deacon 2020-07-21 442 * @msg: a message to emit if condition is false
eb5c2d4b45e3d2 Will Deacon 2020-07-21 443 *
eb5c2d4b45e3d2 Will Deacon 2020-07-21 444 * In tradition of POSIX assert, this macro will break the build if the
eb5c2d4b45e3d2 Will Deacon 2020-07-21 445 * supplied condition is *false*, emitting the supplied error message if the
eb5c2d4b45e3d2 Will Deacon 2020-07-21 446 * compiler has support to do so.
eb5c2d4b45e3d2 Will Deacon 2020-07-21 447 */
eb5c2d4b45e3d2 Will Deacon 2020-07-21 448 #define compiletime_assert(condition, msg) \
eb5c2d4b45e3d2 Will Deacon 2020-07-21 @449 _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
eb5c2d4b45e3d2 Will Deacon 2020-07-21 450
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* RE: RFC: Restricting userspace interfaces for CXL fabric management
From: Harold Johnson @ 2024-04-26 19:25 UTC (permalink / raw)
To: Jonathan Cameron, Dan Williams
Cc: linux-cxl, Sreenivas Bagalkote, Brett Henning, Sumanesh Samanta,
linux-kernel, Davidlohr Bueso, Dave Jiang, Alison Schofield,
Vishal Verma, Ira Weiny, linuxarm, linux-api, Lorenzo Pieralisi,
Natu, Mahesh, gregkh
In-Reply-To: <20240426175341.00002e67@Huawei.com>
[-- Attachment #1: Type: text/plain, Size: 7740 bytes --]
Perhaps a bit more color on a few specifics might be helpful.
I think that there will always be a class of vendor specific APIs/Opcodes
that are related to an implementation of a standard instead of the
standard itself. I've been party to discussion on not creating CXL
defined API/Opcodes that get into the realm of specifying an
implementation. There are also a class of data that can be collected from
a specific implementation that is helpful for debug, for health
monitoring, and perhaps performance monitoring where the implementation
matters and therefore are not easily abstracted to a standard.
A few examples:
a) Temperature monitoring of a component or internal chip die
temperatures. Could CXL define a standard OpCode to gather temperatures,
yes it could; but is this really part of CXL? Then how many temperature
elements and what does each element mean? This enters into the
implementation and therefore is vendor specific. Unless the CXL spec
starts to define the implementation, something along the lines of "thou
shall have an average die temperature, rather than specific temperatures
across a die", etc.
b) Error counters, metrics, internal counters, etc. Could CXL define a
set of common error counters, absolutely. PCIe has done some of this.
However, a specific implementation may have counters and error reporting
that are meaningful only to a specific design and a specific
implementation rather than a "least common denominator" approach of a
standard body.
c) Performance counters, metric, indicators, etc. Performance can be very
implementation specific and tweaking performance is likely to be
implementation specific. Yes, generic and a least common denominator
elements could be created, but are likely to limiting in realizing the
maximum performance of an implementation.
d) Logs, errors and debug information. In addition to spec defined
logging of CXL topology errors, specific designs will have logs, crash
dumps, debug data that is very specific to a implementation. There are
likely to be cases where a product that conforms to a specification like
CXL, may have features that don't directly have anything to do with CXL,
but where a standards based management interface can be used to configure,
manage, and collect data for a non-CXL feature.
e) Innovation. I believe that innovation should be encouraged. There may
be designs that support CXL, but that also incorporate unique and
innovative features or functions that might service a niche market. The
AI space is ripe for innovation and perhaps specialized features that may
not make sense for the overall CXL specification.
I think that in most cases Vendor specific opcodes are not used to
circumvent the standards, but are used when the standards group has no
interested in driving into the standard certain features that are clearly
either implementation specific or are vendor specific additions that have
a specific appeal to a select class of customer, but yet are not relevant
to a specific standard.
At the end of the day, customer want products that solve a specific
problem. Sometimes vendor can address market segments or niches that a
standard group has no interest in supporting. It can also take months,
and in some cases years to reach an agreement on what standardized feature
should look like. I also believe that there can be competitive reasons
why there might be a group that wants to slow down a vendor's
implementation for fear of losing market share.
Thanks
Harold Johnson
-----Original Message-----
From: Jonathan Cameron [mailto:Jonathan.Cameron@Huawei.com]
Sent: Friday, April 26, 2024 11:54 AM
To: Dan Williams
Cc: linux-cxl@vger.kernel.org; Sreenivas Bagalkote; Brett Henning; Harold
Johnson; Sumanesh Samanta; linux-kernel@vger.kernel.org; Davidlohr Bueso;
Dave Jiang; Alison Schofield; Vishal Verma; Ira Weiny;
linuxarm@huawei.com; linux-api@vger.kernel.org; Lorenzo Pieralisi; Natu,
Mahesh; gregkh@linuxfoundation.org
Subject: Re: RFC: Restricting userspace interfaces for CXL fabric
management
On Fri, 26 Apr 2024 09:16:44 -0700
Dan Williams <dan.j.williams@intel.com> wrote:
> Jonathan Cameron wrote:
> [..]
> > To give people an incentive to play the standards game we have to
> > provide an alternative. Userspace libraries will provide some
incentive
> > to standardize if we have enough vendors (we don't today - so they
will
> > do their own libraries), but it is a lot easier to encourage if we
> > exercise control over the interface.
>
> Yes, and I expect you and I are not far off on what can be done
> here.
>
> However, lets cut to a sentiment hanging over this discussion. Referring
> to vendor specific commands:
>
> "CXL spec has them for a reason and they need to be supported."
>
> ...that is an aggressive "vendor specific first" sentiment that
> generates an aggressive "userspace drivers" reaction, because the best
> way to get around community discussions about what ABI makes sense is
> userspace drivers.
>
> Now, if we can step back to where this discussion started, where typical
> Linux collaboration shines, and where I think you and I are more aligned
> than this thread would indicate, is "vendor specific last". Lets
> carefully consider the vendor specific commands that are candidates to
> be de facto cross vendor semantics if not de jure standards.
>
Agreed. I'd go a little further and say I generally have much more warm
and
fuzzy feelings when what is a vendor defined command (today) maps to more
or less the same bit of code for a proposed standards ECN.
IP rules prevent us commenting on specific proposals, but there will be
things we review quicker and with a lighter touch vs others where we
ask lots of annoying questions about generality of the feature etc.
Given the effort we are putting in on the kernel side we all want CXL
to succeed and will do our best to encourage activities that make that
more likely. There are other standards bodies available... which may
make more sense for some features.
Command interfaces are not a good place to compete and maintain secrecy.
If vendors want to do that, then they don't get the pony of upstream
support. They get to convince distros to do a custom kernel build for
them:
Good luck with that, some of those folk are 'blunt' in their responses to
such requests.
My proposal is we go forward with a bunch of the CXL spec defined commands
to show the 'how' and consider specific proposals for upstream support
of vendor defined commands on a case by case basis (so pretty much
what you say above). Maybe after a few are done we can formalize some
rules of thumb help vendors makes such proposals, though maybe some
will figure out it is a better and longer term solution to do 'standards
first development'.
I think we do need to look at the safety filtering of tunneled
commands but don't see that as a particularly tricky addition -
for the simple non destructive commands at least.
Jonathan
--
This electronic communication and the information and any files transmitted
with it, or attached to it, are confidential and are intended solely for
the use of the individual or entity to whom it is addressed and may contain
information that is confidential, legally privileged, protected by privacy
laws, or otherwise restricted from disclosure to anyone else. If you are
not the intended recipient or the person responsible for delivering the
e-mail to the intended recipient, you are hereby notified that any use,
copying, distributing, dissemination, forwarding, printing, or copying of
this e-mail is strictly prohibited. If you received this e-mail in error,
please return the e-mail to the sender, delete it from your computer, and
destroy any printed copy of it.
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4215 bytes --]
^ permalink raw reply
* Re: [PATCHv3 bpf-next 6/7] selftests/bpf: Add uretprobe compat test
From: Andrii Nakryiko @ 2024-04-26 18:06 UTC (permalink / raw)
To: Jiri Olsa
Cc: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
linux-kernel, linux-trace-kernel, linux-api, x86, bpf, Song Liu,
Yonghong Song, John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov (AMD), Ingo Molnar, Andy Lutomirski
In-Reply-To: <20240421194206.1010934-7-jolsa@kernel.org>
On Sun, Apr 21, 2024 at 12:43 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding test that adds return uprobe inside 32 bit task
> and verify the return uprobe and attached bpf programs
> get properly executed.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> tools/testing/selftests/bpf/.gitignore | 1 +
> tools/testing/selftests/bpf/Makefile | 6 ++-
> .../selftests/bpf/prog_tests/uprobe_syscall.c | 40 +++++++++++++++++++
> .../bpf/progs/uprobe_syscall_compat.c | 13 ++++++
> 4 files changed, 59 insertions(+), 1 deletion(-)
> create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscall_compat.c
>
> diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
> index f1aebabfb017..69d71223c0dd 100644
> --- a/tools/testing/selftests/bpf/.gitignore
> +++ b/tools/testing/selftests/bpf/.gitignore
> @@ -45,6 +45,7 @@ test_cpp
> /veristat
> /sign-file
> /uprobe_multi
> +/uprobe_compat
> *.ko
> *.tmp
> xskxceiver
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index edc73f8f5aef..d170b63eca62 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -134,7 +134,7 @@ TEST_GEN_PROGS_EXTENDED = test_sock_addr test_skb_cgroup_id_user \
> xskxceiver xdp_redirect_multi xdp_synproxy veristat xdp_hw_metadata \
> xdp_features bpf_test_no_cfi.ko
>
> -TEST_GEN_FILES += liburandom_read.so urandom_read sign-file uprobe_multi
> +TEST_GEN_FILES += liburandom_read.so urandom_read sign-file uprobe_multi uprobe_compat
you need to add uprobe_compat to TRUNNER_EXTRA_FILES as well, no?
>
> # Emit succinct information message describing current building step
> # $1 - generic step name (e.g., CC, LINK, etc);
> @@ -761,6 +761,10 @@ $(OUTPUT)/uprobe_multi: uprobe_multi.c
> $(call msg,BINARY,,$@)
> $(Q)$(CC) $(CFLAGS) -O0 $(LDFLAGS) $^ $(LDLIBS) -o $@
>
> +$(OUTPUT)/uprobe_compat:
> + $(call msg,BINARY,,$@)
> + $(Q)echo "int main() { return 0; }" | $(CC) $(CFLAGS) -xc -m32 -O0 - -o $@
> +
> EXTRA_CLEAN := $(SCRATCH_DIR) $(HOST_SCRATCH_DIR) \
> prog_tests/tests.h map_tests/tests.h verifier/tests.h \
> feature bpftool \
> diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> index 9233210a4c33..3770254d893b 100644
> --- a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> @@ -11,6 +11,7 @@
> #include <sys/wait.h>
> #include "uprobe_syscall.skel.h"
> #include "uprobe_syscall_call.skel.h"
> +#include "uprobe_syscall_compat.skel.h"
>
> __naked unsigned long uretprobe_regs_trigger(void)
> {
> @@ -291,6 +292,35 @@ static void test_uretprobe_syscall_call(void)
> "read_trace_pipe_iter");
> ASSERT_EQ(found, 0, "found");
> }
> +
> +static void trace_pipe_compat_cb(const char *str, void *data)
> +{
> + if (strstr(str, "uretprobe compat") != NULL)
> + (*(int *)data)++;
> +}
> +
> +static void test_uretprobe_compat(void)
> +{
> + struct uprobe_syscall_compat *skel = NULL;
> + int err, found = 0;
> +
> + skel = uprobe_syscall_compat__open_and_load();
> + if (!ASSERT_OK_PTR(skel, "uprobe_syscall_compat__open_and_load"))
> + goto cleanup;
> +
> + err = uprobe_syscall_compat__attach(skel);
> + if (!ASSERT_OK(err, "uprobe_syscall_compat__attach"))
> + goto cleanup;
> +
> + system("./uprobe_compat");
> +
> + ASSERT_OK(read_trace_pipe_iter(trace_pipe_compat_cb, &found, 1000),
> + "read_trace_pipe_iter");
why so complicated? can't you just set global variable that it was called
> + ASSERT_EQ(found, 1, "found");
> +
> +cleanup:
> + uprobe_syscall_compat__destroy(skel);
> +}
> #else
> static void test_uretprobe_regs_equal(void)
> {
> @@ -306,6 +336,11 @@ static void test_uretprobe_syscall_call(void)
> {
> test__skip();
> }
> +
> +static void test_uretprobe_compat(void)
> +{
> + test__skip();
> +}
> #endif
>
> void test_uprobe_syscall(void)
> @@ -320,3 +355,8 @@ void serial_test_uprobe_syscall_call(void)
> {
> test_uretprobe_syscall_call();
> }
> +
> +void serial_test_uprobe_syscall_compat(void)
and then no need for serial_test?
> +{
> + test_uretprobe_compat();
> +}
> diff --git a/tools/testing/selftests/bpf/progs/uprobe_syscall_compat.c b/tools/testing/selftests/bpf/progs/uprobe_syscall_compat.c
> new file mode 100644
> index 000000000000..f8adde7f08e2
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/uprobe_syscall_compat.c
> @@ -0,0 +1,13 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +char _license[] SEC("license") = "GPL";
> +
> +SEC("uretprobe.multi/./uprobe_compat:main")
> +int uretprobe_compat(struct pt_regs *ctx)
> +{
> + bpf_printk("uretprobe compat\n");
> + return 0;
> +}
> --
> 2.44.0
>
^ permalink raw reply
* Re: [PATCHv3 bpf-next 5/7] selftests/bpf: Add uretprobe syscall call from user space test
From: Andrii Nakryiko @ 2024-04-26 18:03 UTC (permalink / raw)
To: Jiri Olsa
Cc: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
linux-kernel, linux-trace-kernel, linux-api, x86, bpf, Song Liu,
Yonghong Song, John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov (AMD), Ingo Molnar, Andy Lutomirski
In-Reply-To: <20240421194206.1010934-6-jolsa@kernel.org>
On Sun, Apr 21, 2024 at 12:43 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding test to verify that when called from outside of the
> trampoline provided by kernel, the uretprobe syscall will cause
> calling process to receive SIGILL signal and the attached bpf
> program is no executed.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> .../selftests/bpf/prog_tests/uprobe_syscall.c | 92 +++++++++++++++++++
> .../selftests/bpf/progs/uprobe_syscall_call.c | 15 +++
> 2 files changed, 107 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscall_call.c
>
See nits below, but overall LGTM
Acked-by: Andrii Nakryiko <andrii@kernel.org>
[...]
> @@ -219,6 +301,11 @@ static void test_uretprobe_regs_change(void)
> {
> test__skip();
> }
> +
> +static void test_uretprobe_syscall_call(void)
> +{
> + test__skip();
> +}
> #endif
>
> void test_uprobe_syscall(void)
> @@ -228,3 +315,8 @@ void test_uprobe_syscall(void)
> if (test__start_subtest("uretprobe_regs_change"))
> test_uretprobe_regs_change();
> }
> +
> +void serial_test_uprobe_syscall_call(void)
does it need to be serial? non-serial are still run sequentially
within a process (there is no multi-threading), it's more about some
global effects on system.
> +{
> + test_uretprobe_syscall_call();
> +}
> diff --git a/tools/testing/selftests/bpf/progs/uprobe_syscall_call.c b/tools/testing/selftests/bpf/progs/uprobe_syscall_call.c
> new file mode 100644
> index 000000000000..5ea03bb47198
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/uprobe_syscall_call.c
> @@ -0,0 +1,15 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +#include <string.h>
> +
> +struct pt_regs regs;
> +
> +char _license[] SEC("license") = "GPL";
> +
> +SEC("uretprobe//proc/self/exe:uretprobe_syscall_call")
> +int uretprobe(struct pt_regs *regs)
> +{
> + bpf_printk("uretprobe called");
debugging leftover? we probably don't want to pollute trace_pipe from test
> + return 0;
> +}
> --
> 2.44.0
>
^ permalink raw reply
* Re: [PATCHv3 bpf-next 2/7] uprobe: Add uretprobe syscall to speed up return probe
From: Andrii Nakryiko @ 2024-04-26 17:58 UTC (permalink / raw)
To: Jiri Olsa
Cc: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
linux-kernel, linux-trace-kernel, linux-api, x86, bpf, Song Liu,
Yonghong Song, John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov (AMD), Ingo Molnar, Andy Lutomirski
In-Reply-To: <20240421194206.1010934-3-jolsa@kernel.org>
On Sun, Apr 21, 2024 at 12:42 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding uretprobe syscall instead of trap to speed up return probe.
>
> At the moment the uretprobe setup/path is:
>
> - install entry uprobe
>
> - when the uprobe is hit, it overwrites probed function's return address
> on stack with address of the trampoline that contains breakpoint
> instruction
>
> - the breakpoint trap code handles the uretprobe consumers execution and
> jumps back to original return address
>
> This patch replaces the above trampoline's breakpoint instruction with new
> ureprobe syscall call. This syscall does exactly the same job as the trap
> with some more extra work:
>
> - syscall trampoline must save original value for rax/r11/rcx registers
> on stack - rax is set to syscall number and r11/rcx are changed and
> used by syscall instruction
>
> - the syscall code reads the original values of those registers and
> restore those values in task's pt_regs area
>
> - only caller from trampoline exposed in '[uprobes]' is allowed,
> the process will receive SIGILL signal otherwise
>
> Even with some extra work, using the uretprobes syscall shows speed
> improvement (compared to using standard breakpoint):
>
> On Intel (11th Gen Intel(R) Core(TM) i7-1165G7 @ 2.80GHz)
>
> current:
> uretprobe-nop : 1.498 ± 0.000M/s
> uretprobe-push : 1.448 ± 0.001M/s
> uretprobe-ret : 0.816 ± 0.001M/s
>
> with the fix:
> uretprobe-nop : 1.969 ± 0.002M/s < 31% speed up
> uretprobe-push : 1.910 ± 0.000M/s < 31% speed up
> uretprobe-ret : 0.934 ± 0.000M/s < 14% speed up
>
> On Amd (AMD Ryzen 7 5700U)
>
> current:
> uretprobe-nop : 0.778 ± 0.001M/s
> uretprobe-push : 0.744 ± 0.001M/s
> uretprobe-ret : 0.540 ± 0.001M/s
>
> with the fix:
> uretprobe-nop : 0.860 ± 0.001M/s < 10% speed up
> uretprobe-push : 0.818 ± 0.001M/s < 10% speed up
> uretprobe-ret : 0.578 ± 0.000M/s < 7% speed up
>
> The performance test spawns a thread that runs loop which triggers
> uprobe with attached bpf program that increments the counter that
> gets printed in results above.
>
> The uprobe (and uretprobe) kind is determined by which instruction
> is being patched with breakpoint instruction. That's also important
> for uretprobes, because uprobe is installed for each uretprobe.
>
> The performance test is part of bpf selftests:
> tools/testing/selftests/bpf/run_bench_uprobes.sh
>
> Note at the moment uretprobe syscall is supported only for native
> 64-bit process, compat process still uses standard breakpoint.
>
> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> arch/x86/kernel/uprobes.c | 115 ++++++++++++++++++++++++++++++++++++++
> include/linux/uprobes.h | 3 +
> kernel/events/uprobes.c | 24 +++++---
> 3 files changed, 135 insertions(+), 7 deletions(-)
>
LGTM as far as I can follow the code
Acked-by: Andrii Nakryiko <andrii@kernel.org>
[...]
^ permalink raw reply
* Re: [PATCHv3 bpf-next 1/7] uprobe: Wire up uretprobe system call
From: Andrii Nakryiko @ 2024-04-26 17:56 UTC (permalink / raw)
To: Jiri Olsa
Cc: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
linux-kernel, linux-trace-kernel, linux-api, x86, bpf, Song Liu,
Yonghong Song, John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov (AMD), Ingo Molnar, Andy Lutomirski
In-Reply-To: <20240421194206.1010934-2-jolsa@kernel.org>
On Sun, Apr 21, 2024 at 12:42 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Wiring up uretprobe system call, which comes in following changes.
> We need to do the wiring before, because the uretprobe implementation
> needs the syscall number.
>
> Note at the moment uretprobe syscall is supported only for native
> 64-bit process.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> arch/x86/entry/syscalls/syscall_64.tbl | 1 +
> include/linux/syscalls.h | 2 ++
> include/uapi/asm-generic/unistd.h | 5 ++++-
> kernel/sys_ni.c | 2 ++
> 4 files changed, 9 insertions(+), 1 deletion(-)
>
LGTM
Acked-by: Andrii Nakryiko <andrii@kernel.org>
> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
> index 7e8d46f4147f..af0a33ab06ee 100644
> --- a/arch/x86/entry/syscalls/syscall_64.tbl
> +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> @@ -383,6 +383,7 @@
> 459 common lsm_get_self_attr sys_lsm_get_self_attr
> 460 common lsm_set_self_attr sys_lsm_set_self_attr
> 461 common lsm_list_modules sys_lsm_list_modules
> +462 64 uretprobe sys_uretprobe
>
> #
> # Due to a historical design error, certain syscalls are numbered differently
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index e619ac10cd23..5318e0e76799 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -972,6 +972,8 @@ asmlinkage long sys_lsm_list_modules(u64 *ids, u32 *size, u32 flags);
> /* x86 */
> asmlinkage long sys_ioperm(unsigned long from, unsigned long num, int on);
>
> +asmlinkage long sys_uretprobe(void);
> +
> /* pciconfig: alpha, arm, arm64, ia64, sparc */
> asmlinkage long sys_pciconfig_read(unsigned long bus, unsigned long dfn,
> unsigned long off, unsigned long len,
> diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
> index 75f00965ab15..8a747cd1d735 100644
> --- a/include/uapi/asm-generic/unistd.h
> +++ b/include/uapi/asm-generic/unistd.h
> @@ -842,8 +842,11 @@ __SYSCALL(__NR_lsm_set_self_attr, sys_lsm_set_self_attr)
> #define __NR_lsm_list_modules 461
> __SYSCALL(__NR_lsm_list_modules, sys_lsm_list_modules)
>
> +#define __NR_uretprobe 462
> +__SYSCALL(__NR_uretprobe, sys_uretprobe)
> +
> #undef __NR_syscalls
> -#define __NR_syscalls 462
> +#define __NR_syscalls 463
>
> /*
> * 32 bit systems traditionally used different
> diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
> index faad00cce269..be6195e0d078 100644
> --- a/kernel/sys_ni.c
> +++ b/kernel/sys_ni.c
> @@ -391,3 +391,5 @@ COND_SYSCALL(setuid16);
>
> /* restartable sequence */
> COND_SYSCALL(rseq);
> +
> +COND_SYSCALL(uretprobe);
> --
> 2.44.0
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox