* Re: [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: Aleksa Sarai @ 2024-05-06 7:13 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Stas Sergeev, 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: <CALCETrUL3zXAX94CpcQYwj1omwO+=-1Li+J7Bw2kpAw4d7nsyw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 5369 bytes --]
On 2024-04-28, Andy Lutomirski <luto@amacapital.net> wrote:
> > 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.
Tying this to an actual mount rather than a file handle sounds like a
more plausible proposal than OA2_CRED_INHERIT, but it just seems that
this is going to re-create all of the work that went into id-mapped
mounts but with the extra-special step of making the generic VFS
permissions no longer work normally (unless the idea is that everything
would pretend to be owned by current_fsuid()?).
IMHO it also isn't enough to just make open work, you need to make all
operations work (which leads to a non-trivial amount of
filesystem-specific handling), which is just idmapped mounts. A lot of
work was put into making sure that is safe, and collapsing owners seems
like it will cause a lot of headaches.
I also find it somewhat amusing that this proposal is to basically give
up on multi-user permissions for this one directory tree because it's
too annoying to deal with. In that case, isn't chmod 777 a simpler
solution? (I'm being a bit flippant, of course there is a difference,
but the net result is that all users in the container would have the
same permissions with all of the fun issues that implies.)
In short, AFAICS idmapped mounts pretty much solve this problem (minus
the ability to collapse users, which I suspect is not a good idea in
general)?
> 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?
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCHv4 bpf-next 2/7] uprobe: Add uretprobe syscall to speed up return probe
From: Jiri Olsa @ 2024-05-06 10:56 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: olsajiri@gmail.com, songliubraving@fb.com, luto@kernel.org,
mhiramat@kernel.org, andrii@kernel.org, debug@rivosinc.com,
john.fastabend@gmail.com, linux-api@vger.kernel.org,
linux-kernel@vger.kernel.org, mingo@redhat.com,
rostedt@goodmis.org, ast@kernel.org, tglx@linutronix.de,
linux-man@vger.kernel.org, oleg@redhat.com, yhs@fb.com,
daniel@iogearbox.net, peterz@infradead.org,
linux-trace-kernel@vger.kernel.org, bp@alien8.de,
bpf@vger.kernel.org, x86@kernel.org, broonie@kernel.org
In-Reply-To: <d2e0e53581e26358ee0b3d188a07795878938d2f.camel@intel.com>
On Fri, May 03, 2024 at 08:35:24PM +0000, Edgecombe, Rick P wrote:
> On Fri, 2024-05-03 at 22:17 +0200, Jiri Olsa wrote:
> > when uretprobe is created, kernel overwrites the return address on user
> > stack to point to user space trampoline, so the setup is in kernel hands
>
> I mean for uprobes in general. I'm didn't have any specific ideas in mind, but
> in general when we give the kernel more abilities around shadow stack we have to
> think if attackers could use it to work around shadow stack protections.
>
> >
> > with the hack below on top of this patchset I'm no longer seeing shadow
> > stack app crash on uretprobe.. I'll try to polish it and send out next
> > week, any suggestions are welcome ;-)
>
> Thanks. Some comments below.
>
> >
> > thanks,
> > jirka
> >
> >
> > ---
> > diff --git a/arch/x86/include/asm/shstk.h b/arch/x86/include/asm/shstk.h
> > index 42fee8959df7..d374305a6851 100644
> > --- a/arch/x86/include/asm/shstk.h
> > +++ b/arch/x86/include/asm/shstk.h
> > @@ -21,6 +21,8 @@ unsigned long shstk_alloc_thread_stack(struct task_struct
> > *p, unsigned long clon
> > void shstk_free(struct task_struct *p);
> > int setup_signal_shadow_stack(struct ksignal *ksig);
> > int restore_signal_shadow_stack(void);
> > +void uprobe_change_stack(unsigned long addr);
> > +void uprobe_push_stack(unsigned long addr);
>
> Maybe name them:
> shstk_update_last_frame();
> shstk_push_frame();
ok
>
>
> > #else
> > static inline long shstk_prctl(struct task_struct *task, int option,
> > unsigned long arg2) { return -EINVAL; }
> > diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
> > index 59e15dd8d0f8..804c446231d9 100644
> > --- a/arch/x86/kernel/shstk.c
> > +++ b/arch/x86/kernel/shstk.c
> > @@ -577,3 +577,24 @@ long shstk_prctl(struct task_struct *task, int option,
> > unsigned long arg2)
> > return wrss_control(true);
> > return -EINVAL;
> > }
> > +
> > +void uprobe_change_stack(unsigned long addr)
> > +{
> > + unsigned long ssp;
>
> Probably want something like:
>
> if (!features_enabled(ARCH_SHSTK_SHSTK))
> return;
ok
>
> So this doesn't try the below if shadow stack is disabled.
>
> > +
> > + ssp = get_user_shstk_addr();
> > + write_user_shstk_64((u64 __user *)ssp, (u64)addr);
> > +}
>
> Can we know that there was a valid return address just before this point on the
> stack? Or could it be a sigframe or something?
when uprobe hijack the return address it assumes it's on the top of the stack,
so it's saved and replaced with address of the user space trampoline
>
> > +
> > +void uprobe_push_stack(unsigned long addr)
> > +{
> > + unsigned long ssp;
>
> if (!features_enabled(ARCH_SHSTK_SHSTK))
> return;
>
> > +
> > + ssp = get_user_shstk_addr();
> > + ssp -= SS_FRAME_SIZE;
> > + write_user_shstk_64((u64 __user *)ssp, (u64)addr);
> > +
> > + fpregs_lock_and_load();
> > + wrmsrl(MSR_IA32_PL3_SSP, ssp);
> > + fpregs_unlock();
> > +}
> > diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> > index 81e6ee95784d..259457838020 100644
> > --- a/arch/x86/kernel/uprobes.c
> > +++ b/arch/x86/kernel/uprobes.c
> > @@ -416,6 +416,7 @@ SYSCALL_DEFINE0(uretprobe)
> > regs->r11 = regs->flags;
> > regs->cx = regs->ip;
> >
> > + uprobe_push_stack(r11_cx_ax[2]);
>
> I'm concerned this could be used to push arbitrary frames to the shadow stack.
> Couldn't an attacker do a jump to the point that calls this syscall? Maybe this
> is what peterz was raising.
of course never say never, but here's my reasoning why I think it's ok
the page with the syscall trampoline is mapped in user space and can be
found in procfs maps file under '[uprobes]' name
the syscall can be called only from this trampoline, if it's called from
anywhere else the calling process receives SIGILL
now if you run the uretprobe syscall without any pending uretprobe for
the task it will receive SIGILL before it gets to the point of pushing
address on the shadow stack
and to configure the uretprobe you need to have CAP_PERFMON or CAP_SYS_ADMIN
if you'd actually managed to get the pending uretprobe instance, the shadow
stack entry is going to be used/pop-ed right away in the trampoline with
the ret instruction
and as I mentioned above it's ensured that the syscall is returning to the
trampoline and it can't be called from any other place
>
> > return regs->ax;
> >
> > sigill:
> > @@ -1191,8 +1192,10 @@ arch_uretprobe_hijack_return_addr(unsigned long
> > trampoline_vaddr, struct pt_regs
> > return orig_ret_vaddr;
> >
> > nleft = copy_to_user((void __user *)regs->sp, &trampoline_vaddr,
> > rasize);
> > - if (likely(!nleft))
> > + if (likely(!nleft)) {
> > + uprobe_change_stack(trampoline_vaddr);
> > return orig_ret_vaddr;
> > + }
> >
> > if (nleft != rasize) {
> > pr_err("return address clobbered: pid=%d, %%sp=%#lx,
> > %%ip=%#lx\n",
>
I'll try to add uprobe test under tools/testing/selftests/x86/test_shadow_stack.c
and send that and change below as part of new version
thanks for the comments,
jirka
---
diff --git a/arch/x86/include/asm/shstk.h b/arch/x86/include/asm/shstk.h
index 42fee8959df7..2e1ddcf98242 100644
--- a/arch/x86/include/asm/shstk.h
+++ b/arch/x86/include/asm/shstk.h
@@ -21,6 +21,8 @@ unsigned long shstk_alloc_thread_stack(struct task_struct *p, unsigned long clon
void shstk_free(struct task_struct *p);
int setup_signal_shadow_stack(struct ksignal *ksig);
int restore_signal_shadow_stack(void);
+int shstk_update_last_frame(unsigned long val);
+int shstk_push_frame(unsigned long val);
#else
static inline long shstk_prctl(struct task_struct *task, int option,
unsigned long arg2) { return -EINVAL; }
@@ -31,6 +33,8 @@ static inline unsigned long shstk_alloc_thread_stack(struct task_struct *p,
static inline void shstk_free(struct task_struct *p) {}
static inline int setup_signal_shadow_stack(struct ksignal *ksig) { return 0; }
static inline int restore_signal_shadow_stack(void) { return 0; }
+static inline int shstk_update_last_frame(unsigned long val) { return 0; }
+static inline int shstk_push_frame(unsigned long val) { return 0; }
#endif /* CONFIG_X86_USER_SHADOW_STACK */
#endif /* __ASSEMBLY__ */
diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
index 59e15dd8d0f8..66434dfde52e 100644
--- a/arch/x86/kernel/shstk.c
+++ b/arch/x86/kernel/shstk.c
@@ -577,3 +577,32 @@ long shstk_prctl(struct task_struct *task, int option, unsigned long arg2)
return wrss_control(true);
return -EINVAL;
}
+
+int shstk_update_last_frame(unsigned long val)
+{
+ unsigned long ssp;
+
+ if (!features_enabled(ARCH_SHSTK_SHSTK))
+ return 0;
+
+ ssp = get_user_shstk_addr();
+ return write_user_shstk_64((u64 __user *)ssp, (u64)val);
+}
+
+int shstk_push_frame(unsigned long val)
+{
+ unsigned long ssp;
+
+ if (!features_enabled(ARCH_SHSTK_SHSTK))
+ return 0;
+
+ ssp = get_user_shstk_addr();
+ ssp -= SS_FRAME_SIZE;
+ if (write_user_shstk_64((u64 __user *)ssp, (u64)val))
+ return -EFAULT;
+
+ fpregs_lock_and_load();
+ wrmsrl(MSR_IA32_PL3_SSP, ssp);
+ fpregs_unlock();
+ return 0;
+}
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 81e6ee95784d..ae6c3458a675 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -406,6 +406,11 @@ SYSCALL_DEFINE0(uretprobe)
* trampoline's ret instruction
*/
r11_cx_ax[2] = regs->ip;
+
+ /* make the shadow stack follow that */
+ if (shstk_push_frame(regs->ip))
+ goto sigill;
+
regs->ip = ip;
err = copy_to_user((void __user *)regs->sp, r11_cx_ax, sizeof(r11_cx_ax));
@@ -1191,8 +1196,13 @@ arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, struct pt_regs
return orig_ret_vaddr;
nleft = copy_to_user((void __user *)regs->sp, &trampoline_vaddr, rasize);
- if (likely(!nleft))
+ if (likely(!nleft)) {
+ if (shstk_update_last_frame(trampoline_vaddr)) {
+ force_sig(SIGSEGV);
+ return -1;
+ }
return orig_ret_vaddr;
+ }
if (nleft != rasize) {
pr_err("return address clobbered: pid=%d, %%sp=%#lx, %%ip=%#lx\n",
^ permalink raw reply related
* Re: [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: Andy Lutomirski @ 2024-05-06 17:29 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Stas Sergeev, 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: <20240428.171236-tangy.giblet.idle.helpline-y9LqufL7EAAV@cyphar.com>
Replying to a couple emails at once...
On Mon, May 6, 2024 at 12:14 AM Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> On 2024-04-28, Andy Lutomirski <luto@amacapital.net> wrote:
> > > 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.
>
> Tying this to an actual mount rather than a file handle sounds like a
> more plausible proposal than OA2_CRED_INHERIT, but it just seems that
> this is going to re-create all of the work that went into id-mapped
> mounts but with the extra-special step of making the generic VFS
> permissions no longer work normally (unless the idea is that everything
> would pretend to be owned by current_fsuid()?).
I was assuming that the owner uid and gid would be show to stat, etc
as usual. But the permission checks would be done against the
captured creds.
>
> IMHO it also isn't enough to just make open work, you need to make all
> operations work (which leads to a non-trivial amount of
> filesystem-specific handling), which is just idmapped mounts. A lot of
> work was put into making sure that is safe, and collapsing owners seems
> like it will cause a lot of headaches.
>
> I also find it somewhat amusing that this proposal is to basically give
> up on multi-user permissions for this one directory tree because it's
> too annoying to deal with. In that case, isn't chmod 777 a simpler
> solution? (I'm being a bit flippant, of course there is a difference,
> but the net result is that all users in the container would have the
> same permissions with all of the fun issues that implies.)
>
> In short, AFAICS idmapped mounts pretty much solve this problem (minus
> the ability to collapse users, which I suspect is not a good idea in
> general)?
>
With my kernel hat on, maybe I agree. But with my *user* hat on, I
think I pretty strongly disagree. Look, idmapis lousy for
unprivileged use:
$ install -m 0700 -d test_directory
$ echo 'hi there' >test_directory/file
$ podman run -it --rm
--mount=type=bind,src=test_directory,dst=/tmp,idmap [debian-slim]
# cat /tmp/file
hi there
<-- Hey, look, this kind of works!
# setpriv --reuid=1 ls /tmp
ls: cannot open directory '/tmp': Permission denied
<-- Gee, thanks, Linux!
Obviously this is a made up example. But it's quite analogous to a
real example. Suppose I want to make a directory that will contain
some MySQL data. I don't want to share this directory with anyone
else, so I set its mode to 0700. Then I want to fire up an
unprivileged MySQL container, so I build or download it, and then I
run it and bind my directory to /var/lib/mysql and I run it. I don't
need to think about UIDs or anything because it's 2024 and containers
just work. Okay, I need to setenforce 0 because I'm on Fedora and
SELinux makes absolutely no sense in a container world, but I can live
with that.
Except that it doesn't work! Because unless I want to manually futz
with the idmaps to get mysql to have access to the directory inside
the container, only *root* gets to get in. But I bet that even
futzing with the idmap doesn't work, because software like mysql often
expects that root *and* a user can access data. And some software
even does privilege separation and uses more than one UID.
So I want a way to give *an entire container* access to a directory.
Classic UNIX DAC is just *wrong* for this use case. Maybe idmaps
could learn a way to squash multiple ids down to one. Or maybe
something like my silly credential-capturing mount proposal could
work. But the status quo is not actually amazing IMO.
I haven't looked at the idmap implementation nearly enough to have any
opinion as to whether squashing UID is practical or whether there's
any sensible way to specify it in the configuration.
> On Apr 29, 2024, at 2:12 AM, Christian Brauner <brauner@kernel.org> wrote:
>
> Nowadays it's extremely simple due tue open_tree(OPEN_TREE_CLONE) and
> move_mount(). I rewrote the bind-mount logic in systemd based on that
> and util-linux uses that as well now.
> https://brauner.io/2023/02/28/mounting-into-mount-namespaces.html
>
Yep, I remember that.
>> 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.
>
> You're describing idmapped mounts for the most part which are upstream
> and are used in exactly that way by a lot of userspace.
>
See above...
>>
>> 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?
>
> That would mean override creds in the VFS layer when accessing a
> specific subtree which is a terrible idea imho. Not just because it will
> quickly become a potential dos when you do that with a lot of subtrees
> it will also have complex interactions with overlayfs.
I was deliberately talking about semantics, not implementation. This
may well be impossible to implement straightforwardly.
^ permalink raw reply
* Re: [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: Andy Lutomirski @ 2024-05-06 17:34 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Stas Sergeev, 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: <CALCETrU2VwCF-o7E5sc8FN_LBs3Q-vNMBf7N4rm0PAWFRo5QWw@mail.gmail.com>
On Mon, May 6, 2024 at 10:29 AM Andy Lutomirski <luto@amacapital.net> wrote:
>
> Replying to a couple emails at once...
>
> On Mon, May 6, 2024 at 12:14 AM Aleksa Sarai <cyphar@cyphar.com> wrote:
> > I also find it somewhat amusing that this proposal is to basically give
> > up on multi-user permissions for this one directory tree because it's
> > too annoying to deal with. In that case, isn't chmod 777 a simpler
> > solution? (I'm being a bit flippant, of course there is a difference,
> > but the net result is that all users in the container would have the
> > same permissions with all of the fun issues that implies.)
> >
> > In short, AFAICS idmapped mounts pretty much solve this problem (minus
> > the ability to collapse users, which I suspect is not a good idea in
> > general)?
> >
>
> With my kernel hat on, maybe I agree. But with my *user* hat on, I
> think I pretty strongly disagree. Look, idmapis lousy for
> unprivileged use:
>
> $ install -m 0700 -d test_directory
> $ echo 'hi there' >test_directory/file
> $ podman run -it --rm
> --mount=type=bind,src=test_directory,dst=/tmp,idmap [debian-slim]
> # cat /tmp/file
> hi there
>
> <-- Hey, look, this kind of works!
>
> # setpriv --reuid=1 ls /tmp
> ls: cannot open directory '/tmp': Permission denied
>
> <-- Gee, thanks, Linux!
I should add: this is lousy even for privileged use. On a normal
non-containerized system:
$ ls -ld /var/lib/mysql
drwxr-xr-x. 3 mysql mysql 4096 Sep 20 2023 /var/lib/mysql
This makes perfect sense.
But if I want to run mysql in a container in a sane way, my only real
choice is either to trust the container manager quite strongly (so it
only maps this directory into the correct container) or, if I want to
separate out management of this container into its own UID (which is a
good practice), then I'm forced to do some kind of fragile hack like
making a directory only accessible to the correct UID and then
creating a 0777 directory inside it and bind-mounting *that*.
^ permalink raw reply
* RE: [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: David Laight @ 2024-05-06 19:34 UTC (permalink / raw)
To: 'Andy Lutomirski', Aleksa Sarai
Cc: Stas Sergeev, Serge E. Hallyn, linux-kernel@vger.kernel.org,
Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Christian Brauner, Jan Kara, Jeff Layton,
Chuck Lever, Alexander Aring, linux-fsdevel@vger.kernel.org,
linux-api@vger.kernel.org, Paolo Bonzini, Christian Göttsche
In-Reply-To: <CALCETrU2VwCF-o7E5sc8FN_LBs3Q-vNMBf7N4rm0PAWFRo5QWw@mail.gmail.com>
...
> So I want a way to give *an entire container* access to a directory.
> Classic UNIX DAC is just *wrong* for this use case. Maybe idmaps
> could learn a way to squash multiple ids down to one. Or maybe
> something like my silly credential-capturing mount proposal could
> work. But the status quo is not actually amazing IMO.
Isn't that what gids are for :-)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: Andy Lutomirski @ 2024-05-06 21:53 UTC (permalink / raw)
To: David Laight
Cc: Aleksa Sarai, Stas Sergeev, Serge E. Hallyn,
linux-kernel@vger.kernel.org, Stefan Metzmacher, Eric Biederman,
Alexander Viro, Andy Lutomirski, Christian Brauner, Jan Kara,
Jeff Layton, Chuck Lever, Alexander Aring,
linux-fsdevel@vger.kernel.org, linux-api@vger.kernel.org,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <f8fafe1953ed41828a4c98187964477b@AcuMS.aculab.com>
On Mon, May 6, 2024 at 12:35 PM David Laight <David.Laight@aculab.com> wrote:
>
> ...
> > So I want a way to give *an entire container* access to a directory.
> > Classic UNIX DAC is just *wrong* for this use case. Maybe idmaps
> > could learn a way to squash multiple ids down to one. Or maybe
> > something like my silly credential-capturing mount proposal could
> > work. But the status quo is not actually amazing IMO.
>
> Isn't that what gids are for :-)
I dunno. How, exactly, is a regular non-root user of a Linux computer
supposed to configure gids in their home directory so that a container
(which uses subgids, possibly dynamically allocated) gets access to
the correct thing? And why should that poor user need to think about
this at all?
--Andy
^ permalink raw reply
* Re: [PATCH v4 00/30] NT synchronization primitive driver
From: Elizabeth Figura @ 2024-05-07 0:50 UTC (permalink / raw)
To: Peter Zijlstra
Cc: wine-devel, Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet,
Shuah Khan, linux-kernel, linux-api, André Almeida,
Wolfram Sang, Andy Lutomirski, linux-doc, linux-kselftest,
Randy Dunlap, Ingo Molnar, Will Deacon, Waiman Long, Boqun Feng,
Elizabeth Figura
In-Reply-To: <4560699.LvFx2qVVIh@camazotz>
On Friday, April 19, 2024 3:46:07 PM CDT Elizabeth Figura wrote:
> On Friday, 19 April 2024 11:16:11 CDT Peter Zijlstra wrote:
>
> > On Tue, Apr 16, 2024 at 05:18:56PM -0500, Elizabeth Figura wrote:
> >
> > > On Tuesday, 16 April 2024 16:18:24 CDT Elizabeth Figura wrote:
> > >
> > > > On Tuesday, 16 April 2024 03:14:21 CDT Peter Zijlstra wrote:
> > > >
> > > > > I don't support GE has it in his builds? Last time I tried,
> > > > > building
> > > > > Wine was a bit of a pain.
> > > >
> > > >
> > > > It doesn't seem so. I tried to build a GE-compatible ntsync build,
> > > > uploaded
here (thanks Arek for hosting):
> > > >
> > > >
> > > > https://f002.backblazeb2.com/file/wine-ntsync/ntsync-wine.tar.xz
> > >
> > >
> > > Oops, the initial version I uploaded had broken paths. Should be fixed
> > > now.
> > > (It's also broken on an unpatched kernel unless explicitly disabled with
> > >
> > > WINE_DISABLE_FAST_SYNC=1. Not sure what I messed up there—it should fall
> > > back
cleanly—but hopefully shouldn't be too important for testing.)
> >
> >
> > So I've tried using that wine build with lutris, and I can't get it to
> > start EGS or anything else.
> >
> > I even added a printk to the ntsync driver for every open, to see if it
> > gets that far, but I'm not even getting that :/
>
>
> That's odd, it works for me, both as a standalone build and with
> lutris...
>
> Does /dev/ntsync exist (module is loaded) and have nonzero permissions?
> I forgot to mention that's necessary, sorry.
>
> Otherwise I can try to look at an strace, or a Wine debug log. I don't
> think there's an easy way to get the latter with Lutris, but something
> like `WINEDEBUG=+all ./wine winecfg 2>log` should work.
>
>
It's also possible that the build was made against too new libraries. I've
created a new build, built and tested on stock Debian 12, and with some extra
debugging:
https://f002.backblazeb2.com/file/wine-ntsync/ntsync-wine2.tar.xz
Hopefully that's good enough to test with, or at least gives more of a hint as
to why it fails?
^ permalink raw reply
* Re: [PATCH v4 00/30] NT synchronization primitive driver
From: Elizabeth Figura @ 2024-05-07 0:40 UTC (permalink / raw)
To: Peter Zijlstra, wine-devel
Cc: wine-devel, Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet,
Shuah Khan, linux-kernel, linux-api, André Almeida,
Wolfram Sang, Andy Lutomirski, linux-doc, linux-kselftest,
Randy Dunlap, Ingo Molnar, Will Deacon, Waiman Long, Boqun Feng,
Elizabeth Figura
In-Reply-To: <4560699.LvFx2qVVIh@camazotz>
On Friday, April 19, 2024 3:46:07 PM CDT Elizabeth Figura wrote:
> On Friday, 19 April 2024 11:16:11 CDT Peter Zijlstra wrote:
>
> > On Tue, Apr 16, 2024 at 05:18:56PM -0500, Elizabeth Figura wrote:
> >
> > > On Tuesday, 16 April 2024 16:18:24 CDT Elizabeth Figura wrote:
> > >
> > > > On Tuesday, 16 April 2024 03:14:21 CDT Peter Zijlstra wrote:
> > > >
> > > > > I don't support GE has it in his builds? Last time I tried,
> > > > > building
> > > > > Wine was a bit of a pain.
> > > >
> > > >
> > > > It doesn't seem so. I tried to build a GE-compatible ntsync build,
> > > > uploaded
here (thanks Arek for hosting):
> > > >
> > > >
> > > > https://f002.backblazeb2.com/file/wine-ntsync/ntsync-wine.tar.xz
> > >
> > >
> > > Oops, the initial version I uploaded had broken paths. Should be fixed
> > > now.
> > > (It's also broken on an unpatched kernel unless explicitly disabled with
> > >
> > > WINE_DISABLE_FAST_SYNC=1. Not sure what I messed up there—it should fall
> > > back
cleanly—but hopefully shouldn't be too important for testing.)
> >
> >
> > So I've tried using that wine build with lutris, and I can't get it to
> > start EGS or anything else.
> >
> > I even added a printk to the ntsync driver for every open, to see if it
> > gets that far, but I'm not even getting that :/
>
>
> That's odd, it works for me, both as a standalone build and with
> lutris...
>
> Does /dev/ntsync exist (module is loaded) and have nonzero permissions?
> I forgot to mention that's necessary, sorry.
>
> Otherwise I can try to look at an strace, or a Wine debug log. I don't
> think there's an easy way to get the latter with Lutris, but something
> like `WINEDEBUG=+all ./wine winecfg 2>log` should work.
It's also possible I made that build against a too-new distribution. I've
created a new build against Debian 12, which is hopefully better, and it has
some extra debug logging:
https://f002.backblazeb2.com/file/wine-ntsync/ntsync-wine2.tar.xz
Hopefully that's functional enough to test with, or at least give more a hint
as to why it doesn't work?
^ permalink raw reply
* Re: [PATCH v5 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: Christian Brauner @ 2024-05-07 7:42 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Aleksa Sarai, Stas Sergeev, Serge E. Hallyn, linux-kernel,
Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Jan Kara, Jeff Layton, Chuck Lever,
Alexander Aring, David Laight, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <CALCETrU2VwCF-o7E5sc8FN_LBs3Q-vNMBf7N4rm0PAWFRo5QWw@mail.gmail.com>
> With my kernel hat on, maybe I agree. But with my *user* hat on, I
> think I pretty strongly disagree. Look, idmapis lousy for
> unprivileged use:
>
> $ install -m 0700 -d test_directory
> $ echo 'hi there' >test_directory/file
> $ podman run -it --rm
> --mount=type=bind,src=test_directory,dst=/tmp,idmap [debian-slim]
$ podman run -it --rm --mount=type=bind,src=test_directory,dst=/tmp,idmap [debian-slim]
as an unprivileged user doesn't use idmapped mounts at all. So I'm not
sure what this is showing. I suppose you're talking about idmaps in
general.
> # cat /tmp/file
> hi there
>
> <-- Hey, look, this kind of works!
>
> # setpriv --reuid=1 ls /tmp
> ls: cannot open directory '/tmp': Permission denied
>
> <-- Gee, thanks, Linux!
>
>
> Obviously this is a made up example. But it's quite analogous to a
> real example. Suppose I want to make a directory that will contain
> some MySQL data. I don't want to share this directory with anyone
> else, so I set its mode to 0700. Then I want to fire up an
> unprivileged MySQL container, so I build or download it, and then I
> run it and bind my directory to /var/lib/mysql and I run it. I don't
> need to think about UIDs or anything because it's 2024 and containers
> just work. Okay, I need to setenforce 0 because I'm on Fedora and
> SELinux makes absolutely no sense in a container world, but I can live
> with that.
>
> Except that it doesn't work! Because unless I want to manually futz
> with the idmaps to get mysql to have access to the directory inside
> the container, only *root* gets to get in. But I bet that even
> futzing with the idmap doesn't work, because software like mysql often
> expects that root *and* a user can access data. And some software
> even does privilege separation and uses more than one UID.
If the directory is 700 and it's owned by say root:root on the host and
you want to share that with arbitrary container users then this isn't
something you can do today (ignoring group permissions and ACLs for the
sake of your argument) even on the host so that's not a limitation of
userns or idmapped mounts. That means many to one mappings of uids/gids.
> So I want a way to give *an entire container* access to a directory.
> Classic UNIX DAC is just *wrong* for this use case. Maybe idmaps
> could learn a way to squash multiple ids down to one. Or maybe
Many idmappings to one is in principle possible and I've noted that idea
down as a possible extension at
https://github.com/uapi-group/kernel-features quite a while (2 years?) ago.
> I haven't looked at the idmap implementation nearly enough to have any
> opinion as to whether squashing UID is practical or whether there's
It's doable. The interesting bit to me was that if we want to allow
writes we'd need a way to determine what the uid/gid would be to write
down. Imho, that's not super difficult to solve though. The most obvious
one is that userspace can just determine it when creating the idmapped
mount.
^ permalink raw reply
* Re: [PATCHv4 bpf-next 0/7] uprobe: uretprobe speed up
From: Jiri Olsa @ 2024-05-07 7:47 UTC (permalink / raw)
To: Jiri Olsa
Cc: Andrii Nakryiko, Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski
In-Reply-To: <ZjVLedyQFBoHh-T_@krava>
On Fri, May 03, 2024 at 10:39:21PM +0200, Jiri Olsa wrote:
> On Fri, May 03, 2024 at 11:03:24AM -0700, Andrii Nakryiko wrote:
> > On Thu, May 2, 2024 at 1:04 PM Jiri Olsa <olsajiri@gmail.com> wrote:
> > >
> > > On Thu, May 02, 2024 at 09:43:02AM -0700, Andrii Nakryiko wrote:
> > > > On Thu, May 2, 2024 at 5:23 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > > > >
> > > > > hi,
> > > > > as part of the effort on speeding up the uprobes [0] coming with
> > > > > return uprobe optimization by using syscall instead of the trap
> > > > > on the uretprobe trampoline.
> > > > >
> > > > > The speed up depends on instruction type that uprobe is installed
> > > > > and depends on specific HW type, please check patch 1 for details.
> > > > >
> > > > > Patches 1-6 are based on bpf-next/master, but path 1 and 2 are
> > > > > apply-able on linux-trace.git tree probes/for-next branch.
> > > > > Patch 7 is based on man-pages master.
> > > > >
> > > > > v4 changes:
> > > > > - added acks [Oleg,Andrii,Masami]
> > > > > - reworded the man page and adding more info to NOTE section [Masami]
> > > > > - rewrote bpf tests not to use trace_pipe [Andrii]
> > > > > - cc-ed linux-man list
> > > > >
> > > > > Also available at:
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> > > > > uretprobe_syscall
> > > > >
> > > >
> > > > It looks great to me, thanks! Unfortunately BPF CI build is broken,
> > > > probably due to some of the Makefile additions, please investigate and
> > > > fix (or we'll need to fix something on BPF CI side), but it looks like
> > > > you'll need another revision, unfortunately.
> > > >
> > > > pw-bot: cr
> > > >
> > > > [0] https://github.com/kernel-patches/bpf/actions/runs/8923849088/job/24509002194
> > >
> > > yes, I think it's missing the 32-bit libc for uprobe_compat binary,
> > > probably it needs to be added to github.com:libbpf/ci.git setup-build-env/action.yml ?
> > > hm but I'm not sure how to test it, need to check
> >
> > You can create a custom PR directly against Github repo
> > (kernel-patches/bpf) and BPF CI will run all the tests on your custom
> > code. This way you can iterate without spamming the mailing list.
>
> I'm running CI tests like that, but I think I need to change the action
> which is in other repo (github.com:libbpf/ci.git)
>
> >
> > But I'm just wondering if it's worth complicating setup just for
> > testing this x32 compat mode. So maybe just dropping one of those
> > patches would be better?
>
> well, we had compat process crashing on uretprobe because of this change,
> so I rather keep the test.. or it can go in later on when the CI stuff is
> figured out.. I got busy with the shadow stack issue today, will check on
> the CI PR next week
ok, it's not as easy as just adding the package.. I don't want to delay
this on my missing github skills, I'll skip the test in next version and
submit it separately when the github ci is ready for that
jirka
^ permalink raw reply
* Re: [PATCH v6 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: Aleksa Sarai @ 2024-05-07 7:50 UTC (permalink / raw)
To: Stas Sergeev
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: <20240427112451.1609471-1-stsp2@yandex.ru>
[-- Attachment #1: Type: text/plain, Size: 2570 bytes --]
On 2024-04-27, 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.
>
> 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.
(I meant to reply last week but I couldn't get my mail server to send
mail...)
It seems to me that this can already be implemented using
MOUNT_ATTR_IDMAP, without creating a new form of credential overriding
within the filesystem (and with such a deceptively simple
implementation...)
If you are a privileged process which plans to change users, you can
create a detached tree with a user mapping that gives that user access
to only that tree. This is far more effective at restricting possible
attacks because id-mapped mounts don't override credentials during VFS
operations (meaning that if you miss something, you have a big problem),
instead they only affect uid-related operations within the filesystem
for that mount. Since this implementation does no inherit
CAP_DAC_OVERRIDE, being able to rewrite uid/gids is all you need.
A new attack I just thought of while writing this mail is that because
there is no RESOLVE_NO_XDEV requirement, it should be possible for the
process to get an arbitrary write primitive by creating a new
userns+mountns and then bind-mounting / underneath the directory. Since
O_CRED_INHERIT uses override_creds, it doesn't care about whether
something about the O_CRED_ALLOW directory changed afterwards. Yes, you
can "just fix this" by adding a RESOLVE_NO_XDEV requirement too, but
given that there have been 2-3 security issues with this design found
already, it makes me feel really uneasy. Using id-mapped mounts avoids
this issue because the new mount will not have the id-mapping applied
and thus there is no security issue.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v6 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: stsp @ 2024-05-07 9:02 UTC (permalink / raw)
To: Aleksa Sarai
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: <20240506.071502-teak.lily.alpine.girls-aiKJgErDohK@cyphar.com>
07.05.2024 10:50, Aleksa Sarai пишет:
> If you are a privileged process which plans to change users,
Not privileged at all.
But I think what you say is still possible
with userns?
> A new attack I just thought of while writing this mail is that because
> there is no RESOLVE_NO_XDEV requirement, it should be possible for the
> process to get an arbitrary write primitive by creating a new
> userns+mountns and then bind-mounting / underneath the directory.
Doesn't this need a write perm to a
directory? In his case this is not a threat,
because you are not supposed to have a
write perm to that dir. OA2_CRED_INHERIT
is the only way to write.
^ permalink raw reply
* [PATCHv5 bpf-next 0/8] uprobe: uretprobe speed up
From: Jiri Olsa @ 2024-05-07 10:53 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
hi,
as part of the effort on speeding up the uprobes [0] coming with
return uprobe optimization by using syscall instead of the trap
on the uretprobe trampoline.
The speed up depends on instruction type that uprobe is installed
and depends on specific HW type, please check patch 1 for details.
Patches 1-7 are based on bpf-next/master, but path 1 and 2 are
apply-able on linux-trace.git tree probes/for-next branch.
Patch 8 is based on man-pages master.
v5 changes:
- added shadow stack support for uretprobe [peterz]
- reworded man page + typos [Alejandro Colom]
- added pipe ASSERT_OK [Andrii]
- added acks [Andrii]
- removed compat test for now before ci is fixed
Also available at:
https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
uretprobe_syscall
thanks,
jirka
Notes to check list items in Documentation/process/adding-syscalls.rst:
- System Call Alternatives
New syscall seems like the best way in here, because we need
just to quickly enter kernel with no extra arguments processing,
which we'd need to do if we decided to use another syscall.
- Designing the API: Planning for Extension
The uretprobe syscall is very specific and most likely won't be
extended in the future.
At the moment it does not take any arguments and even if it does
in future, it's allowed to be called only from trampoline prepared
by kernel, so there'll be no broken user.
- Designing the API: Other Considerations
N/A because uretprobe syscall does not return reference to kernel
object.
- Proposing the API
Wiring up of the uretprobe system call is in separate change,
selftests and man page changes are part of the patchset.
- Generic System Call Implementation
There's no CONFIG option for the new functionality because it
keeps the same behaviour from the user POV.
- x86 System Call Implementation
It's 64-bit syscall only.
- Compatibility System Calls (Generic)
N/A uretprobe syscall has no arguments and is not supported
for compat processes.
- Compatibility System Calls (x86)
N/A uretprobe syscall is not supported for compat processes.
- System Calls Returning Elsewhere
N/A.
- Other Details
N/A.
- Testing
Adding new bpf selftests and ran ltp on top of this change.
- Man Page
Attached.
- Do not call System Calls in the Kernel
N/A.
[0] https://lore.kernel.org/bpf/ZeCXHKJ--iYYbmLj@krava/
---
Jiri Olsa (7):
uprobe: Wire up uretprobe system call
uprobe: Add uretprobe syscall to speed up return probe
selftests/bpf: Add uretprobe syscall test for regs integrity
selftests/bpf: Add uretprobe syscall test for regs changes
selftests/bpf: Add uretprobe syscall call from user space test
x86/shstk: Add return uprobe support
selftests/x86: Add return uprobe shadow stack test
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
arch/x86/include/asm/shstk.h | 4 ++
arch/x86/kernel/shstk.c | 29 +++++++++
arch/x86/kernel/uprobes.c | 127 +++++++++++++++++++++++++++++++++++-
include/linux/syscalls.h | 2 +
include/linux/uprobes.h | 3 +
include/uapi/asm-generic/unistd.h | 5 +-
kernel/events/uprobes.c | 24 +++++--
kernel/sys_ni.c | 2 +
tools/include/linux/compiler.h | 4 ++
tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c | 123 ++++++++++++++++++++++++++++++++++-
tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c | 325 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
tools/testing/selftests/bpf/progs/uprobe_syscall.c | 15 +++++
tools/testing/selftests/bpf/progs/uprobe_syscall_executed.c | 17 +++++
tools/testing/selftests/x86/test_shadow_stack.c | 142 ++++++++++++++++++++++++++++++++++++++++
15 files changed, 813 insertions(+), 10 deletions(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscall.c
create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscall_executed.c
Jiri Olsa (1):
man2: Add uretprobe syscall page
man2/uretprobe.2 | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 50 insertions(+)
create mode 100644 man2/uretprobe.2
^ permalink raw reply
* [PATCHv5 bpf-next 1/8] uprobe: Wire up uretprobe system call
From: Jiri Olsa @ 2024-05-07 10:53 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240507105321.71524-1-jolsa@kernel.org>
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.
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
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(-)
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 related
* [PATCHv5 bpf-next 2/8] uprobe: Add uretprobe syscall to speed up return probe
From: Jiri Olsa @ 2024-05-07 10:53 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240507105321.71524-1-jolsa@kernel.org>
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>
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Acked-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(-)
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 6c07f6daaa22..81e6ee95784d 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -12,6 +12,7 @@
#include <linux/ptrace.h>
#include <linux/uprobes.h>
#include <linux/uaccess.h>
+#include <linux/syscalls.h>
#include <linux/kdebug.h>
#include <asm/processor.h>
@@ -308,6 +309,120 @@ static int uprobe_init_insn(struct arch_uprobe *auprobe, struct insn *insn, bool
}
#ifdef CONFIG_X86_64
+
+asm (
+ ".pushsection .rodata\n"
+ ".global uretprobe_syscall_entry\n"
+ "uretprobe_syscall_entry:\n"
+ "pushq %rax\n"
+ "pushq %rcx\n"
+ "pushq %r11\n"
+ "movq $" __stringify(__NR_uretprobe) ", %rax\n"
+ "syscall\n"
+ ".global uretprobe_syscall_check\n"
+ "uretprobe_syscall_check:\n"
+ "popq %r11\n"
+ "popq %rcx\n"
+
+ /* The uretprobe syscall replaces stored %rax value with final
+ * return address, so we don't restore %rax in here and just
+ * call ret.
+ */
+ "retq\n"
+ ".global uretprobe_syscall_end\n"
+ "uretprobe_syscall_end:\n"
+ ".popsection\n"
+);
+
+extern u8 uretprobe_syscall_entry[];
+extern u8 uretprobe_syscall_check[];
+extern u8 uretprobe_syscall_end[];
+
+void *arch_uprobe_trampoline(unsigned long *psize)
+{
+ static uprobe_opcode_t insn = UPROBE_SWBP_INSN;
+ struct pt_regs *regs = task_pt_regs(current);
+
+ /*
+ * At the moment the uretprobe syscall trampoline is supported
+ * only for native 64-bit process, the compat process still uses
+ * standard breakpoint.
+ */
+ if (user_64bit_mode(regs)) {
+ *psize = uretprobe_syscall_end - uretprobe_syscall_entry;
+ return uretprobe_syscall_entry;
+ }
+
+ *psize = UPROBE_SWBP_INSN_SIZE;
+ return &insn;
+}
+
+static unsigned long trampoline_check_ip(void)
+{
+ unsigned long tramp = uprobe_get_trampoline_vaddr();
+
+ return tramp + (uretprobe_syscall_check - uretprobe_syscall_entry);
+}
+
+SYSCALL_DEFINE0(uretprobe)
+{
+ struct pt_regs *regs = task_pt_regs(current);
+ unsigned long err, ip, sp, r11_cx_ax[3];
+
+ if (regs->ip != trampoline_check_ip())
+ goto sigill;
+
+ err = copy_from_user(r11_cx_ax, (void __user *)regs->sp, sizeof(r11_cx_ax));
+ if (err)
+ goto sigill;
+
+ /* expose the "right" values of r11/cx/ax/sp to uprobe_consumer/s */
+ regs->r11 = r11_cx_ax[0];
+ regs->cx = r11_cx_ax[1];
+ regs->ax = r11_cx_ax[2];
+ regs->sp += sizeof(r11_cx_ax);
+ regs->orig_ax = -1;
+
+ ip = regs->ip;
+ sp = regs->sp;
+
+ uprobe_handle_trampoline(regs);
+
+ /*
+ * uprobe_consumer has changed sp, we can do nothing,
+ * just return via iret
+ */
+ if (regs->sp != sp)
+ return regs->ax;
+ regs->sp -= sizeof(r11_cx_ax);
+
+ /* for the case uprobe_consumer has changed r11/cx */
+ r11_cx_ax[0] = regs->r11;
+ r11_cx_ax[1] = regs->cx;
+
+ /*
+ * ax register is passed through as return value, so we can use
+ * its space on stack for ip value and jump to it through the
+ * trampoline's ret instruction
+ */
+ r11_cx_ax[2] = regs->ip;
+ regs->ip = ip;
+
+ err = copy_to_user((void __user *)regs->sp, r11_cx_ax, sizeof(r11_cx_ax));
+ if (err)
+ goto sigill;
+
+ /* ensure sysret, see do_syscall_64() */
+ regs->r11 = regs->flags;
+ regs->cx = regs->ip;
+
+ return regs->ax;
+
+sigill:
+ force_sig(SIGILL);
+ return -1;
+}
+
/*
* If arch_uprobe->insn doesn't use rip-relative addressing, return
* immediately. Otherwise, rewrite the instruction so that it accesses
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index f46e0ca0169c..b503fafb7fb3 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -138,6 +138,9 @@ extern bool arch_uretprobe_is_alive(struct return_instance *ret, enum rp_check c
extern bool arch_uprobe_ignore(struct arch_uprobe *aup, struct pt_regs *regs);
extern void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr,
void *src, unsigned long len);
+extern void uprobe_handle_trampoline(struct pt_regs *regs);
+extern void *arch_uprobe_trampoline(unsigned long *psize);
+extern unsigned long uprobe_get_trampoline_vaddr(void);
#else /* !CONFIG_UPROBES */
struct uprobes_state {
};
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index e4834d23e1d1..c550449d66be 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1474,11 +1474,20 @@ static int xol_add_vma(struct mm_struct *mm, struct xol_area *area)
return ret;
}
+void * __weak arch_uprobe_trampoline(unsigned long *psize)
+{
+ static uprobe_opcode_t insn = UPROBE_SWBP_INSN;
+
+ *psize = UPROBE_SWBP_INSN_SIZE;
+ return &insn;
+}
+
static struct xol_area *__create_xol_area(unsigned long vaddr)
{
struct mm_struct *mm = current->mm;
- uprobe_opcode_t insn = UPROBE_SWBP_INSN;
+ unsigned long insns_size;
struct xol_area *area;
+ void *insns;
area = kmalloc(sizeof(*area), GFP_KERNEL);
if (unlikely(!area))
@@ -1502,7 +1511,8 @@ static struct xol_area *__create_xol_area(unsigned long vaddr)
/* Reserve the 1st slot for get_trampoline_vaddr() */
set_bit(0, area->bitmap);
atomic_set(&area->slot_count, 1);
- arch_uprobe_copy_ixol(area->pages[0], 0, &insn, UPROBE_SWBP_INSN_SIZE);
+ insns = arch_uprobe_trampoline(&insns_size);
+ arch_uprobe_copy_ixol(area->pages[0], 0, insns, insns_size);
if (!xol_add_vma(mm, area))
return area;
@@ -1827,7 +1837,7 @@ void uprobe_copy_process(struct task_struct *t, unsigned long flags)
*
* Returns -1 in case the xol_area is not allocated.
*/
-static unsigned long get_trampoline_vaddr(void)
+unsigned long uprobe_get_trampoline_vaddr(void)
{
struct xol_area *area;
unsigned long trampoline_vaddr = -1;
@@ -1878,7 +1888,7 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
if (!ri)
return;
- trampoline_vaddr = get_trampoline_vaddr();
+ trampoline_vaddr = uprobe_get_trampoline_vaddr();
orig_ret_vaddr = arch_uretprobe_hijack_return_addr(trampoline_vaddr, regs);
if (orig_ret_vaddr == -1)
goto fail;
@@ -2123,7 +2133,7 @@ static struct return_instance *find_next_ret_chain(struct return_instance *ri)
return ri;
}
-static void handle_trampoline(struct pt_regs *regs)
+void uprobe_handle_trampoline(struct pt_regs *regs)
{
struct uprobe_task *utask;
struct return_instance *ri, *next;
@@ -2187,8 +2197,8 @@ static void handle_swbp(struct pt_regs *regs)
int is_swbp;
bp_vaddr = uprobe_get_swbp_addr(regs);
- if (bp_vaddr == get_trampoline_vaddr())
- return handle_trampoline(regs);
+ if (bp_vaddr == uprobe_get_trampoline_vaddr())
+ return uprobe_handle_trampoline(regs);
uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
if (!uprobe) {
--
2.44.0
^ permalink raw reply related
* [PATCHv5 bpf-next 3/8] selftests/bpf: Add uretprobe syscall test for regs integrity
From: Jiri Olsa @ 2024-05-07 10:53 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240507105321.71524-1-jolsa@kernel.org>
Add uretprobe syscall test that compares register values before
and after the uretprobe is hit. It also compares the register
values seen from attached bpf program.
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/include/linux/compiler.h | 4 +
.../selftests/bpf/prog_tests/uprobe_syscall.c | 163 ++++++++++++++++++
.../selftests/bpf/progs/uprobe_syscall.c | 15 ++
3 files changed, 182 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscall.c
diff --git a/tools/include/linux/compiler.h b/tools/include/linux/compiler.h
index 8a63a9913495..6f7f22ac9da5 100644
--- a/tools/include/linux/compiler.h
+++ b/tools/include/linux/compiler.h
@@ -62,6 +62,10 @@
#define __nocf_check __attribute__((nocf_check))
#endif
+#ifndef __naked
+#define __naked __attribute__((__naked__))
+#endif
+
/* Are two types/vars the same type (ignoring qualifiers)? */
#ifndef __same_type
# define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
new file mode 100644
index 000000000000..311ac19d8992
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
@@ -0,0 +1,163 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <test_progs.h>
+
+#ifdef __x86_64__
+
+#include <unistd.h>
+#include <asm/ptrace.h>
+#include <linux/compiler.h>
+#include "uprobe_syscall.skel.h"
+
+__naked unsigned long uretprobe_regs_trigger(void)
+{
+ asm volatile (
+ "movq $0xdeadbeef, %rax\n"
+ "ret\n"
+ );
+}
+
+__naked void uretprobe_regs(struct pt_regs *before, struct pt_regs *after)
+{
+ asm volatile (
+ "movq %r15, 0(%rdi)\n"
+ "movq %r14, 8(%rdi)\n"
+ "movq %r13, 16(%rdi)\n"
+ "movq %r12, 24(%rdi)\n"
+ "movq %rbp, 32(%rdi)\n"
+ "movq %rbx, 40(%rdi)\n"
+ "movq %r11, 48(%rdi)\n"
+ "movq %r10, 56(%rdi)\n"
+ "movq %r9, 64(%rdi)\n"
+ "movq %r8, 72(%rdi)\n"
+ "movq %rax, 80(%rdi)\n"
+ "movq %rcx, 88(%rdi)\n"
+ "movq %rdx, 96(%rdi)\n"
+ "movq %rsi, 104(%rdi)\n"
+ "movq %rdi, 112(%rdi)\n"
+ "movq $0, 120(%rdi)\n" /* orig_rax */
+ "movq $0, 128(%rdi)\n" /* rip */
+ "movq $0, 136(%rdi)\n" /* cs */
+ "pushf\n"
+ "pop %rax\n"
+ "movq %rax, 144(%rdi)\n" /* eflags */
+ "movq %rsp, 152(%rdi)\n" /* rsp */
+ "movq $0, 160(%rdi)\n" /* ss */
+
+ /* save 2nd argument */
+ "pushq %rsi\n"
+ "call uretprobe_regs_trigger\n"
+
+ /* save return value and load 2nd argument pointer to rax */
+ "pushq %rax\n"
+ "movq 8(%rsp), %rax\n"
+
+ "movq %r15, 0(%rax)\n"
+ "movq %r14, 8(%rax)\n"
+ "movq %r13, 16(%rax)\n"
+ "movq %r12, 24(%rax)\n"
+ "movq %rbp, 32(%rax)\n"
+ "movq %rbx, 40(%rax)\n"
+ "movq %r11, 48(%rax)\n"
+ "movq %r10, 56(%rax)\n"
+ "movq %r9, 64(%rax)\n"
+ "movq %r8, 72(%rax)\n"
+ "movq %rcx, 88(%rax)\n"
+ "movq %rdx, 96(%rax)\n"
+ "movq %rsi, 104(%rax)\n"
+ "movq %rdi, 112(%rax)\n"
+ "movq $0, 120(%rax)\n" /* orig_rax */
+ "movq $0, 128(%rax)\n" /* rip */
+ "movq $0, 136(%rax)\n" /* cs */
+
+ /* restore return value and 2nd argument */
+ "pop %rax\n"
+ "pop %rsi\n"
+
+ "movq %rax, 80(%rsi)\n"
+
+ "pushf\n"
+ "pop %rax\n"
+
+ "movq %rax, 144(%rsi)\n" /* eflags */
+ "movq %rsp, 152(%rsi)\n" /* rsp */
+ "movq $0, 160(%rsi)\n" /* ss */
+ "ret\n"
+);
+}
+
+static void test_uretprobe_regs_equal(void)
+{
+ struct uprobe_syscall *skel = NULL;
+ struct pt_regs before = {}, after = {};
+ unsigned long *pb = (unsigned long *) &before;
+ unsigned long *pa = (unsigned long *) &after;
+ unsigned long *pp;
+ unsigned int i, cnt;
+ int err;
+
+ skel = uprobe_syscall__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "uprobe_syscall__open_and_load"))
+ goto cleanup;
+
+ err = uprobe_syscall__attach(skel);
+ if (!ASSERT_OK(err, "uprobe_syscall__attach"))
+ goto cleanup;
+
+ uretprobe_regs(&before, &after);
+
+ pp = (unsigned long *) &skel->bss->regs;
+ cnt = sizeof(before)/sizeof(*pb);
+
+ for (i = 0; i < cnt; i++) {
+ unsigned int offset = i * sizeof(unsigned long);
+
+ /*
+ * Check register before and after uretprobe_regs_trigger call
+ * that triggers the uretprobe.
+ */
+ switch (offset) {
+ case offsetof(struct pt_regs, rax):
+ ASSERT_EQ(pa[i], 0xdeadbeef, "return value");
+ break;
+ default:
+ if (!ASSERT_EQ(pb[i], pa[i], "register before-after value check"))
+ fprintf(stdout, "failed register offset %u\n", offset);
+ }
+
+ /*
+ * Check register seen from bpf program and register after
+ * uretprobe_regs_trigger call
+ */
+ switch (offset) {
+ /*
+ * These values will be different (not set in uretprobe_regs),
+ * we don't care.
+ */
+ case offsetof(struct pt_regs, orig_rax):
+ case offsetof(struct pt_regs, rip):
+ case offsetof(struct pt_regs, cs):
+ case offsetof(struct pt_regs, rsp):
+ case offsetof(struct pt_regs, ss):
+ break;
+ default:
+ if (!ASSERT_EQ(pp[i], pa[i], "register prog-after value check"))
+ fprintf(stdout, "failed register offset %u\n", offset);
+ }
+ }
+
+cleanup:
+ uprobe_syscall__destroy(skel);
+}
+#else
+static void test_uretprobe_regs_equal(void)
+{
+ test__skip();
+}
+#endif
+
+void test_uprobe_syscall(void)
+{
+ if (test__start_subtest("uretprobe_regs_equal"))
+ test_uretprobe_regs_equal();
+}
diff --git a/tools/testing/selftests/bpf/progs/uprobe_syscall.c b/tools/testing/selftests/bpf/progs/uprobe_syscall.c
new file mode 100644
index 000000000000..8a4fa6c7ef59
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/uprobe_syscall.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_regs_trigger")
+int uretprobe(struct pt_regs *ctx)
+{
+ __builtin_memcpy(®s, ctx, sizeof(regs));
+ return 0;
+}
--
2.44.0
^ permalink raw reply related
* [PATCHv5 bpf-next 4/8] selftests/bpf: Add uretprobe syscall test for regs changes
From: Jiri Olsa @ 2024-05-07 10:53 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240507105321.71524-1-jolsa@kernel.org>
Adding test that creates uprobe consumer on uretprobe which changes some
of the registers. Making sure the changed registers are propagated to the
user space when the ureptobe syscall trampoline is used on x86_64.
To be able to do this, adding support to bpf_testmod to create uprobe via
new attribute file:
/sys/kernel/bpf_testmod_uprobe
This file is expecting file offset and creates related uprobe on current
process exe file and removes existing uprobe if offset is 0. The can be
only single uprobe at any time.
The uprobe has specific consumer that changes registers used in ureprobe
syscall trampoline and which are later checked in the test.
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
.../selftests/bpf/bpf_testmod/bpf_testmod.c | 123 +++++++++++++++++-
.../selftests/bpf/prog_tests/uprobe_syscall.c | 67 ++++++++++
2 files changed, 189 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index e93013fc7bf4..6c43d68fa323 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -18,6 +18,7 @@
#include <linux/in6.h>
#include <linux/un.h>
#include <net/sock.h>
+#include <linux/namei.h>
#include "bpf_testmod.h"
#include "bpf_testmod_kfunc.h"
@@ -355,6 +356,119 @@ static struct bin_attribute bin_attr_bpf_testmod_file __ro_after_init = {
.write = bpf_testmod_test_write,
};
+/* bpf_testmod_uprobe sysfs attribute is so far enabled for x86_64 only,
+ * please see test_uretprobe_regs_change test
+ */
+#ifdef __x86_64__
+
+static int
+uprobe_ret_handler(struct uprobe_consumer *self, unsigned long func,
+ struct pt_regs *regs)
+
+{
+ regs->ax = 0x12345678deadbeef;
+ regs->cx = 0x87654321feebdaed;
+ regs->r11 = (u64) -1;
+ return true;
+}
+
+struct testmod_uprobe {
+ struct path path;
+ loff_t offset;
+ struct uprobe_consumer consumer;
+};
+
+static DEFINE_MUTEX(testmod_uprobe_mutex);
+
+static struct testmod_uprobe uprobe = {
+ .consumer.ret_handler = uprobe_ret_handler,
+};
+
+static int testmod_register_uprobe(loff_t offset)
+{
+ int err = -EBUSY;
+
+ if (uprobe.offset)
+ return -EBUSY;
+
+ mutex_lock(&testmod_uprobe_mutex);
+
+ if (uprobe.offset)
+ goto out;
+
+ err = kern_path("/proc/self/exe", LOOKUP_FOLLOW, &uprobe.path);
+ if (err)
+ goto out;
+
+ err = uprobe_register_refctr(d_real_inode(uprobe.path.dentry),
+ offset, 0, &uprobe.consumer);
+ if (err)
+ path_put(&uprobe.path);
+ else
+ uprobe.offset = offset;
+
+out:
+ mutex_unlock(&testmod_uprobe_mutex);
+ return err;
+}
+
+static void testmod_unregister_uprobe(void)
+{
+ mutex_lock(&testmod_uprobe_mutex);
+
+ if (uprobe.offset) {
+ uprobe_unregister(d_real_inode(uprobe.path.dentry),
+ uprobe.offset, &uprobe.consumer);
+ uprobe.offset = 0;
+ }
+
+ mutex_unlock(&testmod_uprobe_mutex);
+}
+
+static ssize_t
+bpf_testmod_uprobe_write(struct file *file, struct kobject *kobj,
+ struct bin_attribute *bin_attr,
+ char *buf, loff_t off, size_t len)
+{
+ unsigned long offset;
+ int err;
+
+ if (kstrtoul(buf, 0, &offset))
+ return -EINVAL;
+
+ if (offset)
+ err = testmod_register_uprobe(offset);
+ else
+ testmod_unregister_uprobe();
+
+ return err ?: strlen(buf);
+}
+
+static struct bin_attribute bin_attr_bpf_testmod_uprobe_file __ro_after_init = {
+ .attr = { .name = "bpf_testmod_uprobe", .mode = 0666, },
+ .write = bpf_testmod_uprobe_write,
+};
+
+static int register_bpf_testmod_uprobe(void)
+{
+ return sysfs_create_bin_file(kernel_kobj, &bin_attr_bpf_testmod_uprobe_file);
+}
+
+static void unregister_bpf_testmod_uprobe(void)
+{
+ testmod_unregister_uprobe();
+ sysfs_remove_bin_file(kernel_kobj, &bin_attr_bpf_testmod_uprobe_file);
+}
+
+#else
+static int register_bpf_testmod_uprobe(void)
+{
+ return 0;
+}
+
+static void unregister_bpf_testmod_uprobe(void) { }
+#endif
+
BTF_KFUNCS_START(bpf_testmod_common_kfunc_ids)
BTF_ID_FLAGS(func, bpf_iter_testmod_seq_new, KF_ITER_NEW)
BTF_ID_FLAGS(func, bpf_iter_testmod_seq_next, KF_ITER_NEXT | KF_RET_NULL)
@@ -909,7 +1023,13 @@ static int bpf_testmod_init(void)
return -EINVAL;
sock = NULL;
mutex_init(&sock_lock);
- return sysfs_create_bin_file(kernel_kobj, &bin_attr_bpf_testmod_file);
+ ret = sysfs_create_bin_file(kernel_kobj, &bin_attr_bpf_testmod_file);
+ if (ret < 0)
+ return ret;
+ ret = register_bpf_testmod_uprobe();
+ if (ret < 0)
+ return ret;
+ return 0;
}
static void bpf_testmod_exit(void)
@@ -924,6 +1044,7 @@ static void bpf_testmod_exit(void)
bpf_kfunc_close_sock();
sysfs_remove_bin_file(kernel_kobj, &bin_attr_bpf_testmod_file);
+ unregister_bpf_testmod_uprobe();
}
module_init(bpf_testmod_init);
diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
index 311ac19d8992..1a50cd35205d 100644
--- a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
@@ -149,15 +149,82 @@ static void test_uretprobe_regs_equal(void)
cleanup:
uprobe_syscall__destroy(skel);
}
+
+#define BPF_TESTMOD_UPROBE_TEST_FILE "/sys/kernel/bpf_testmod_uprobe"
+
+static int write_bpf_testmod_uprobe(unsigned long offset)
+{
+ size_t n, ret;
+ char buf[30];
+ int fd;
+
+ n = sprintf(buf, "%lu", offset);
+
+ fd = open(BPF_TESTMOD_UPROBE_TEST_FILE, O_WRONLY);
+ if (fd < 0)
+ return -errno;
+
+ ret = write(fd, buf, n);
+ close(fd);
+ return ret != n ? (int) ret : 0;
+}
+
+static void test_uretprobe_regs_change(void)
+{
+ struct pt_regs before = {}, after = {};
+ unsigned long *pb = (unsigned long *) &before;
+ unsigned long *pa = (unsigned long *) &after;
+ unsigned long cnt = sizeof(before)/sizeof(*pb);
+ unsigned int i, err, offset;
+
+ offset = get_uprobe_offset(uretprobe_regs_trigger);
+
+ err = write_bpf_testmod_uprobe(offset);
+ if (!ASSERT_OK(err, "register_uprobe"))
+ return;
+
+ uretprobe_regs(&before, &after);
+
+ err = write_bpf_testmod_uprobe(0);
+ if (!ASSERT_OK(err, "unregister_uprobe"))
+ return;
+
+ for (i = 0; i < cnt; i++) {
+ unsigned int offset = i * sizeof(unsigned long);
+
+ switch (offset) {
+ case offsetof(struct pt_regs, rax):
+ ASSERT_EQ(pa[i], 0x12345678deadbeef, "rax");
+ break;
+ case offsetof(struct pt_regs, rcx):
+ ASSERT_EQ(pa[i], 0x87654321feebdaed, "rcx");
+ break;
+ case offsetof(struct pt_regs, r11):
+ ASSERT_EQ(pa[i], (__u64) -1, "r11");
+ break;
+ default:
+ if (!ASSERT_EQ(pa[i], pb[i], "register before-after value check"))
+ fprintf(stdout, "failed register offset %u\n", offset);
+ }
+ }
+}
+
#else
static void test_uretprobe_regs_equal(void)
{
test__skip();
}
+
+static void test_uretprobe_regs_change(void)
+{
+ test__skip();
+}
#endif
void test_uprobe_syscall(void)
{
if (test__start_subtest("uretprobe_regs_equal"))
test_uretprobe_regs_equal();
+ if (test__start_subtest("uretprobe_regs_change"))
+ test_uretprobe_regs_change();
}
--
2.44.0
^ permalink raw reply related
* [PATCHv5 bpf-next 5/8] selftests/bpf: Add uretprobe syscall call from user space test
From: Jiri Olsa @ 2024-05-07 10:53 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240507105321.71524-1-jolsa@kernel.org>
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 not executed.
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
.../selftests/bpf/prog_tests/uprobe_syscall.c | 95 +++++++++++++++++++
.../bpf/progs/uprobe_syscall_executed.c | 17 ++++
2 files changed, 112 insertions(+)
create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscall_executed.c
diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
index 1a50cd35205d..3ef324c2db50 100644
--- a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
@@ -7,7 +7,10 @@
#include <unistd.h>
#include <asm/ptrace.h>
#include <linux/compiler.h>
+#include <linux/stringify.h>
+#include <sys/wait.h>
#include "uprobe_syscall.skel.h"
+#include "uprobe_syscall_executed.skel.h"
__naked unsigned long uretprobe_regs_trigger(void)
{
@@ -209,6 +212,91 @@ static void test_uretprobe_regs_change(void)
}
}
+#ifndef __NR_uretprobe
+#define __NR_uretprobe 462
+#endif
+
+__naked unsigned long uretprobe_syscall_call_1(void)
+{
+ /*
+ * Pretend we are uretprobe trampoline to trigger the return
+ * probe invocation in order to verify we get SIGILL.
+ */
+ asm volatile (
+ "pushq %rax\n"
+ "pushq %rcx\n"
+ "pushq %r11\n"
+ "movq $" __stringify(__NR_uretprobe) ", %rax\n"
+ "syscall\n"
+ "popq %r11\n"
+ "popq %rcx\n"
+ "retq\n"
+ );
+}
+
+__naked unsigned long uretprobe_syscall_call(void)
+{
+ asm volatile (
+ "call uretprobe_syscall_call_1\n"
+ "retq\n"
+ );
+}
+
+static void test_uretprobe_syscall_call(void)
+{
+ LIBBPF_OPTS(bpf_uprobe_multi_opts, opts,
+ .retprobe = true,
+ );
+ struct uprobe_syscall_executed *skel;
+ int pid, status, err, go[2], c;
+
+ if (ASSERT_OK(pipe(go), "pipe"))
+ return;
+
+ skel = uprobe_syscall_executed__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "uprobe_syscall_executed__open_and_load"))
+ goto cleanup;
+
+ pid = fork();
+ if (!ASSERT_GE(pid, 0, "fork"))
+ goto cleanup;
+
+ /* child */
+ if (pid == 0) {
+ close(go[1]);
+
+ /* wait for parent's kick */
+ err = read(go[0], &c, 1);
+ if (err != 1)
+ exit(-1);
+
+ uretprobe_syscall_call();
+ _exit(0);
+ }
+
+ skel->links.test = bpf_program__attach_uprobe_multi(skel->progs.test, pid,
+ "/proc/self/exe",
+ "uretprobe_syscall_call", &opts);
+ if (!ASSERT_OK_PTR(skel->links.test, "bpf_program__attach_uprobe_multi"))
+ goto cleanup;
+
+ /* kick the child */
+ write(go[1], &c, 1);
+ err = waitpid(pid, &status, 0);
+ ASSERT_EQ(err, pid, "waitpid");
+
+ /* verify the child got killed with SIGILL */
+ ASSERT_EQ(WIFSIGNALED(status), 1, "WIFSIGNALED");
+ ASSERT_EQ(WTERMSIG(status), SIGILL, "WTERMSIG");
+
+ /* verify the uretprobe program wasn't called */
+ ASSERT_EQ(skel->bss->executed, 0, "executed");
+
+cleanup:
+ uprobe_syscall_executed__destroy(skel);
+ close(go[1]);
+ close(go[0]);
+}
#else
static void test_uretprobe_regs_equal(void)
{
@@ -219,6 +307,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)
@@ -227,4 +320,6 @@ void test_uprobe_syscall(void)
test_uretprobe_regs_equal();
if (test__start_subtest("uretprobe_regs_change"))
test_uretprobe_regs_change();
+ if (test__start_subtest("uretprobe_syscall_call"))
+ test_uretprobe_syscall_call();
}
diff --git a/tools/testing/selftests/bpf/progs/uprobe_syscall_executed.c b/tools/testing/selftests/bpf/progs/uprobe_syscall_executed.c
new file mode 100644
index 000000000000..0d7f1a7db2e2
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/uprobe_syscall_executed.c
@@ -0,0 +1,17 @@
+// 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";
+
+int executed = 0;
+
+SEC("uretprobe.multi")
+int test(struct pt_regs *regs)
+{
+ executed = 1;
+ return 0;
+}
--
2.44.0
^ permalink raw reply related
* [PATCHv5 bpf-next 6/8] x86/shstk: Add return uprobe support
From: Jiri Olsa @ 2024-05-07 10:53 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240507105321.71524-1-jolsa@kernel.org>
Adding return uprobe support to work with enabled shadow stack.
Currently the application with enabled shadow stack will crash
if it sets up return uprobe. The reason is the uretprobe kernel
code changes the user space task's stack, but does not update
shadow stack accordingly.
Adding new functions to update values on shadow stack and using
them in uprobe code to keep shadow stack in sync with uretprobe
changes to user stack.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
arch/x86/include/asm/shstk.h | 4 ++++
arch/x86/kernel/shstk.c | 29 +++++++++++++++++++++++++++++
arch/x86/kernel/uprobes.c | 12 +++++++++++-
3 files changed, 44 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/shstk.h b/arch/x86/include/asm/shstk.h
index 42fee8959df7..2e1ddcf98242 100644
--- a/arch/x86/include/asm/shstk.h
+++ b/arch/x86/include/asm/shstk.h
@@ -21,6 +21,8 @@ unsigned long shstk_alloc_thread_stack(struct task_struct *p, unsigned long clon
void shstk_free(struct task_struct *p);
int setup_signal_shadow_stack(struct ksignal *ksig);
int restore_signal_shadow_stack(void);
+int shstk_update_last_frame(unsigned long val);
+int shstk_push_frame(unsigned long val);
#else
static inline long shstk_prctl(struct task_struct *task, int option,
unsigned long arg2) { return -EINVAL; }
@@ -31,6 +33,8 @@ static inline unsigned long shstk_alloc_thread_stack(struct task_struct *p,
static inline void shstk_free(struct task_struct *p) {}
static inline int setup_signal_shadow_stack(struct ksignal *ksig) { return 0; }
static inline int restore_signal_shadow_stack(void) { return 0; }
+static inline int shstk_update_last_frame(unsigned long val) { return 0; }
+static inline int shstk_push_frame(unsigned long val) { return 0; }
#endif /* CONFIG_X86_USER_SHADOW_STACK */
#endif /* __ASSEMBLY__ */
diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
index 59e15dd8d0f8..66434dfde52e 100644
--- a/arch/x86/kernel/shstk.c
+++ b/arch/x86/kernel/shstk.c
@@ -577,3 +577,32 @@ long shstk_prctl(struct task_struct *task, int option, unsigned long arg2)
return wrss_control(true);
return -EINVAL;
}
+
+int shstk_update_last_frame(unsigned long val)
+{
+ unsigned long ssp;
+
+ if (!features_enabled(ARCH_SHSTK_SHSTK))
+ return 0;
+
+ ssp = get_user_shstk_addr();
+ return write_user_shstk_64((u64 __user *)ssp, (u64)val);
+}
+
+int shstk_push_frame(unsigned long val)
+{
+ unsigned long ssp;
+
+ if (!features_enabled(ARCH_SHSTK_SHSTK))
+ return 0;
+
+ ssp = get_user_shstk_addr();
+ ssp -= SS_FRAME_SIZE;
+ if (write_user_shstk_64((u64 __user *)ssp, (u64)val))
+ return -EFAULT;
+
+ fpregs_lock_and_load();
+ wrmsrl(MSR_IA32_PL3_SSP, ssp);
+ fpregs_unlock();
+ return 0;
+}
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 81e6ee95784d..ae6c3458a675 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -406,6 +406,11 @@ SYSCALL_DEFINE0(uretprobe)
* trampoline's ret instruction
*/
r11_cx_ax[2] = regs->ip;
+
+ /* make the shadow stack follow that */
+ if (shstk_push_frame(regs->ip))
+ goto sigill;
+
regs->ip = ip;
err = copy_to_user((void __user *)regs->sp, r11_cx_ax, sizeof(r11_cx_ax));
@@ -1191,8 +1196,13 @@ arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr, struct pt_regs
return orig_ret_vaddr;
nleft = copy_to_user((void __user *)regs->sp, &trampoline_vaddr, rasize);
- if (likely(!nleft))
+ if (likely(!nleft)) {
+ if (shstk_update_last_frame(trampoline_vaddr)) {
+ force_sig(SIGSEGV);
+ return -1;
+ }
return orig_ret_vaddr;
+ }
if (nleft != rasize) {
pr_err("return address clobbered: pid=%d, %%sp=%#lx, %%ip=%#lx\n",
--
2.44.0
^ permalink raw reply related
* [PATCHv5 bpf-next 7/8] selftests/x86: Add return uprobe shadow stack test
From: Jiri Olsa @ 2024-05-07 10:53 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240507105321.71524-1-jolsa@kernel.org>
Adding return uprobe test for shadow stack and making sure it's
working properly. Borrowed some of the code from bpf selftests.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
.../testing/selftests/x86/test_shadow_stack.c | 142 ++++++++++++++++++
1 file changed, 142 insertions(+)
diff --git a/tools/testing/selftests/x86/test_shadow_stack.c b/tools/testing/selftests/x86/test_shadow_stack.c
index 757e6527f67e..1b919baa999b 100644
--- a/tools/testing/selftests/x86/test_shadow_stack.c
+++ b/tools/testing/selftests/x86/test_shadow_stack.c
@@ -34,6 +34,7 @@
#include <sys/ptrace.h>
#include <sys/signal.h>
#include <linux/elf.h>
+#include <linux/perf_event.h>
/*
* Define the ABI defines if needed, so people can run the tests
@@ -681,6 +682,141 @@ int test_32bit(void)
return !segv_triggered;
}
+static int parse_uint_from_file(const char *file, const char *fmt)
+{
+ int err, ret;
+ FILE *f;
+
+ f = fopen(file, "re");
+ if (!f) {
+ err = -errno;
+ printf("failed to open '%s': %d\n", file, err);
+ return err;
+ }
+ err = fscanf(f, fmt, &ret);
+ if (err != 1) {
+ err = err == EOF ? -EIO : -errno;
+ printf("failed to parse '%s': %d\n", file, err);
+ fclose(f);
+ return err;
+ }
+ fclose(f);
+ return ret;
+}
+
+static int determine_uprobe_perf_type(void)
+{
+ const char *file = "/sys/bus/event_source/devices/uprobe/type";
+
+ return parse_uint_from_file(file, "%d\n");
+}
+
+static int determine_uprobe_retprobe_bit(void)
+{
+ const char *file = "/sys/bus/event_source/devices/uprobe/format/retprobe";
+
+ return parse_uint_from_file(file, "config:%d\n");
+}
+
+static ssize_t get_uprobe_offset(const void *addr)
+{
+ size_t start, end, base;
+ char buf[256];
+ bool found = false;
+ FILE *f;
+
+ f = fopen("/proc/self/maps", "r");
+ if (!f)
+ return -errno;
+
+ while (fscanf(f, "%zx-%zx %s %zx %*[^\n]\n", &start, &end, buf, &base) == 4) {
+ if (buf[2] == 'x' && (uintptr_t)addr >= start && (uintptr_t)addr < end) {
+ found = true;
+ break;
+ }
+ }
+
+ fclose(f);
+
+ if (!found)
+ return -ESRCH;
+
+ return (uintptr_t)addr - start + base;
+}
+
+static __attribute__((noinline)) void uretprobe_trigger(void)
+{
+ asm volatile ("");
+}
+
+/*
+ * This test setups return uprobe, which is sensitive to shadow stack
+ * (crashes without extra fix). After executing the uretprobe we fail
+ * the test if we receive SIGSEGV, no crash means we're good.
+ *
+ * Helper functions above borrowed from bpf selftests.
+ */
+static int test_uretprobe(void)
+{
+ const size_t attr_sz = sizeof(struct perf_event_attr);
+ const char *file = "/proc/self/exe";
+ int bit, fd = 0, type, err = 1;
+ struct perf_event_attr attr;
+ struct sigaction sa = {};
+ ssize_t offset;
+
+ type = determine_uprobe_perf_type();
+ if (type < 0)
+ return 1;
+
+ offset = get_uprobe_offset(uretprobe_trigger);
+ if (offset < 0)
+ return 1;
+
+ bit = determine_uprobe_retprobe_bit();
+ if (bit < 0)
+ return 1;
+
+ sa.sa_sigaction = segv_gp_handler;
+ sa.sa_flags = SA_SIGINFO;
+ if (sigaction(SIGSEGV, &sa, NULL))
+ return 1;
+
+ /* Setup return uprobe through perf event interface. */
+ memset(&attr, 0, attr_sz);
+ attr.size = attr_sz;
+ attr.type = type;
+ attr.config = 1 << bit;
+ attr.config1 = (__u64) (unsigned long) file;
+ attr.config2 = offset;
+
+ fd = syscall(__NR_perf_event_open, &attr, 0 /* pid */, -1 /* cpu */,
+ -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
+ if (fd < 0)
+ goto out;
+
+ if (sigsetjmp(jmp_buffer, 1))
+ goto out;
+
+ ARCH_PRCTL(ARCH_SHSTK_ENABLE, ARCH_SHSTK_SHSTK);
+
+ /*
+ * This either segfaults and goes through sigsetjmp above
+ * or succeeds and we're good.
+ */
+ uretprobe_trigger();
+
+ printf("[OK]\tUretprobe test\n");
+ err = 0;
+
+out:
+ ARCH_PRCTL(ARCH_SHSTK_DISABLE, ARCH_SHSTK_SHSTK);
+ signal(SIGSEGV, SIG_DFL);
+ if (fd)
+ close(fd);
+ return err;
+}
+
void segv_handler_ptrace(int signum, siginfo_t *si, void *uc)
{
/* The SSP adjustment caused a segfault. */
@@ -867,6 +1003,12 @@ int main(int argc, char *argv[])
goto out;
}
+ if (test_uretprobe()) {
+ ret = 1;
+ printf("[FAIL]\turetprobe test\n");
+ goto out;
+ }
+
return ret;
out:
--
2.44.0
^ permalink raw reply related
* [PATCHv5 8/8] man2: Add uretprobe syscall page
From: Jiri Olsa @ 2024-05-07 10:53 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu, Oleg Nesterov,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: linux-kernel, linux-trace-kernel, linux-api, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240507105321.71524-1-jolsa@kernel.org>
Adding man page for new uretprobe syscall.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
man2/uretprobe.2 | 50 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 50 insertions(+)
create mode 100644 man2/uretprobe.2
diff --git a/man2/uretprobe.2 b/man2/uretprobe.2
new file mode 100644
index 000000000000..690fe3b1a44f
--- /dev/null
+++ b/man2/uretprobe.2
@@ -0,0 +1,50 @@
+.\" Copyright (C) 2024, Jiri Olsa <jolsa@kernel.org>
+.\"
+.\" SPDX-License-Identifier: Linux-man-pages-copyleft
+.\"
+.TH uretprobe 2 (date) "Linux man-pages (unreleased)"
+.SH NAME
+uretprobe \- execute pending return uprobes
+.SH SYNOPSIS
+.nf
+.B int uretprobe(void)
+.fi
+.SH DESCRIPTION
+The
+.BR uretprobe ()
+syscall is an alternative to breakpoint instructions for
+triggering return uprobe consumers.
+.P
+Calls to
+.BR uretprobe ()
+suscall are only made from the user-space trampoline provided by the kernel.
+Calls from any other place result in a
+.BR SIGILL .
+
+.SH RETURN VALUE
+The
+.BR uretprobe ()
+syscall return value is architecture-specific.
+
+.SH VERSIONS
+This syscall is not specified in POSIX,
+and details of its behavior vary across systems.
+.SH STANDARDS
+None.
+.SH HISTORY
+TBD
+.SH NOTES
+The
+.BR uretprobe ()
+syscall was initially introduced for the x86_64 architecture where it was shown
+to be faster than breakpoint traps. It might be extended to other architectures.
+.P
+The
+.BR uretprobe ()
+syscall exists only to allow the invocation of return uprobe consumers.
+It should
+.B never
+be called directly.
+Details of the arguments (if any) passed to
+.BR uretprobe ()
+and the return value are architecture-specific.
--
2.44.0
^ permalink raw reply related
* Re: [PATCH v6 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: Aleksa Sarai @ 2024-05-07 11:58 UTC (permalink / raw)
To: stsp
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: <5b5cc31f-a5be-4f64-a97b-7708466ace82@yandex.ru>
[-- Attachment #1: Type: text/plain, Size: 1758 bytes --]
On 2024-05-07, stsp <stsp2@yandex.ru> wrote:
> 07.05.2024 10:50, Aleksa Sarai пишет:
> > If you are a privileged process which plans to change users,
>
> Not privileged at all. But I think what you say is still possible with
> userns?
It is possible to configure MOUNT_ATTR_IDMAP in a user namespace but
there are some restrictions that I suspect will make this complicated.
If you try to do something with a regular filesystem you'll probably run
into issues because you won't have CAP_SYS_ADMIN in the super block's
userns. But you could probably do it with tmpfs.
> > A new attack I just thought of while writing this mail is that because
> > there is no RESOLVE_NO_XDEV requirement, it should be possible for the
> > process to get an arbitrary write primitive by creating a new
> > userns+mountns and then bind-mounting / underneath the directory.
> Doesn't this need a write perm to a
> directory? In his case this is not a threat,
> because you are not supposed to have a
> write perm to that dir. OA2_CRED_INHERIT
> is the only way to write.
No, bind-mounts don't require write permission. As long as you can
resolve the target path you can bind-mount on top of it, so if there's a
subdirectory you can bind-mount / underneath (and if there is only a
file you can bind-mount any file you want to access/overwrite instead).
There are restrictions on mounting through /proc/self/fd/... but they
don't apply here (all files opened by a process doing setns/unshare have
their vfsmounts updated to be from the new mount namespace, meaning you
can do mounts through them with /proc/self/fd/... without issue.)
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v6 0/3] implement OA2_CRED_INHERIT flag for openat2()
From: stsp @ 2024-05-07 12:48 UTC (permalink / raw)
To: Aleksa Sarai
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: <20240507.110127-muggy.duff.trained.hobby-u9ZNUZ9CW5k@cyphar.com>
07.05.2024 14:58, Aleksa Sarai пишет:
> On 2024-05-07, stsp <stsp2@yandex.ru> wrote:
>> 07.05.2024 10:50, Aleksa Sarai пишет:
>>> If you are a privileged process which plans to change users,
>> Not privileged at all. But I think what you say is still possible with
>> userns?
> It is possible to configure MOUNT_ATTR_IDMAP in a user namespace but
> there are some restrictions that I suspect will make this complicated.
> If you try to do something with a regular filesystem you'll probably run
> into issues because you won't have CAP_SYS_ADMIN in the super block's
> userns. But you could probably do it with tmpfs.
Then its likely not a replacement for
my proposal, as I really don't need that
on tmpfs.
Perhaps right now I can use the helper
process and an rpc as a replacement.
This is much more work and is slower,
but more or less can approximate my
original design decision quite precisely.
Another disadvantage of an rpc approach
is that the fds I get from the helper
process, can not be trusted, as in this
case kernel doesn't guarantee the fd
actually refers to the resource I requested.
I've seen a few OSes where rpc is checked
by a trusted entity to avoid such problem.
>>> A new attack I just thought of while writing this mail is that because
>>> there is no RESOLVE_NO_XDEV requirement, it should be possible for the
>>> process to get an arbitrary write primitive by creating a new
>>> userns+mountns and then bind-mounting / underneath the directory.
>> Doesn't this need a write perm to a
>> directory? In his case this is not a threat,
>> because you are not supposed to have a
>> write perm to that dir. OA2_CRED_INHERIT
>> is the only way to write.
> No, bind-mounts don't require write permission.
Oh, isn't this a problem by itself?
Yes, in this case my patch needs to
avoid RESOLVE_NO_XDEV, but I find this a harsh restriction. Maybe the
bind mount was done before a priv drop? Then it is fully legitimate.
Anyway, I don't know if I should work on it or not, as there seem to be
no indication of a possible acceptance.
^ permalink raw reply
* Re: [PATCHv5 bpf-next 5/8] selftests/bpf: Add uretprobe syscall call from user space test
From: Andrii Nakryiko @ 2024-05-07 16:57 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, linux-man, x86, bpf,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov (AMD), Ingo Molnar,
Andy Lutomirski, Edgecombe, Rick P, Deepak Gupta
In-Reply-To: <20240507105321.71524-6-jolsa@kernel.org>
On Tue, May 7, 2024 at 3:54 AM 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 not executed.
>
> Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> .../selftests/bpf/prog_tests/uprobe_syscall.c | 95 +++++++++++++++++++
> .../bpf/progs/uprobe_syscall_executed.c | 17 ++++
> 2 files changed, 112 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscall_executed.c
>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
> diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> index 1a50cd35205d..3ef324c2db50 100644
> --- a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> @@ -7,7 +7,10 @@
> #include <unistd.h>
> #include <asm/ptrace.h>
> #include <linux/compiler.h>
> +#include <linux/stringify.h>
> +#include <sys/wait.h>
> #include "uprobe_syscall.skel.h"
> +#include "uprobe_syscall_executed.skel.h"
>
[...]
^ permalink raw reply
* Re: [PATCHv5 bpf-next 6/8] x86/shstk: Add return uprobe support
From: Edgecombe, Rick P @ 2024-05-07 17:35 UTC (permalink / raw)
To: jolsa@kernel.org, mhiramat@kernel.org, rostedt@goodmis.org,
ast@kernel.org, andrii@kernel.org, oleg@redhat.com,
daniel@iogearbox.net
Cc: debug@rivosinc.com, luto@kernel.org, bp@alien8.de, yhs@fb.com,
songliubraving@fb.com, linux-api@vger.kernel.org, x86@kernel.org,
linux-kernel@vger.kernel.org, john.fastabend@gmail.com,
tglx@linutronix.de, mingo@redhat.com,
linux-trace-kernel@vger.kernel.org, bpf@vger.kernel.org,
linux-man@vger.kernel.org, peterz@infradead.org
In-Reply-To: <20240507105321.71524-7-jolsa@kernel.org>
On Tue, 2024-05-07 at 12:53 +0200, Jiri Olsa wrote:
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 81e6ee95784d..ae6c3458a675 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -406,6 +406,11 @@ SYSCALL_DEFINE0(uretprobe)
> * trampoline's ret instruction
> */
> r11_cx_ax[2] = regs->ip;
> +
> + /* make the shadow stack follow that */
> + if (shstk_push_frame(regs->ip))
> + goto sigill;
> +
> regs->ip = ip;
>
Per the earlier discussion, this cannot be reached unless uretprobes are in use,
which cannot happen without something with privileges taking an action. But are
uretprobes ever used for monitoring applications where security is important? Or
is it strictly a debug-time thing?
^ 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