* Re: Crash when attaching uretprobes to processes running in Docker
From: Oleg Nesterov @ 2025-01-15 15:06 UTC (permalink / raw)
To: Jiri Olsa
Cc: Eyal Birger, Aleksa Sarai, mhiramat, linux-kernel,
linux-trace-kernel, BPF-dev-list, Song Liu, Yonghong Song,
John Fastabend, peterz, tglx, bp, x86, linux-api, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
rostedt@goodmis.org, rafi, Shmulik Ladkani
In-Reply-To: <Z4eBs0-kJ3iVZjXL@krava>
On 01/15, Jiri Olsa wrote:
>
> On Tue, Jan 14, 2025 at 06:25:20PM +0100, Oleg Nesterov wrote:
> >
> > Sorry, I don't understand... What exactly we can do? Aside from checking
> > IS_ENABLED(CONFIG_SECCOMP) in arch_uprobe_trampoline() ?
>
> I need to check more on seccomp, but I imagine we could do following:
> - when seccomp filter is installed we could check uprobe trampoline
> and if it's already installed we change it to int3 trampoline
> - when uprobe trampoline is getting installed we check if there's
> seccomp filter installed for task and we use int3 trampoline
I still don't understand... But whatever you meant, I doubt it can work.
> other than that I guess we will have to add sysctl to enable uretprobe
> trampoline..
Or we can change __secure_computing() to do nothing if
this_syscall == __NR_uretprobe. Or even change syscall_trace_enter/exit
to do this check.
But I don't really like this idea, I don't feel this is the right solution...
Oleg.
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Jiri Olsa @ 2025-01-15 13:25 UTC (permalink / raw)
To: Jiri Olsa
Cc: Oleg Nesterov, Eyal Birger, Aleksa Sarai, mhiramat, linux-kernel,
linux-trace-kernel, BPF-dev-list, Song Liu, Yonghong Song,
John Fastabend, peterz, tglx, bp, x86, linux-api, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
rostedt@goodmis.org, rafi, Shmulik Ladkani
In-Reply-To: <Z4eBs0-kJ3iVZjXL@krava>
On Wed, Jan 15, 2025 at 10:36:51AM +0100, Jiri Olsa wrote:
> On Tue, Jan 14, 2025 at 06:25:20PM +0100, Oleg Nesterov wrote:
> > On 01/14, Jiri Olsa wrote:
> > >
> > > ugh.. could we just 'disable' uretprobe trampoline when seccomp gets enabled?
> > > overwrite first byte with int3.. and similarly check on seccomp when installing
> > > uretprobe and switch to int3
> >
> > Sorry, I don't understand... What exactly we can do? Aside from checking
> > IS_ENABLED(CONFIG_SECCOMP) in arch_uprobe_trampoline() ?
>
> I need to check more on seccomp, but I imagine we could do following:
> - when seccomp filter is installed we could check uprobe trampoline
> and if it's already installed we change it to int3 trampoline
nah, other threads could be in there already.. :-\
jirka
> - when uprobe trampoline is getting installed we check if there's
> seccomp filter installed for task and we use int3 trampoline
>
> other than that I guess we will have to add sysctl to enable uretprobe
> trampoline..
>
> jirka
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Eyal Birger @ 2025-01-15 13:24 UTC (permalink / raw)
To: Jiri Olsa
Cc: Oleg Nesterov, Aleksa Sarai, mhiramat, linux-kernel,
linux-trace-kernel, BPF-dev-list, Song Liu, Yonghong Song,
John Fastabend, peterz, tglx, bp, x86, linux-api, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
rostedt@goodmis.org, rafi, Shmulik Ladkani
In-Reply-To: <Z4eBs0-kJ3iVZjXL@krava>
Hi,
On Wed, Jan 15, 2025 at 1:36 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Tue, Jan 14, 2025 at 06:25:20PM +0100, Oleg Nesterov wrote:
> > On 01/14, Jiri Olsa wrote:
> > >
> > > ugh.. could we just 'disable' uretprobe trampoline when seccomp gets enabled?
> > > overwrite first byte with int3.. and similarly check on seccomp when installing
> > > uretprobe and switch to int3
> >
> > Sorry, I don't understand... What exactly we can do? Aside from checking
> > IS_ENABLED(CONFIG_SECCOMP) in arch_uprobe_trampoline() ?
>
> I need to check more on seccomp, but I imagine we could do following:
> - when seccomp filter is installed we could check uprobe trampoline
> and if it's already installed we change it to int3 trampoline
> - when uprobe trampoline is getting installed we check if there's
> seccomp filter installed for task and we use int3 trampoline
Sounds reasonable to me.
I'm wondering how hard it is to figure out the seccomp installation
given that from what I understand it's inherited.
>
> other than that I guess we will have to add sysctl to enable uretprobe
> trampoline..
I'm wondering when one would enable/disable such sysctl.
"Give me speed but potentially crash processes I don't control"
is a curious semantic...
Eyal
>
> jirka
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Jiri Olsa @ 2025-01-15 9:36 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Jiri Olsa, Eyal Birger, Aleksa Sarai, mhiramat, linux-kernel,
linux-trace-kernel, BPF-dev-list, Song Liu, Yonghong Song,
John Fastabend, peterz, tglx, bp, x86, linux-api, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
rostedt@goodmis.org, rafi, Shmulik Ladkani
In-Reply-To: <20250114172519.GB29305@redhat.com>
On Tue, Jan 14, 2025 at 06:25:20PM +0100, Oleg Nesterov wrote:
> On 01/14, Jiri Olsa wrote:
> >
> > ugh.. could we just 'disable' uretprobe trampoline when seccomp gets enabled?
> > overwrite first byte with int3.. and similarly check on seccomp when installing
> > uretprobe and switch to int3
>
> Sorry, I don't understand... What exactly we can do? Aside from checking
> IS_ENABLED(CONFIG_SECCOMP) in arch_uprobe_trampoline() ?
I need to check more on seccomp, but I imagine we could do following:
- when seccomp filter is installed we could check uprobe trampoline
and if it's already installed we change it to int3 trampoline
- when uprobe trampoline is getting installed we check if there's
seccomp filter installed for task and we use int3 trampoline
other than that I guess we will have to add sysctl to enable uretprobe
trampoline..
jirka
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Shmulik Ladkani @ 2025-01-15 5:45 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Eyal Birger, Andrii Nakryiko, Jiri Olsa, Sarai Aleksa, mhiramat,
linux-kernel, linux-trace-kernel, BPF-dev-list, Song Liu,
Yonghong Song, John Fastabend, peterz, tglx, bp, x86, linux-api,
Andrii Nakryiko, Daniel Borkmann, Alexei Starovoitov, rostedt,
rafi
In-Reply-To: <20250115005012.GA10946@redhat.com>
On Wed, 15 Jan 2025 01:50:13 +0100 Oleg Nesterov <oleg@redhat.com>
wrote:
> On 01/14, Eyal Birger wrote:
> >
> > Its software, that’s working fine in previous kernel versions and
> > upon upgrade starts creating crashes in other processes.
> >
> > IMHO demanding that other software (e.g docker) be upgraded in
> > order to run on a newer kernel is not what Linux formerly
> > guaranteed.
>
> Agreed.
IMO There are 2 problematic aspects with ff474a78cef5
("uprobe: Add uretprobe syscall to speed up return probe").
The first, as Eyal mentioned, is the kernel regression: There are
endless systems out there (iaas and paas) that have both
telementry/instrumentation/tracing software (utilizing uprobes) and
container environments (duch as docker) that enforce syscall
restrictions on their workloads.
These systems worked so far, and with kernels having ff474a78cef5 the
workloads processes fault.
The second, is the fact that ff474a78cef5 (which adds a new syscall
invocation to the uretprobe trampoline) *exposes an internal kernel
implementation* to the userspace system:
There are millions of binaries/libraries out there that *never issue*
the new syscall: they simply do not have that call in their
instructions. Take for example hello-world.
However, once hello-world is traced (with software utilizing
uprobes) hello-world *unknowingly* DO issue the new syscall, just
because the kernel decided to implement its uretprobe trampoline using
a new syscall - a mechanism that should be completely transparent and
seamless to the user program.
This is totally unexpected, and to ask a system admin to "guess" whether
hello-world is "going to issue the syscall despite the fact that
such invocation does not exist in its own code at all" (and set seccomp
permissions accordingly) is asking for the admin to know the exact
*internal mechanisms* that the kernel use for implemeting the
trampolines.
Just like we won't add a div-by-zero fault to the trampoline, we
shoudn't add any instruction (such as a syscall) that isn't *completely
transparent* to the userspace program.
Best,
Shmulik
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Oleg Nesterov @ 2025-01-15 0:50 UTC (permalink / raw)
To: Eyal Birger
Cc: Andrii Nakryiko, Jiri Olsa, Sarai Aleksa, mhiramat, linux-kernel,
linux-trace-kernel, BPF-dev-list, Song Liu, Yonghong Song,
John Fastabend, peterz, tglx, bp, x86, linux-api, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, rostedt, rafi,
Shmulik Ladkani
In-Reply-To: <EBE7D529-5418-4BD6-B9B5-64BE0FBE8569@gmail.com>
On 01/14, Eyal Birger wrote:
>
> Its software, that’s working fine in previous kernel versions and upon
> upgrade starts creating crashes in other processes.
>
> IMHO demanding that other software (e.g docker) be upgraded in order to
> run on a newer kernel is not what Linux formerly guaranteed.
Agreed.
Oleg.
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Eyal Birger @ 2025-01-15 0:09 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Oleg Nesterov, Jiri Olsa, Sarai Aleksa, mhiramat, linux-kernel,
linux-trace-kernel, BPF-dev-list, Song Liu, Yonghong Song,
John Fastabend, peterz, tglx, bp, x86, linux-api, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, rostedt, rafi,
Shmulik Ladkani
In-Reply-To: <CAEf4BzZquQBW1DuEmfhUTicoyHOeEpT6FG7VBR-kG35f7Rb5Zw@mail.gmail.com>
Hi,
> On Jan 14, 2025, at 15:52, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Jan 14, 2025 at 2:11 PM Oleg Nesterov <oleg@redhat.com> wrote:
>>
>>> On 01/14, Andrii Nakryiko wrote:
>>>
>>> On Tue, Jan 14, 2025 at 12:40 PM Oleg Nesterov <oleg@redhat.com> wrote:
>>>>
>>>> But, unlike sys_uretprobe(), sys_rt_sigreturn() is old, so the existing
>>>> setups must know that sigreturn() should be respected...
>>>
>>> someday sys_uretprobe will be old as well ;) FWIW, systemd allowlisted
>>> sys_uretprobe, see [0]
>>
>> And I agree! ;)
>>
>> I mean, I'd personally prefer to do nothing and wait until userspace figures
>> out that we have another "special" syscall.
>>
>> But can we do it? I simply do not know. Can we ignore this (valid) bug report?
>>
>
> Seems wrong for kernel to try to guess whether some syscall is
> filtered by some policy or not (though maybe I'm misunderstanding the
> details and it's kernel-originated problem?). Seems like a recipe for
> more problems.
>
> Nothing is really fundamentally broken. Some piece of software needs
> an upgraded library to not disable the kernel's special syscall (just
> like sys_rt_sigreturn, nothing "new" here, really). Users can't do
> uprobing in such broken setups (but not in general), seems like a good
> incentive for everyone to push for the right thing here: fixed up to
> date software.
It’s not “users” doing the uprobing in this case.
Its software, that’s working fine in previous kernel versions and upon upgrade starts creating crashes in other processes.
IMHO demanding that other software (e.g docker) be upgraded in order to run on a newer kernel is not what Linux formerly guaranteed.
Eyal
>
>> Oleg.
>>
^ permalink raw reply
* Re: [PATCH] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Darrick J. Wong @ 2025-01-15 0:05 UTC (permalink / raw)
To: Andrey Albershteyn
Cc: linux-fsdevel, linux-api, monstr, mpe, npiggin, christophe.leroy,
naveen, maddy, luto, tglx, mingo, bp, dave.hansen, x86, hpa,
chris, jcmvbkbc, viro, brauner, jack, arnd, linux-alpha,
linux-kernel, linux-m68k, linux-parisc, linuxppc-dev, linux-s390,
linux-sh, sparclinux, linux-security-module, linux-arch
In-Reply-To: <20250109174540.893098-1-aalbersh@kernel.org>
On Thu, Jan 09, 2025 at 06:45:40PM +0100, Andrey Albershteyn wrote:
> From: Andrey Albershteyn <aalbersh@redhat.com>
>
> Introduce getfsxattrat and setfsxattrat syscalls to manipulate inode
> extended attributes/flags. The syscalls take parent directory FD and
> path to the child together with struct fsxattr.
>
> This is an alternative to FS_IOC_FSSETXATTR ioctl with a difference
> that file don't need to be open. By having this we can manipulated
> inode extended attributes not only on normal files but also on
> special ones. This is not possible with FS_IOC_FSSETXATTR ioctl as
> opening special files returns VFS special inode instead of
> underlying filesystem one.
>
> This patch adds two new syscalls which allows userspace to set
> extended inode attributes on special files by using parent directory
> to open FS inode.
>
> Also, as vfs_fileattr_set() is now will be called on special files
> too, let's forbid any other attributes except projid and nextents
> (symlink can have an extent).
>
> CC: linux-api@vger.kernel.org
> Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
> ---
>
> Notes:
> Previous discussion:
> https://lore.kernel.org/linux-xfs/20240520164624.665269-2-aalbersh@redhat.com/
>
> XFS has project quotas which could be attached to a directory. All
> new inodes in these directories inherit project ID set on parent
> directory.
>
> The project is created from userspace by opening and calling
> FS_IOC_FSSETXATTR on each inode. This is not possible for special
> files such as FIFO, SOCK, BLK etc. Therefore, some inodes are left
> with empty project ID. Those inodes then are not shown in the quota
> accounting but still exist in the directory. Moreover, in the case
> when special files are created in the directory with already
> existing project quota, these inode inherit extended attributes.
> This than leaves them with these attributes without the possibility
> to clear them out. This, in turn, prevents userspace from
> re-creating quota project on these existing files.
>
> arch/alpha/kernel/syscalls/syscall.tbl | 2 +
> arch/m68k/kernel/syscalls/syscall.tbl | 2 +
> arch/microblaze/kernel/syscalls/syscall.tbl | 2 +
> arch/parisc/kernel/syscalls/syscall.tbl | 2 +
> arch/powerpc/kernel/syscalls/syscall.tbl | 2 +
> arch/s390/kernel/syscalls/syscall.tbl | 2 +
> arch/sh/kernel/syscalls/syscall.tbl | 2 +
> arch/sparc/kernel/syscalls/syscall.tbl | 2 +
> arch/x86/entry/syscalls/syscall_32.tbl | 2 +
> arch/x86/entry/syscalls/syscall_64.tbl | 2 +
> arch/xtensa/kernel/syscalls/syscall.tbl | 2 +
> fs/inode.c | 105 ++++++++++++++++++++
> fs/ioctl.c | 17 +++-
> include/linux/fileattr.h | 1 +
> include/linux/syscalls.h | 4 +
> include/uapi/asm-generic/unistd.h | 8 +-
> 16 files changed, 154 insertions(+), 3 deletions(-)
>
> diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl
> index c59d53d6d3f3..4b9e687494c1 100644
> --- a/arch/alpha/kernel/syscalls/syscall.tbl
> +++ b/arch/alpha/kernel/syscalls/syscall.tbl
> @@ -506,3 +506,5 @@
> 574 common getxattrat sys_getxattrat
> 575 common listxattrat sys_listxattrat
> 576 common removexattrat sys_removexattrat
> +577 common getfsxattrat sys_getfsxattrat
> +578 common setfsxattrat sys_setfsxattrat
> diff --git a/arch/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl
> index f5ed71f1910d..159476387f39 100644
> --- a/arch/m68k/kernel/syscalls/syscall.tbl
> +++ b/arch/m68k/kernel/syscalls/syscall.tbl
> @@ -466,3 +466,5 @@
> 464 common getxattrat sys_getxattrat
> 465 common listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat
> +467 common getfsxattrat sys_getfsxattrat
> +468 common setfsxattrat sys_setfsxattrat
> diff --git a/arch/microblaze/kernel/syscalls/syscall.tbl b/arch/microblaze/kernel/syscalls/syscall.tbl
> index 680f568b77f2..a6d59ee740b5 100644
> --- a/arch/microblaze/kernel/syscalls/syscall.tbl
> +++ b/arch/microblaze/kernel/syscalls/syscall.tbl
> @@ -472,3 +472,5 @@
> 464 common getxattrat sys_getxattrat
> 465 common listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat
> +467 common getfsxattrat sys_getfsxattrat
> +468 common setfsxattrat sys_setfsxattrat
> diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl
> index d9fc94c86965..b3578fac43d6 100644
> --- a/arch/parisc/kernel/syscalls/syscall.tbl
> +++ b/arch/parisc/kernel/syscalls/syscall.tbl
> @@ -465,3 +465,5 @@
> 464 common getxattrat sys_getxattrat
> 465 common listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat
> +467 common getfsxattrat sys_getfsxattrat
> +468 common setfsxattrat sys_setfsxattrat
> diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
> index d8b4ab78bef0..808045d82c94 100644
> --- a/arch/powerpc/kernel/syscalls/syscall.tbl
> +++ b/arch/powerpc/kernel/syscalls/syscall.tbl
> @@ -557,3 +557,5 @@
> 464 common getxattrat sys_getxattrat
> 465 common listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat
> +467 common getfsxattrat sys_getfsxattrat
> +468 common setfsxattrat sys_setfsxattrat
> diff --git a/arch/s390/kernel/syscalls/syscall.tbl b/arch/s390/kernel/syscalls/syscall.tbl
> index e9115b4d8b63..78dfc2c184d4 100644
> --- a/arch/s390/kernel/syscalls/syscall.tbl
> +++ b/arch/s390/kernel/syscalls/syscall.tbl
> @@ -469,3 +469,5 @@
> 464 common getxattrat sys_getxattrat sys_getxattrat
> 465 common listxattrat sys_listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat sys_removexattrat
> +467 common getfsxattrat sys_getfsxattrat sys_getfsxattrat
> +468 common setfsxattrat sys_setfsxattrat sys_setfsxattrat
> diff --git a/arch/sh/kernel/syscalls/syscall.tbl b/arch/sh/kernel/syscalls/syscall.tbl
> index c8cad33bf250..d5a5c8339f0e 100644
> --- a/arch/sh/kernel/syscalls/syscall.tbl
> +++ b/arch/sh/kernel/syscalls/syscall.tbl
> @@ -470,3 +470,5 @@
> 464 common getxattrat sys_getxattrat
> 465 common listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat
> +467 common getfsxattrat sys_getfsxattrat
> +468 common setfsxattrat sys_setfsxattrat
> diff --git a/arch/sparc/kernel/syscalls/syscall.tbl b/arch/sparc/kernel/syscalls/syscall.tbl
> index 727f99d333b3..817dcd8603bc 100644
> --- a/arch/sparc/kernel/syscalls/syscall.tbl
> +++ b/arch/sparc/kernel/syscalls/syscall.tbl
> @@ -512,3 +512,5 @@
> 464 common getxattrat sys_getxattrat
> 465 common listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat
> +467 common getfsxattrat sys_getfsxattrat
> +468 common setfsxattrat sys_setfsxattrat
> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
> index 4d0fb2fba7e2..b4842c027c5d 100644
> --- a/arch/x86/entry/syscalls/syscall_32.tbl
> +++ b/arch/x86/entry/syscalls/syscall_32.tbl
> @@ -472,3 +472,5 @@
> 464 i386 getxattrat sys_getxattrat
> 465 i386 listxattrat sys_listxattrat
> 466 i386 removexattrat sys_removexattrat
> +467 i386 getfsxattrat sys_getfsxattrat
> +468 i386 setfsxattrat sys_setfsxattrat
> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
> index 5eb708bff1c7..b6f0a7236aae 100644
> --- a/arch/x86/entry/syscalls/syscall_64.tbl
> +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> @@ -390,6 +390,8 @@
> 464 common getxattrat sys_getxattrat
> 465 common listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat
> +467 common getfsxattrat sys_getfsxattrat
> +468 common setfsxattrat sys_setfsxattrat
>
> #
> # Due to a historical design error, certain syscalls are numbered differently
> diff --git a/arch/xtensa/kernel/syscalls/syscall.tbl b/arch/xtensa/kernel/syscalls/syscall.tbl
> index 37effc1b134e..425d56be337d 100644
> --- a/arch/xtensa/kernel/syscalls/syscall.tbl
> +++ b/arch/xtensa/kernel/syscalls/syscall.tbl
> @@ -437,3 +437,5 @@
> 464 common getxattrat sys_getxattrat
> 465 common listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat
> +467 common getfsxattrat sys_getfsxattrat
> +468 common setfsxattrat sys_setfsxattrat
> diff --git a/fs/inode.c b/fs/inode.c
> index 6b4c77268fc0..fc8939c6c8a7 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -23,6 +23,9 @@
> #include <linux/rw_hint.h>
> #include <linux/seq_file.h>
> #include <linux/debugfs.h>
> +#include <linux/syscalls.h>
> +#include <linux/fileattr.h>
> +#include <linux/namei.h>
> #include <trace/events/writeback.h>
> #define CREATE_TRACE_POINTS
> #include <trace/events/timestamp.h>
> @@ -2953,3 +2956,105 @@ umode_t mode_strip_sgid(struct mnt_idmap *idmap,
> return mode & ~S_ISGID;
> }
> EXPORT_SYMBOL(mode_strip_sgid);
> +
> +SYSCALL_DEFINE4(getfsxattrat, int, dfd, const char __user *, filename,
> + struct fsxattr *, fsx, int, at_flags)
> +{
> + struct fd dir;
> + struct fileattr fa;
> + struct path filepath;
> + struct inode *inode;
> + int error;
> +
> + if (at_flags)
> + return -EINVAL;
> +
> + if (!capable(CAP_FOWNER))
> + return -EPERM;
> +
> + dir = fdget(dfd);
> + if (!fd_file(dir))
> + return -EBADF;
> +
> + if (!S_ISDIR(file_inode(fd_file(dir))->i_mode)) {
> + error = -EBADF;
> + goto out;
> + }
> +
> + error = user_path_at(dfd, filename, at_flags, &filepath);
Same comments as Jan, but I wanted to point out that the third argument
to user_path_at() is LOOKUP_*, not AT_*. Right now you don't allow any
AT_* flags, but that's something to fix before the next revision.
--D
> + if (error)
> + goto out;
> +
> + inode = filepath.dentry->d_inode;
> + if (file_inode(fd_file(dir))->i_sb->s_magic != inode->i_sb->s_magic) {
> + error = -EBADF;
> + goto out_path;
> + }
> +
> + error = vfs_fileattr_get(filepath.dentry, &fa);
> + if (error)
> + goto out_path;
> +
> + if (copy_fsxattr_to_user(&fa, fsx))
> + error = -EFAULT;
> +
> +out_path:
> + path_put(&filepath);
> +out:
> + fdput(dir);
> + return error;
> +}
> +
> +SYSCALL_DEFINE4(setfsxattrat, int, dfd, const char __user *, filename,
> + struct fsxattr *, fsx, int, at_flags)
> +{
> + struct fd dir;
> + struct fileattr fa;
> + struct inode *inode;
> + struct path filepath;
> + int error;
> +
> + if (at_flags)
> + return -EINVAL;
> +
> + if (!capable(CAP_FOWNER))
> + return -EPERM;
> +
> + dir = fdget(dfd);
> + if (!fd_file(dir))
> + return -EBADF;
> +
> + if (!S_ISDIR(file_inode(fd_file(dir))->i_mode)) {
> + error = -EBADF;
> + goto out;
> + }
> +
> + if (copy_fsxattr_from_user(&fa, fsx)) {
> + error = -EFAULT;
> + goto out;
> + }
> +
> + error = user_path_at(dfd, filename, at_flags, &filepath);
> + if (error)
> + goto out;
> +
> + inode = filepath.dentry->d_inode;
> + if (file_inode(fd_file(dir))->i_sb->s_magic != inode->i_sb->s_magic) {
> + error = -EBADF;
> + goto out_path;
> + }
> +
> + error = mnt_want_write(filepath.mnt);
> + if (error)
> + goto out_path;
> +
> + error = vfs_fileattr_set(file_mnt_idmap(fd_file(dir)), filepath.dentry,
> + &fa);
> + mnt_drop_write(filepath.mnt);
> +
> +out_path:
> + path_put(&filepath);
> +out:
> + fdput(dir);
> + return error;
> +}
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 638a36be31c1..df14f1868165 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -558,8 +558,7 @@ int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa)
> }
> EXPORT_SYMBOL(copy_fsxattr_to_user);
>
> -static int copy_fsxattr_from_user(struct fileattr *fa,
> - struct fsxattr __user *ufa)
> +int copy_fsxattr_from_user(struct fileattr *fa, struct fsxattr __user *ufa)
> {
> struct fsxattr xfa;
>
> @@ -574,6 +573,7 @@ static int copy_fsxattr_from_user(struct fileattr *fa,
>
> return 0;
> }
> +EXPORT_SYMBOL(copy_fsxattr_from_user);
>
> /*
> * Generic function to check FS_IOC_FSSETXATTR/FS_IOC_SETFLAGS values and reject
> @@ -646,6 +646,19 @@ static int fileattr_set_prepare(struct inode *inode,
> if (fa->fsx_cowextsize == 0)
> fa->fsx_xflags &= ~FS_XFLAG_COWEXTSIZE;
>
> + /*
> + * The only use case for special files is to set project ID, forbid any
> + * other attributes
> + */
> + if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode))) {
> + if (fa->fsx_xflags & ~FS_XFLAG_PROJINHERIT)
> + return -EINVAL;
> + if (!S_ISLNK(inode->i_mode) && fa->fsx_nextents)
> + return -EINVAL;
> + if (fa->fsx_extsize || fa->fsx_cowextsize)
> + return -EINVAL;
> + }
> +
> return 0;
> }
>
> diff --git a/include/linux/fileattr.h b/include/linux/fileattr.h
> index 47c05a9851d0..8598e94b530b 100644
> --- a/include/linux/fileattr.h
> +++ b/include/linux/fileattr.h
> @@ -34,6 +34,7 @@ struct fileattr {
> };
>
> int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa);
> +int copy_fsxattr_from_user(struct fileattr *fa, struct fsxattr __user *ufa);
>
> void fileattr_fill_xflags(struct fileattr *fa, u32 xflags);
> void fileattr_fill_flags(struct fileattr *fa, u32 flags);
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index c6333204d451..a983023d21ab 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -371,6 +371,10 @@ asmlinkage long sys_removexattrat(int dfd, const char __user *path,
> asmlinkage long sys_lremovexattr(const char __user *path,
> const char __user *name);
> asmlinkage long sys_fremovexattr(int fd, const char __user *name);
> +asmlinkage long sys_getfsxattrat(int dfd, const char __user *filename,
> + struct fsxattr *fsx, int at_flags);
> +asmlinkage long sys_setfsxattrat(int dfd, const char __user *filename,
> + struct fsxattr *fsx, int at_flags);
> asmlinkage long sys_getcwd(char __user *buf, unsigned long size);
> asmlinkage long sys_eventfd2(unsigned int count, int flags);
> asmlinkage long sys_epoll_create1(int flags);
> diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
> index 88dc393c2bca..50be2e1007bc 100644
> --- a/include/uapi/asm-generic/unistd.h
> +++ b/include/uapi/asm-generic/unistd.h
> @@ -850,8 +850,14 @@ __SYSCALL(__NR_listxattrat, sys_listxattrat)
> #define __NR_removexattrat 466
> __SYSCALL(__NR_removexattrat, sys_removexattrat)
>
> +/* fs/inode.c */
> +#define __NR_getfsxattrat 467
> +__SYSCALL(__NR_getfsxattrat, sys_getfsxattrat)
> +#define __NR_setfsxattrat 468
> +__SYSCALL(__NR_setfsxattrat, sys_setfsxattrat)
> +
> #undef __NR_syscalls
> -#define __NR_syscalls 467
> +#define __NR_syscalls 469
>
> /*
> * 32 bit systems traditionally used different
> --
> 2.47.0
>
>
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Andrii Nakryiko @ 2025-01-14 23:52 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Jiri Olsa, Aleksa Sarai, Eyal Birger, mhiramat, linux-kernel,
linux-trace-kernel, BPF-dev-list, Song Liu, Yonghong Song,
John Fastabend, peterz, tglx, bp, x86, linux-api, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, rostedt@goodmis.org, rafi,
Shmulik Ladkani
In-Reply-To: <20250114221002.GA10122@redhat.com>
On Tue, Jan 14, 2025 at 2:11 PM Oleg Nesterov <oleg@redhat.com> wrote:
>
> On 01/14, Andrii Nakryiko wrote:
> >
> > On Tue, Jan 14, 2025 at 12:40 PM Oleg Nesterov <oleg@redhat.com> wrote:
> > >
> > > But, unlike sys_uretprobe(), sys_rt_sigreturn() is old, so the existing
> > > setups must know that sigreturn() should be respected...
> >
> > someday sys_uretprobe will be old as well ;) FWIW, systemd allowlisted
> > sys_uretprobe, see [0]
>
> And I agree! ;)
>
> I mean, I'd personally prefer to do nothing and wait until userspace figures
> out that we have another "special" syscall.
>
> But can we do it? I simply do not know. Can we ignore this (valid) bug report?
>
Seems wrong for kernel to try to guess whether some syscall is
filtered by some policy or not (though maybe I'm misunderstanding the
details and it's kernel-originated problem?). Seems like a recipe for
more problems.
Nothing is really fundamentally broken. Some piece of software needs
an upgraded library to not disable the kernel's special syscall (just
like sys_rt_sigreturn, nothing "new" here, really). Users can't do
uprobing in such broken setups (but not in general), seems like a good
incentive for everyone to push for the right thing here: fixed up to
date software.
> Oleg.
>
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Oleg Nesterov @ 2025-01-14 22:10 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Jiri Olsa, Aleksa Sarai, Eyal Birger, mhiramat, linux-kernel,
linux-trace-kernel, BPF-dev-list, Song Liu, Yonghong Song,
John Fastabend, peterz, tglx, bp, x86, linux-api, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, rostedt@goodmis.org, rafi,
Shmulik Ladkani
In-Reply-To: <CAEf4BzaRCzWMVvyGC_T52djF7q65yM8=AdBEMOPUU8edG-PLxg@mail.gmail.com>
On 01/14, Andrii Nakryiko wrote:
>
> On Tue, Jan 14, 2025 at 12:40 PM Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > But, unlike sys_uretprobe(), sys_rt_sigreturn() is old, so the existing
> > setups must know that sigreturn() should be respected...
>
> someday sys_uretprobe will be old as well ;) FWIW, systemd allowlisted
> sys_uretprobe, see [0]
And I agree! ;)
I mean, I'd personally prefer to do nothing and wait until userspace figures
out that we have another "special" syscall.
But can we do it? I simply do not know. Can we ignore this (valid) bug report?
Oleg.
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Andrii Nakryiko @ 2025-01-14 21:45 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Jiri Olsa, Aleksa Sarai, Eyal Birger, mhiramat, linux-kernel,
linux-trace-kernel, BPF-dev-list, Song Liu, Yonghong Song,
John Fastabend, peterz, tglx, bp, x86, linux-api, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, rostedt@goodmis.org, rafi,
Shmulik Ladkani
In-Reply-To: <20250114203922.GA5051@redhat.com>
On Tue, Jan 14, 2025 at 12:40 PM Oleg Nesterov <oleg@redhat.com> wrote:
>
> On 01/14, Andrii Nakryiko wrote:
> >
> > Should we just fix whoever is blocking kernel-internal special syscall
> > (sys_uretprobe)?
>
> Well, we can add __NR_uretprobe to mode1_syscalls[] but this won't
> really help.
>
> We can't "fix" the existing user-space setups which can nack any
> "unnecessary/unknown" syscall.
>
> > What would happen if someone blocked that other
> > special kernel-internal syscall for signal handling (can't remember
> > the name,
>
> sys_rt_sigreturn().
>
> Yes, the task will crash after return from the signal handler if this
> syscall is filtered out.
>
> But, unlike sys_uretprobe(), sys_rt_sigreturn() is old, so the existing
> setups must know that sigreturn() should be respected...
someday sys_uretprobe will be old as well ;) FWIW, systemd allowlisted
sys_uretprobe, see [0]
[0] https://github.com/systemd/systemd/issues/34615#issuecomment-2406761451
>
> Oleg.
>
>
^ permalink raw reply
* Re: [PATCH v23 7/8] samples/check-exec: Add an enlighten "inc" interpreter and 28 tests
From: Nathan Chancellor @ 2025-01-14 20:56 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Al Viro, Christian Brauner, Kees Cook, Paul Moore, Serge Hallyn,
Adhemerval Zanella Netto, Alejandro Colomar, Aleksa Sarai,
Andrew Morton, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Christian Heimes, Dmitry Vyukov, Elliott Hughes, Eric Biggers,
Eric Chiang, Fan Wu, Florian Weimer, Geert Uytterhoeven,
James Morris, Jan Kara, Jann Horn, Jeff Xu, Jonathan Corbet,
Jordan R Abrahams, Lakshmi Ramasubramanian, Linus Torvalds,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Roberto Sassu, Scott Shell,
Shuah Khan, Shuah Khan, Stephen Rothwell, Steve Dower,
Steve Grubb, Theodore Ts'o, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, kernel-hardening, linux-api,
linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20241212174223.389435-8-mic@digikod.net>
Hi Mickaël,
On Thu, Dec 12, 2024 at 06:42:22PM +0100, Mickaël Salaün wrote:
> Add a very simple script interpreter called "inc" that can evaluate two
> different commands (one per line):
> - "?" to initialize a counter from user's input;
> - "+" to increment the counter (which is set to 0 by default).
>
> It is enlighten to only interpret executable files according to
> AT_EXECVE_CHECK and the related securebits:
>
> # Executing a script with RESTRICT_FILE is only allowed if the script
> # is executable:
> ./set-exec -f -- ./inc script-exec.inc # Allowed
> ./set-exec -f -- ./inc script-noexec.inc # Denied
>
> # Executing stdin with DENY_INTERACTIVE is only allowed if stdin is an
> # executable regular file:
> ./set-exec -i -- ./inc -i < script-exec.inc # Allowed
> ./set-exec -i -- ./inc -i < script-noexec.inc # Denied
>
> # However, a pipe is not executable and it is then denied:
> cat script-noexec.inc | ./set-exec -i -- ./inc -i # Denied
>
> # Executing raw data (e.g. command argument) with DENY_INTERACTIVE is
> # always denied.
> ./set-exec -i -- ./inc -c "+" # Denied
> ./inc -c "$(<script-ask.inc)" # Allowed
>
> # To directly execute a script, we can update $PATH (used by `env`):
> PATH="${PATH}:." ./script-exec.inc
>
> # To execute several commands passed as argument:
>
> Add a complete test suite to check the script interpreter against all
> possible execution cases:
>
> make TARGETS=exec kselftest-install
> ./tools/testing/selftests/kselftest_install/run_kselftest.sh
>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Serge Hallyn <serge@hallyn.com>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Link: https://lore.kernel.org/r/20241212174223.389435-8-mic@digikod.net
...
> diff --git a/samples/check-exec/inc.c b/samples/check-exec/inc.c
> new file mode 100644
> index 000000000000..94b87569d2a2
> --- /dev/null
> +++ b/samples/check-exec/inc.c
...
> +/* Returns 1 on error, 0 otherwise. */
> +static int interpret_stream(FILE *script, char *const script_name,
> + char *const *const envp, const bool restrict_stream)
> +{
> + int err;
> + char *const script_argv[] = { script_name, NULL };
> + char buf[128] = {};
> + size_t buf_size = sizeof(buf);
> +
> + /*
> + * We pass a valid argv and envp to the kernel to emulate a native
> + * script execution. We must use the script file descriptor instead of
> + * the script path name to avoid race conditions.
> + */
> + err = execveat(fileno(script), "", script_argv, envp,
> + AT_EMPTY_PATH | AT_EXECVE_CHECK);
> + if (err && restrict_stream) {
> + perror("ERROR: Script execution check");
> + return 1;
> + }
> +
> + /* Reads script. */
> + buf_size = fread(buf, 1, buf_size - 1, script);
> + return interpret_buffer(buf, buf_size);
> +}
The use of execveat() in this test case breaks the build when glibc is
less than 2.34, as that is the earliest version that has the execveat()
wrapper:
https://sourceware.org/git/?p=glibc.git;a=commit;h=19d83270fcd993cc349570164e21b06d57036704
$ ldd --version | head -1
ldd (Debian GLIBC 2.31-13+deb11u11) 2.31
$ make -skj"$(nproc)" ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- mrproper allmodconfig samples/
...
samples/check-exec/inc.c:81:8: error: call to undeclared function 'execveat'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
81 | err = execveat(fileno(script), "", script_argv, envp,
| ^
samples/check-exec/inc.c:81:8: note: did you mean 'execve'?
/usr/include/unistd.h:551:12: note: 'execve' declared here
551 | extern int execve (const char *__path, char *const __argv[],
| ^
1 error generated.
...
Should this just use the syscall directly?
Cheers,
Nathan
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Oleg Nesterov @ 2025-01-14 20:39 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Jiri Olsa, Aleksa Sarai, Eyal Birger, mhiramat, linux-kernel,
linux-trace-kernel, BPF-dev-list, Song Liu, Yonghong Song,
John Fastabend, peterz, tglx, bp, x86, linux-api, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, rostedt@goodmis.org, rafi,
Shmulik Ladkani
In-Reply-To: <CAEf4BzZoa6gBQzfPLeMTQu+s=GqVdmihFdb1BHkcPPQMFQp+MQ@mail.gmail.com>
On 01/14, Andrii Nakryiko wrote:
>
> Should we just fix whoever is blocking kernel-internal special syscall
> (sys_uretprobe)?
Well, we can add __NR_uretprobe to mode1_syscalls[] but this won't
really help.
We can't "fix" the existing user-space setups which can nack any
"unnecessary/unknown" syscall.
> What would happen if someone blocked that other
> special kernel-internal syscall for signal handling (can't remember
> the name,
sys_rt_sigreturn().
Yes, the task will crash after return from the signal handler if this
syscall is filtered out.
But, unlike sys_uretprobe(), sys_rt_sigreturn() is old, so the existing
setups must know that sigreturn() should be respected...
Oleg.
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Andrii Nakryiko @ 2025-01-14 19:21 UTC (permalink / raw)
To: Jiri Olsa
Cc: Oleg Nesterov, Aleksa Sarai, Eyal Birger, mhiramat, linux-kernel,
linux-trace-kernel, BPF-dev-list, Song Liu, Yonghong Song,
John Fastabend, peterz, tglx, bp, x86, linux-api, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, rostedt@goodmis.org, rafi,
Shmulik Ladkani
In-Reply-To: <Z4ZyYudZSD92DPiF@krava>
On Tue, Jan 14, 2025 at 6:19 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Tue, Jan 14, 2025 at 11:58:03AM +0100, Oleg Nesterov wrote:
> > On 01/14, Jiri Olsa wrote:
> > >
> > > --- a/arch/x86/kernel/uprobes.c
> > > +++ b/arch/x86/kernel/uprobes.c
> > > @@ -315,14 +315,25 @@ asm (
> > > ".global uretprobe_trampoline_entry\n"
> > > "uretprobe_trampoline_entry:\n"
> > > "pushq %rax\n"
> > > + "pushq %rbx\n"
> > > "pushq %rcx\n"
> > > "pushq %r11\n"
> > > + "movq $1, %rbx\n"
> > > "movq $" __stringify(__NR_uretprobe) ", %rax\n"
> > > "syscall\n"
> > > ".global uretprobe_syscall_check\n"
> > > "uretprobe_syscall_check:\n"
> > > + "or %rbx,%rbx\n"
> > > + "jz uretprobe_syscall_return\n"
> > > "popq %r11\n"
> > > "popq %rcx\n"
> > > + "popq %rbx\n"
> > > + "popq %rax\n"
> > > + "int3\n"
> > > + "uretprobe_syscall_return:\n"
> > > + "popq %r11\n"
> > > + "popq %rcx\n"
> > > + "popq %rbx\n"
> >
> > But why do we need to abuse %rbx? Can't uretprobe_trampoline_entry do
> >
> > syscall
> >
> > // int3_section, in case sys_uretprobe() doesn't work
> > popq %r11
> > popq %rcx
> > popq %rax
> > int3
> >
> > uretprobe_syscall_return:
> > popq %r11
> > popq %rcx
> > popq %rbx
> > retq
> >
> > and change sys_uretprobe() to do
> >
> > - regs->ip = ip;
> > + regs->ip = ip + sizeof(int3_section);
>
> nice idea, I wonder we get the trampoline size under one xol slot with that
>
Should we just fix whoever is blocking kernel-internal special syscall
(sys_uretprobe)? What would happen if someone blocked that other
special kernel-internal syscall for signal handling (can't remember
the name, but the one that was an inspiration/justification for
sys_uretprobe)?
> thanks,
> jirka
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Oleg Nesterov @ 2025-01-14 17:43 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Jiri Olsa, Aleksa Sarai, Eyal Birger, mhiramat, linux-kernel,
linux-trace-kernel, BPF-dev-list, Song Liu, Yonghong Song,
John Fastabend, tglx, bp, x86, linux-api, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
rostedt@goodmis.org, rafi, Shmulik Ladkani
In-Reply-To: <20250114140729.GQ5388@noisy.programming.kicks-ass.net>
On 01/14, Peter Zijlstra wrote:
>
> On Tue, Jan 14, 2025 at 01:32:58PM +0100, Oleg Nesterov wrote:
>
> > OK, suppose we have
> >
> > void start_SECCOMP_MODE_STRICT(void)
> > {
> > // in particular nacks __NR_uretprobe
> > seccomp(SECCOMP_MODE_STRICT, ...);
> > }
> >
> > and we want to add uretprobe to this function.
> >
> > In this case prepare_uretprobe() can't know that sys_uretprobe() won't
> > work when this function returns?
>
> Indeed. But any further probes placed after seccomp() would be able to,
> and installing trampolines for them would be a waste, no?
But the probed task will crash when it returns from
start_SECCOMP_MODE_STRICT() above.
Even if, due to seccomp filtering, sys_uretprobe() doesn't kill the task
(I missed the fact it can) but just returns ENOSYS/whatever.
Oleg.
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Oleg Nesterov @ 2025-01-14 17:25 UTC (permalink / raw)
To: Jiri Olsa
Cc: Eyal Birger, Aleksa Sarai, mhiramat, linux-kernel,
linux-trace-kernel, BPF-dev-list, Song Liu, Yonghong Song,
John Fastabend, peterz, tglx, bp, x86, linux-api, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
rostedt@goodmis.org, rafi, Shmulik Ladkani
In-Reply-To: <Z4Z7OkrtXBauaLcm@krava>
On 01/14, Jiri Olsa wrote:
>
> ugh.. could we just 'disable' uretprobe trampoline when seccomp gets enabled?
> overwrite first byte with int3.. and similarly check on seccomp when installing
> uretprobe and switch to int3
Sorry, I don't understand... What exactly we can do? Aside from checking
IS_ENABLED(CONFIG_SECCOMP) in arch_uprobe_trampoline() ?
Oleg.
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Jiri Olsa @ 2025-01-14 14:56 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Eyal Birger, Jiri Olsa, Aleksa Sarai, mhiramat, linux-kernel,
linux-trace-kernel, BPF-dev-list, Song Liu, Yonghong Song,
John Fastabend, peterz, tglx, bp, x86, linux-api, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
rostedt@goodmis.org, rafi, Shmulik Ladkani
In-Reply-To: <20250114143313.GA29305@redhat.com>
On Tue, Jan 14, 2025 at 03:33:13PM +0100, Oleg Nesterov wrote:
> On 01/14, Eyal Birger wrote:
> >
> > FWIW If I change the seccomp policy to SCMP_ACT_KILL this still fails.
>
> Ah... I don't know what SCMP_ACT_KILL is, but indeed, in general it is
> not safe to even try to call sys_uretprobe() if it is filtered.
>
> Say, __secure_computing(SECCOMP_MODE_STRICT)->__secure_computing_strict()
> does do_exit(SIGKILL) :/
ugh.. could we just 'disable' uretprobe trampoline when seccomp gets enabled?
overwrite first byte with int3.. and similarly check on seccomp when installing
uretprobe and switch to int3
jirka
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Oleg Nesterov @ 2025-01-14 14:33 UTC (permalink / raw)
To: Eyal Birger
Cc: Jiri Olsa, Aleksa Sarai, mhiramat, linux-kernel,
linux-trace-kernel, BPF-dev-list, Song Liu, Yonghong Song,
John Fastabend, peterz, tglx, bp, x86, linux-api, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
rostedt@goodmis.org, rafi, Shmulik Ladkani
In-Reply-To: <CAHsH6Gst+UGCtiCaNq2ikaknZGghpTq2SFZX7S0A8=uDsXt=Zw@mail.gmail.com>
On 01/14, Eyal Birger wrote:
>
> FWIW If I change the seccomp policy to SCMP_ACT_KILL this still fails.
Ah... I don't know what SCMP_ACT_KILL is, but indeed, in general it is
not safe to even try to call sys_uretprobe() if it is filtered.
Say, __secure_computing(SECCOMP_MODE_STRICT)->__secure_computing_strict()
does do_exit(SIGKILL) :/
Oleg.
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Jiri Olsa @ 2025-01-14 14:21 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Masami Hiramatsu, Jiri Olsa, Aleksa Sarai, Eyal Birger,
linux-kernel, linux-trace-kernel, BPF-dev-list, Song Liu,
Yonghong Song, John Fastabend, peterz, tglx, bp, x86, linux-api,
Andrii Nakryiko, Daniel Borkmann, Alexei Starovoitov,
Andrii Nakryiko, rostedt@goodmis.org, rafi, Shmulik Ladkani
In-Reply-To: <20250114112106.GC19816@redhat.com>
On Tue, Jan 14, 2025 at 12:21:07PM +0100, Oleg Nesterov wrote:
> On 01/14, Masami Hiramatsu wrote:
> >
> > On Tue, 14 Jan 2025 10:22:20 +0100
> > Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > > @@ -418,6 +439,9 @@ SYSCALL_DEFINE0(uretprobe)
> > > regs->r11 = regs->flags;
> > > regs->cx = regs->ip;
> > >
> > > + /* zero rbx to signal trampoline that uretprobe syscall was executed */
> > > + regs->bx = 0;
> >
> > Can we just return -ENOSYS as like as other syscall instead of
> > using rbx as a side channel?
> > We can carefully check the return address is not -ERRNO when set up
> > and reserve the -ENOSYS for this use case.
>
> Not sure I understand...
>
> But please not that the uretprobed function can return any value
> including -ENOSYS, and this is what sys_uretprobe() has to return.
right, uretprobe syscall returns value of the uretprobed function,
so we can't use any reserved value
jirka
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Jiri Olsa @ 2025-01-14 14:19 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Jiri Olsa, Aleksa Sarai, Eyal Birger, mhiramat, linux-kernel,
linux-trace-kernel, BPF-dev-list, Song Liu, Yonghong Song,
John Fastabend, peterz, tglx, bp, x86, linux-api, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
rostedt@goodmis.org, rafi, Shmulik Ladkani
In-Reply-To: <20250114105802.GA19816@redhat.com>
On Tue, Jan 14, 2025 at 11:58:03AM +0100, Oleg Nesterov wrote:
> On 01/14, Jiri Olsa wrote:
> >
> > --- a/arch/x86/kernel/uprobes.c
> > +++ b/arch/x86/kernel/uprobes.c
> > @@ -315,14 +315,25 @@ asm (
> > ".global uretprobe_trampoline_entry\n"
> > "uretprobe_trampoline_entry:\n"
> > "pushq %rax\n"
> > + "pushq %rbx\n"
> > "pushq %rcx\n"
> > "pushq %r11\n"
> > + "movq $1, %rbx\n"
> > "movq $" __stringify(__NR_uretprobe) ", %rax\n"
> > "syscall\n"
> > ".global uretprobe_syscall_check\n"
> > "uretprobe_syscall_check:\n"
> > + "or %rbx,%rbx\n"
> > + "jz uretprobe_syscall_return\n"
> > "popq %r11\n"
> > "popq %rcx\n"
> > + "popq %rbx\n"
> > + "popq %rax\n"
> > + "int3\n"
> > + "uretprobe_syscall_return:\n"
> > + "popq %r11\n"
> > + "popq %rcx\n"
> > + "popq %rbx\n"
>
> But why do we need to abuse %rbx? Can't uretprobe_trampoline_entry do
>
> syscall
>
> // int3_section, in case sys_uretprobe() doesn't work
> popq %r11
> popq %rcx
> popq %rax
> int3
>
> uretprobe_syscall_return:
> popq %r11
> popq %rcx
> popq %rbx
> retq
>
> and change sys_uretprobe() to do
>
> - regs->ip = ip;
> + regs->ip = ip + sizeof(int3_section);
nice idea, I wonder we get the trampoline size under one xol slot with that
thanks,
jirka
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Eyal Birger @ 2025-01-14 14:08 UTC (permalink / raw)
To: Jiri Olsa
Cc: oleg, Aleksa Sarai, mhiramat, linux-kernel, linux-trace-kernel,
BPF-dev-list, Song Liu, Yonghong Song, John Fastabend, peterz,
tglx, bp, x86, linux-api, Andrii Nakryiko, Daniel Borkmann,
Alexei Starovoitov, Andrii Nakryiko, rostedt@goodmis.org, rafi,
Shmulik Ladkani
In-Reply-To: <Z4YszJfOvFEAaKjF@krava>
Hi Jiri,
On Tue, Jan 14, 2025 at 1:22 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Sat, Jan 11, 2025 at 07:40:15PM +0100, Jiri Olsa wrote:
> > On Sat, Jan 11, 2025 at 02:25:37AM +1100, Aleksa Sarai wrote:
> > > On 2025-01-10, Eyal Birger <eyal.birger@gmail.com> wrote:
> > > > Hi,
> > > >
> > > > When attaching uretprobes to processes running inside docker, the attached
> > > > process is segfaulted when encountering the retprobe. The offending commit
> > > > is:
> > > >
> > > > ff474a78cef5 ("uprobe: Add uretprobe syscall to speed up return probe")
> > > >
> > > > To my understanding, the reason is that now that uretprobe is a system call,
> > > > the default seccomp filters in docker block it as they only allow a specific
> > > > set of known syscalls.
> > >
> > > FWIW, the default seccomp profile of Docker _should_ return -ENOSYS for
> > > uretprobe (runc has a bunch of ugly logic to try to guarantee this if
> > > Docker hasn't updated their profile to include it). Though I guess that
> > > isn't sufficient for the magic that uretprobe(2) does...
> > >
> > > > This behavior can be reproduced by the below bash script, which works before
> > > > this commit.
> > > >
> > > > Reported-by: Rafael Buchbinder <rafi@rbk.io>
> >
> > hi,
> > nice ;-) thanks for the report, the problem seems to be that uretprobe syscall
> > is blocked and uretprobe trampoline does not expect that
> >
> > I think we could add code to the uretprobe trampoline to detect this and
> > execute standard int3 as fallback to process uretprobe, I'm checking on that
>
> hack below seems to fix the issue, it's using rbx to signal that uretprobe
> syscall got executed, if not, trampoline does int3 and executes uretprobe
> handler in the old way
FWIW If I change the seccomp policy to SCMP_ACT_KILL this still fails.
Eyal.
>
> unfortunately now the uretprobe trampoline size crosses the xol slot limit so
> will need to come up with some generic/arch code solution for that, code below
> is neglecting that for now
>
> jirka
>
>
> ---
> arch/x86/kernel/uprobes.c | 24 ++++++++++++++++++++++++
> include/linux/uprobes.h | 1 +
> kernel/events/uprobes.c | 10 ++++++++--
> 3 files changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 5a952c5ea66b..b54863f6fa25 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -315,14 +315,25 @@ asm (
> ".global uretprobe_trampoline_entry\n"
> "uretprobe_trampoline_entry:\n"
> "pushq %rax\n"
> + "pushq %rbx\n"
> "pushq %rcx\n"
> "pushq %r11\n"
> + "movq $1, %rbx\n"
> "movq $" __stringify(__NR_uretprobe) ", %rax\n"
> "syscall\n"
> ".global uretprobe_syscall_check\n"
> "uretprobe_syscall_check:\n"
> + "or %rbx,%rbx\n"
> + "jz uretprobe_syscall_return\n"
> "popq %r11\n"
> "popq %rcx\n"
> + "popq %rbx\n"
> + "popq %rax\n"
> + "int3\n"
> + "uretprobe_syscall_return:\n"
> + "popq %r11\n"
> + "popq %rcx\n"
> + "popq %rbx\n"
>
> /* The uretprobe syscall replaces stored %rax value with final
> * return address, so we don't restore %rax in here and just
> @@ -338,6 +349,16 @@ extern u8 uretprobe_trampoline_entry[];
> extern u8 uretprobe_trampoline_end[];
> extern u8 uretprobe_syscall_check[];
>
> +#define UINSNS_PER_PAGE (PAGE_SIZE/UPROBE_XOL_SLOT_BYTES)
> +
> +bool arch_is_uretprobe_trampoline(unsigned long vaddr)
> +{
> + unsigned long start = uprobe_get_trampoline_vaddr();
> + unsigned long end = start + 2*UINSNS_PER_PAGE;
> +
> + return vaddr >= start && vaddr < end;
> +}
> +
> void *arch_uprobe_trampoline(unsigned long *psize)
> {
> static uprobe_opcode_t insn = UPROBE_SWBP_INSN;
> @@ -418,6 +439,9 @@ SYSCALL_DEFINE0(uretprobe)
> regs->r11 = regs->flags;
> regs->cx = regs->ip;
>
> + /* zero rbx to signal trampoline that uretprobe syscall was executed */
> + regs->bx = 0;
> +
> return regs->ax;
>
> sigill:
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index e0a4c2082245..dbde57a68a1b 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -213,6 +213,7 @@ extern void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr,
> 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);
> +bool arch_is_uretprobe_trampoline(unsigned long vaddr);
> #else /* !CONFIG_UPROBES */
> struct uprobes_state {
> };
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index fa04b14a7d72..73df64109f38 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1703,6 +1703,11 @@ void * __weak arch_uprobe_trampoline(unsigned long *psize)
> return &insn;
> }
>
> +bool __weak arch_is_uretprobe_trampoline(unsigned long vaddr)
> +{
> + return vaddr == uprobe_get_trampoline_vaddr();
> +}
> +
> static struct xol_area *__create_xol_area(unsigned long vaddr)
> {
> struct mm_struct *mm = current->mm;
> @@ -1725,8 +1730,9 @@ static struct xol_area *__create_xol_area(unsigned long vaddr)
>
> area->vaddr = vaddr;
> init_waitqueue_head(&area->wq);
> - /* Reserve the 1st slot for get_trampoline_vaddr() */
> + /* Reserve the first two slots for get_trampoline_vaddr() */
> set_bit(0, area->bitmap);
> + set_bit(1, area->bitmap);
> insns = arch_uprobe_trampoline(&insns_size);
> arch_uprobe_copy_ixol(area->page, 0, insns, insns_size);
>
> @@ -2536,7 +2542,7 @@ static void handle_swbp(struct pt_regs *regs)
> int is_swbp;
>
> bp_vaddr = uprobe_get_swbp_addr(regs);
> - if (bp_vaddr == uprobe_get_trampoline_vaddr())
> + if (arch_is_uretprobe_trampoline(bp_vaddr))
> return uprobe_handle_trampoline(regs);
>
> rcu_read_lock_trace();
> --
> 2.47.1
>
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Peter Zijlstra @ 2025-01-14 14:07 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Jiri Olsa, Aleksa Sarai, Eyal Birger, mhiramat, linux-kernel,
linux-trace-kernel, BPF-dev-list, Song Liu, Yonghong Song,
John Fastabend, tglx, bp, x86, linux-api, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
rostedt@goodmis.org, rafi, Shmulik Ladkani
In-Reply-To: <20250114123257.GD19816@redhat.com>
On Tue, Jan 14, 2025 at 01:32:58PM +0100, Oleg Nesterov wrote:
> Sorry, I don't understand... Perhaps because I am enjoying my state after
> dentist appointment ;)
For some reason I thought to remember that parent thread would spawn
restricted child, however:
> OK, suppose we have
>
> void start_SECCOMP_MODE_STRICT(void)
> {
> // in particular nacks __NR_uretprobe
> seccomp(SECCOMP_MODE_STRICT, ...);
> }
>
> and we want to add uretprobe to this function.
>
> In this case prepare_uretprobe() can't know that sys_uretprobe() won't
> work when this function returns?
Indeed. But any further probes placed after seccomp() would be able to,
and installing trampolines for them would be a waste, no?
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Oleg Nesterov @ 2025-01-14 12:32 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Jiri Olsa, Aleksa Sarai, Eyal Birger, mhiramat, linux-kernel,
linux-trace-kernel, BPF-dev-list, Song Liu, Yonghong Song,
John Fastabend, tglx, bp, x86, linux-api, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
rostedt@goodmis.org, rafi, Shmulik Ladkani
In-Reply-To: <20250114120235.GP5388@noisy.programming.kicks-ass.net>
On 01/14, Peter Zijlstra wrote:
>
> On Tue, Jan 14, 2025 at 12:01:50PM +0100, Oleg Nesterov wrote:
> > On 01/14, Peter Zijlstra wrote:
> > >
> > > On Tue, Jan 14, 2025 at 10:22:20AM +0100, Jiri Olsa wrote:
> > > >
> > > > hack below seems to fix the issue, it's using rbx to signal that uretprobe
> > > > syscall got executed, if not, trampoline does int3 and executes uretprobe
> > > > handler in the old way
> > > >
> > > > unfortunately now the uretprobe trampoline size crosses the xol slot limit so
> > > > will need to come up with some generic/arch code solution for that, code below
> > > > is neglecting that for now
> > >
> > > Can't you detect the filter earlier and simply not install the
> > > trampoline?
> >
> > Did you mean detect the filter in prepare_uretprobe() ?
>
> Yep. Aren't syscall filters static for the duration of the task?
>
> > The probed function can install the filter before return...
>
> If you're running a task with dynamic syscall filtering, you get to keep
> the pieces no?
Sorry, I don't understand... Perhaps because I am enjoying my state after
dentist appointment ;)
OK, suppose we have
void start_SECCOMP_MODE_STRICT(void)
{
// in particular nacks __NR_uretprobe
seccomp(SECCOMP_MODE_STRICT, ...);
}
and we want to add uretprobe to this function.
In this case prepare_uretprobe() can't know that sys_uretprobe() won't
work when this function returns?
Oleg.
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Peter Zijlstra @ 2025-01-14 12:02 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Jiri Olsa, Aleksa Sarai, Eyal Birger, mhiramat, linux-kernel,
linux-trace-kernel, BPF-dev-list, Song Liu, Yonghong Song,
John Fastabend, tglx, bp, x86, linux-api, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
rostedt@goodmis.org, rafi, Shmulik Ladkani
In-Reply-To: <20250114110149.GB19816@redhat.com>
On Tue, Jan 14, 2025 at 12:01:50PM +0100, Oleg Nesterov wrote:
> On 01/14, Peter Zijlstra wrote:
> >
> > On Tue, Jan 14, 2025 at 10:22:20AM +0100, Jiri Olsa wrote:
> > >
> > > hack below seems to fix the issue, it's using rbx to signal that uretprobe
> > > syscall got executed, if not, trampoline does int3 and executes uretprobe
> > > handler in the old way
> > >
> > > unfortunately now the uretprobe trampoline size crosses the xol slot limit so
> > > will need to come up with some generic/arch code solution for that, code below
> > > is neglecting that for now
> >
> > Can't you detect the filter earlier and simply not install the
> > trampoline?
>
> Did you mean detect the filter in prepare_uretprobe() ?
Yep. Aren't syscall filters static for the duration of the task?
> The probed function can install the filter before return...
If you're running a task with dynamic syscall filtering, you get to keep
the pieces no?
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Oleg Nesterov @ 2025-01-14 11:21 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Jiri Olsa, Aleksa Sarai, Eyal Birger, linux-kernel,
linux-trace-kernel, BPF-dev-list, Song Liu, Yonghong Song,
John Fastabend, peterz, tglx, bp, x86, linux-api, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
rostedt@goodmis.org, rafi, Shmulik Ladkani
In-Reply-To: <20250114190521.0b69a1af64cac41106101154@kernel.org>
On 01/14, Masami Hiramatsu wrote:
>
> On Tue, 14 Jan 2025 10:22:20 +0100
> Jiri Olsa <olsajiri@gmail.com> wrote:
>
> > @@ -418,6 +439,9 @@ SYSCALL_DEFINE0(uretprobe)
> > regs->r11 = regs->flags;
> > regs->cx = regs->ip;
> >
> > + /* zero rbx to signal trampoline that uretprobe syscall was executed */
> > + regs->bx = 0;
>
> Can we just return -ENOSYS as like as other syscall instead of
> using rbx as a side channel?
> We can carefully check the return address is not -ERRNO when set up
> and reserve the -ENOSYS for this use case.
Not sure I understand...
But please not that the uretprobed function can return any value
including -ENOSYS, and this is what sys_uretprobe() has to return.
Oleg.
^ 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