* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Oleg Nesterov @ 2025-01-21 15:28 UTC (permalink / raw)
To: Kees Cook
Cc: Eyal Birger, luto, wad, ldv, mhiramat, andrii, jolsa,
alexei.starovoitov, olsajiri, cyphar, songliubraving, yhs,
john.fastabend, peterz, tglx, bp, daniel, ast, andrii.nakryiko,
rostedt, rafi, shmulik.ladkani, bpf, linux-api,
linux-trace-kernel, x86, linux-kernel, stable
In-Reply-To: <202501201331.83DB01794@keescook>
On 01/20, Kees Cook wrote:
>
> > The only difference is that sys_uretprobe() is new and existing setups
> > doesn't know about it. Suppose you have
> >
> > int func(void)
> > {
> > return 123;
> > }
> >
> > int main(void)
> > {
> > seccomp(SECCOMP_SET_MODE_STRICT, 0,0);
> > for (;;)
> > func();
> > }
> >
> > and it runs with func() uretprobed.
> >
> > If you install the new kernel, this application will crash immediately.
> >
> > I understand your objections, but what do you think we can do instead?
> > I don't think a new "try_to_speedup_uretprobes_at_your_own_risk" sysctl
> > makes sense, it will be almost never enabled...
>
> This seems like a uretprobes design problem. If it's going to use
> syscalls, it must take things like seccomp into account.
True. I reviewed that patch, and I forgot about seccomp too.
> SECCOMP_SET_MODE_STRICT will also crash in the face of syscall_restart...
Yes, I guess SECCOMP_SET_MODE_STRICT assumes that read/write can't return
ERESTART_RESTARTBLOCK.
But again, what can we do right now?
I do not like the idea to revert the patch which adds sys_uretprobe().
Don't get me wrong, I do not use uprobes, so personally I don't really
care about the performance improvements it adds. Not to mention FRED,
although I have no idea when it will be available.
Lets forget about sys_uretprobe(). Lets suppose the kernel doesn't have
ERESTART_RESTARTBLOCK/sys_restart_syscall and we want to add this feature
today.
How do you think we can do this without breaking the existing setups which
use seccomp ?
Oleg.
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Steven Rostedt @ 2025-01-21 16:16 UTC (permalink / raw)
To: Jiri Olsa
Cc: Eyal Birger, Kees Cook, luto, wad, oleg, ldv, mhiramat, andrii,
alexei.starovoitov, cyphar, songliubraving, yhs, john.fastabend,
peterz, tglx, bp, daniel, ast, andrii.nakryiko, rafi,
shmulik.ladkani, bpf, linux-api, linux-trace-kernel, x86,
linux-kernel, stable
In-Reply-To: <Z4-xeFH0Mgo3llga@krava>
[ Watching this with popcorn from the sidelines, but I'll chime in anyway ]
On Tue, 21 Jan 2025 15:38:48 +0100
Jiri Olsa <olsajiri@gmail.com> wrote:
> I'm still trying to come up with some other solution but wanted
> to exhaust all the options I could think of
I think this may have been mentioned, but is there a way that the kernel
could know that this system call is being monitored by seccomp, and if so,
just stick with the interrupt version? If not, enable the system call?
-- Steve
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Oleg Nesterov @ 2025-01-21 16:44 UTC (permalink / raw)
To: Steven Rostedt
Cc: Jiri Olsa, Eyal Birger, Kees Cook, luto, wad, ldv, mhiramat,
andrii, alexei.starovoitov, cyphar, songliubraving, yhs,
john.fastabend, peterz, tglx, bp, daniel, ast, andrii.nakryiko,
rafi, shmulik.ladkani, bpf, linux-api, linux-trace-kernel, x86,
linux-kernel, stable
In-Reply-To: <20250121111631.6e830edd@gandalf.local.home>
On 01/21, Steven Rostedt wrote:
>
> I think this may have been mentioned, but is there a way that the kernel
> could know that this system call is being monitored by seccomp, and if so,
> just stick with the interrupt version? If not, enable the system call?
Consider
int func_to_uretprobe()
{
seccomp(SECCOMP_SET_MODE_STRICT/whatever);
return 123;
}
by the time it is called, the kernel can't know that this function will
call seccomp/install-the-filters/etc, so prepare_uretprobe() can't know
if it is safe to use uretprobe or not.
Oleg.
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Jiri Olsa @ 2025-01-21 16:55 UTC (permalink / raw)
To: Steven Rostedt
Cc: Jiri Olsa, Eyal Birger, Kees Cook, luto, wad, oleg, ldv, mhiramat,
andrii, alexei.starovoitov, cyphar, songliubraving, yhs,
john.fastabend, peterz, tglx, bp, daniel, ast, andrii.nakryiko,
rafi, shmulik.ladkani, bpf, linux-api, linux-trace-kernel, x86,
linux-kernel, stable
In-Reply-To: <20250121111631.6e830edd@gandalf.local.home>
On Tue, Jan 21, 2025 at 11:16:31AM -0500, Steven Rostedt wrote:
>
> [ Watching this with popcorn from the sidelines, but I'll chime in anyway ]
>
> On Tue, 21 Jan 2025 15:38:48 +0100
> Jiri Olsa <olsajiri@gmail.com> wrote:
>
> > I'm still trying to come up with some other solution but wanted
> > to exhaust all the options I could think of
>
> I think this may have been mentioned, but is there a way that the kernel
> could know that this system call is being monitored by seccomp, and if so,
> just stick with the interrupt version? If not, enable the system call?
yes [1], the problem with that solution is that we install uretprobe
trampoline at function's uprobe entry probe, so we won't catch case
where seccomp is enabled in this probed function, like:
foo
uprobe -> install uretprobe trampoline
...
seccomp(SECCOMP_MODE_STRICT..
...
ret -> execute uretprobe trampoline with sys_uretprobe
I thought we could perhaps switch existing uretprobe trampoline to
int3 when we are in sys_seccomp, but another user thread might be
already executing the existing uretprobe trampoline, so I don't
think we can do that
jirka
[1] https://lore.kernel.org/bpf/20250114123257.GD19816@redhat.com/
^ permalink raw reply
* Re: [PATCH v6 01/15] lib: Add TLV parser
From: Thomas Weißschuh @ 2025-01-21 21:42 UTC (permalink / raw)
To: Roberto Sassu
Cc: zohar, dmitry.kasatkin, eric.snowberg, corbet, mcgrof, petr.pavlu,
samitolvanen, da.gomez, akpm, paul, jmorris, serge, shuah,
mcoquelin.stm32, alexandre.torgue, linux-integrity, linux-doc,
linux-kernel, linux-api, linux-modules, linux-security-module,
linux-kselftest, wufan, pbrobinson, zbyszek, hch, mjg59, pmatilai,
jannh, dhowells, jikos, mkoutny, ppavlu, petr.vorel, mzerqung,
kgold, Roberto Sassu
In-Reply-To: <b14358075fa56f7250d5c9000ab8ee181003ff13.camel@huaweicloud.com>
On 2025-01-21 15:55:28+0100, Roberto Sassu wrote:
> On Tue, 2025-01-21 at 14:29 +0100, Thomas Weißschuh wrote:
> > On 2024-11-19 11:49:08+0100, Roberto Sassu wrote:
[..]
> > > +typedef int (*callback)(void *callback_data, __u16 field,
> > > + const __u8 *field_data, __u32 field_len);
> >
> > No need for __underscore types in kernel-only signatures.
>
> It is just for convenience. I'm reusing the same file for the userspace
> counterpart digest-cache-tools. In that case, the parser is used for
> example to show the content of the digest list.
This reuse leads to quite some ugly constructs.
Given that the single function will be really simple after removing the
unnecessary parts, maybe two clean copies are easier.
One copy is needed for Frama-C anyways.
> > > +
> > > +int tlv_parse(callback callback, void *callback_data, const __u8 *data,
> > > + size_t data_len, const char **fields, __u32 num_fields);
> > > +
> > > +#endif /* _LINUX_TLV_PARSER_H */
> > > diff --git a/include/uapi/linux/tlv_parser.h b/include/uapi/linux/tlv_parser.h
> > > new file mode 100644
> > > index 000000000000..171d0cfd2c4c
> > > --- /dev/null
> > > +++ b/include/uapi/linux/tlv_parser.h
> > > @@ -0,0 +1,41 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > > +/*
> > > + * Copyright (C) 2023-2024 Huawei Technologies Duesseldorf GmbH
> > > + *
> > > + * Author: Roberto Sassu <roberto.sassu@huawei.com>
> > > + *
> > > + * Implement the user space interface for the TLV parser.
> > > + */
> >
> > Can you explain in the commit message where this will be exposed to
> > userspace as binary?
>
> I see that my explanation is not ideal.
>
> This is the format for data exchange between user space and kernel
> space, but it is still the kernel that reads and parses the TLV-
> formatted file for extracting the digests and adding them to the digest
> cache.
I figured that out :-)
It should be clear from the commit itself, though.
> > > +
> > > +#ifndef _UAPI_LINUX_TLV_PARSER_H
> > > +#define _UAPI_LINUX_TLV_PARSER_H
> > > +
> > > +#include <linux/types.h>
> > > +
> > > +/*
> > > + * TLV format:
> > > + *
> > > + * +--------------+--+---------+--------+---------+
> > > + * | field1 (u16) | len1 (u32) | value1 (u8 len1) |
> > > + * +--------------+------------+------------------+
> > > + * | ... | ... | ... |
> > > + * +--------------+------------+------------------+
> > > + * | fieldN (u16) | lenN (u32) | valueN (u8 lenN) |
> > > + * +--------------+------------+------------------+
> > > + */
> > > +
> > > +/**
> > > + * struct tlv_entry - Entry of TLV format
> > > + * @field: Field identifier
> > > + * @length: Data length
> > > + * @data: Data
> > > + *
> > > + * This structure represents an entry of the TLV format.
> > > + */
> > > +struct tlv_entry {
> > > + __u16 field;
> > > + __u32 length;
Looking at this again, the "length" field is unaligned by default.
Also FYI there is already a TLV implementation in
include/uapi/linux/tipc_config.
> > > +} __attribute__((packed));
[..]
> > Some kunit tests would be great.
>
> I implemented kselftests also injecting errors (patch 13). If it is not
> enough, I implement kunit tests too.
These selftests are for the digest_cache.
If the TLV library is meant to be used alone, some dedicated tests would
be nice. kunit has the advantage that it can directly call kernel
functions with arbitrary parameters and does not require any userspace
setup.
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Andrii Nakryiko @ 2025-01-21 22:38 UTC (permalink / raw)
To: Jiri Olsa, Kees Cook
Cc: Steven Rostedt, Eyal Birger, luto, wad, oleg, ldv, mhiramat,
andrii, alexei.starovoitov, cyphar, songliubraving, yhs,
john.fastabend, peterz, tglx, bp, daniel, ast, rafi,
shmulik.ladkani, bpf, linux-api, linux-trace-kernel, x86,
linux-kernel, stable
In-Reply-To: <Z4_Riahgmj-bMR8s@krava>
On Tue, Jan 21, 2025 at 8:55 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Tue, Jan 21, 2025 at 11:16:31AM -0500, Steven Rostedt wrote:
> >
> > [ Watching this with popcorn from the sidelines, but I'll chime in anyway ]
> >
> > On Tue, 21 Jan 2025 15:38:48 +0100
> > Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > > I'm still trying to come up with some other solution but wanted
> > > to exhaust all the options I could think of
> >
> > I think this may have been mentioned, but is there a way that the kernel
> > could know that this system call is being monitored by seccomp, and if so,
> > just stick with the interrupt version? If not, enable the system call?
>
> yes [1], the problem with that solution is that we install uretprobe
> trampoline at function's uprobe entry probe, so we won't catch case
> where seccomp is enabled in this probed function, like:
>
> foo
> uprobe -> install uretprobe trampoline
> ...
> seccomp(SECCOMP_MODE_STRICT..
> ...
> ret -> execute uretprobe trampoline with sys_uretprobe
>
>
> I thought we could perhaps switch existing uretprobe trampoline to
> int3 when we are in sys_seccomp, but another user thread might be
> already executing the existing uretprobe trampoline, so I don't
> think we can do that
Jiri,
We should abandon the vector of "let's try to detect whether someone
is blocking sys_uretprobe" as a solution, I don't believe it's
possible. Blocking sys_uretprobe is too dynamic of a thing. There is
an arbitrary periods of time between adding uretprobe trampoline
(i.e., sys_uretprobe) and actually disabling sys_uretprobe through
seccomp (or even BPF: LSM or even kprobes can do that, why not?), and
userspace can flip this decision many times over.
And as Oleg said, sysctl
"please-make-my-uretprobe-2x-faster-assuming-i-know-about-this-option"
makes no sense either, this will basically almost never get enabled.
Kees,
You said yourself that sys_uretprobe is no different from rt_sigreturn
and restart_syscall, so why would we rollback sys_uretprobe if we
wouldn't rollback rt_sigreturn/restart_syscall? Given it's impossible,
generally speaking, to know if userspace is blocking the syscall (and
that can change dynamically and very frequently), any improvement or
optimization that kernel would do with the help of special syscall is
now prohibited, effectively. That doesn't seem wise to restrict the
kernel development so much just because libseccomp blocks any unknown
syscall by default.
I'm OK either asking libseccomp to learn about sys_uretprobe and not
block it (like systemd is doing), or if we want to bend over
backwards, prevent user policy from filtering theses special syscalls
which are meant to be used by kernel only. We can't single out
sys_uretprobe just because it's the newest of this special cohort.
You also asked "what if userspace wants to block uprobes"? If that's
really the goal, that would be done at uprobe attachment time, not
when uprobe is (conceptually) attached, new process is forked, and
kernel installs uretprobe trampoline with uretprobe syscall. Or just
control that through (lack of) capabilities. Using seccomp to block
*second part of uretprobe handling* doesn't make much sense. It's just
the wrong place for that.
P.S. Also using FRED as an excuse for not doing sys_uretprobe is
manipulative. When we get FRED-enabled CPUs widely available and
deployed *and* all (or at least majority of) the currently used CPUs
are decommissioned, only then we can realistically talk about
sys_uretprobe being unnecessary. That's years and years. sys_uretprobe
is necessary and important *right now* and will be for the foreseeable
future.
>
> jirka
>
>
> [1] https://lore.kernel.org/bpf/20250114123257.GD19816@redhat.com/
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Steven Rostedt @ 2025-01-21 22:46 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Jiri Olsa, Kees Cook, Eyal Birger, luto, wad, oleg, ldv, mhiramat,
andrii, alexei.starovoitov, cyphar, songliubraving, yhs,
john.fastabend, peterz, tglx, bp, daniel, ast, rafi,
shmulik.ladkani, bpf, linux-api, linux-trace-kernel, x86,
linux-kernel, stable
In-Reply-To: <CAEf4BzZv3s0NtrviQ1MCCwZMO-SqCsiQF-WXpG6_-p4u5GeA2A@mail.gmail.com>
On Tue, 21 Jan 2025 14:38:09 -0800
Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> You said yourself that sys_uretprobe is no different from rt_sigreturn
> and restart_syscall, so why would we rollback sys_uretprobe if we
> wouldn't rollback rt_sigreturn/restart_syscall? Given it's impossible,
> generally speaking, to know if userspace is blocking the syscall (and
> that can change dynamically and very frequently), any improvement or
> optimization that kernel would do with the help of special syscall is
> now prohibited, effectively. That doesn't seem wise to restrict the
> kernel development so much just because libseccomp blocks any unknown
> syscall by default.
What happens if the system call is hit when there was no uprobe attached to
it? Perhaps it should segfault? That is, this system call is only used when
the kernel attaches it, if the kernel did not attach it, perhaps there's
some malicious code that is trying to use it for some CVE corner case. But
if it always crashes when added, the only thing the malicious code can do
by adding this system call is to crash the application. That shouldn't be a
problem, as if malicious code can add a system call, it can also change the
code to crash as well.
Perhaps the security folks would feel better if there were other
protections against this system call when not used as expected?
-- Steve
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Eyal Birger @ 2025-01-21 23:13 UTC (permalink / raw)
To: Steven Rostedt
Cc: Andrii Nakryiko, Jiri Olsa, Kees Cook, luto, wad, oleg, ldv,
mhiramat, andrii, alexei.starovoitov, cyphar, songliubraving, yhs,
john.fastabend, peterz, tglx, bp, daniel, ast, rafi,
shmulik.ladkani, bpf, linux-api, linux-trace-kernel, x86,
linux-kernel, stable
In-Reply-To: <20250121174620.06a0c811@gandalf.local.home>
On Tue, Jan 21, 2025 at 2:46 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Tue, 21 Jan 2025 14:38:09 -0800
> Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> > You said yourself that sys_uretprobe is no different from rt_sigreturn
> > and restart_syscall, so why would we rollback sys_uretprobe if we
> > wouldn't rollback rt_sigreturn/restart_syscall? Given it's impossible,
> > generally speaking, to know if userspace is blocking the syscall (and
> > that can change dynamically and very frequently), any improvement or
> > optimization that kernel would do with the help of special syscall is
> > now prohibited, effectively. That doesn't seem wise to restrict the
> > kernel development so much just because libseccomp blocks any unknown
> > syscall by default.
>
> What happens if the system call is hit when there was no uprobe attached to
> it? Perhaps it should segfault? That is, this system call is only used when
> the kernel attaches it, if the kernel did not attach it, perhaps there's
> some malicious code that is trying to use it for some CVE corner case. But
> if it always crashes when added, the only thing the malicious code can do
> by adding this system call is to crash the application. That shouldn't be a
> problem, as if malicious code can add a system call, it can also change the
> code to crash as well.
>
> Perhaps the security folks would feel better if there were other
> protections against this system call when not used as expected?
Isn't that the case already, or maybe I misunderstood what Jiri wrote [1]:
> On Sun, Jan 19, 2025 at 2:44 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> that's correct, uretprobe syscall is installed by kernel to special user
> memory map and it can be executed only from there and if process calls it
> from another place it receives sigill
Eyal.
[1] https://lore.kernel.org/lkml/Z4zXlaEMPbiYYlQ8@krava/
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Steven Rostedt @ 2025-01-21 23:29 UTC (permalink / raw)
To: Eyal Birger
Cc: Andrii Nakryiko, Jiri Olsa, Kees Cook, luto, wad, oleg, ldv,
mhiramat, andrii, alexei.starovoitov, cyphar, songliubraving, yhs,
john.fastabend, peterz, tglx, bp, daniel, ast, rafi,
shmulik.ladkani, bpf, linux-api, linux-trace-kernel, x86,
linux-kernel, stable
In-Reply-To: <CAHsH6GvcOjNh8VMpPs9CzyVSCOB+92zRj_3ZeDOd6APySWdm5Q@mail.gmail.com>
On Tue, 21 Jan 2025 15:13:52 -0800
Eyal Birger <eyal.birger@gmail.com> wrote:
> Isn't that the case already, or maybe I misunderstood what Jiri wrote [1]:
>
> > On Sun, Jan 19, 2025 at 2:44 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> > that's correct, uretprobe syscall is installed by kernel to special user
> > memory map and it can be executed only from there and if process calls it
> > from another place it receives sigill
>
> Eyal.
>
> [1] https://lore.kernel.org/lkml/Z4zXlaEMPbiYYlQ8@krava/
Ah, he did. Thanks I missed that:
> that's correct, uretprobe syscall is installed by kernel to special user
> memory map and it can be executed only from there and if process calls it
> from another place it receives sigill
-- Steve
^ permalink raw reply
* [PATCH v2] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Andrey Albershteyn @ 2025-01-22 14:18 UTC (permalink / raw)
To: linux-m68k, tglx, jcmvbkbc, linux-security-module, arnd,
linux-fsdevel, chris, npiggin, linuxppc-dev, mpe, luto, jack,
brauner, monstr, linux-arch, mingo, linux-alpha, christophe.leroy,
linux-sh, linux-parisc, naveen, bp, hpa, sparclinux, linux-kernel,
x86, maddy, dave.hansen, viro, linux-s390, linux-api
Cc: Andrey Albershteyn, linux-xfs
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
CC: linux-fsdevel@vger.kernel.org
CC: linux-xfs@vger.kernel.org
Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
---
v1:
https://lore.kernel.org/linuxppc-dev/20250109174540.893098-1-aalbersh@kernel.org/
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/arm/tools/syscall.tbl | 2 +
arch/arm64/tools/syscall_32.tbl | 2 +
arch/m68k/kernel/syscalls/syscall.tbl | 2 +
arch/microblaze/kernel/syscalls/syscall.tbl | 2 +
arch/mips/kernel/syscalls/syscall_n32.tbl | 2 +
arch/mips/kernel/syscalls/syscall_n64.tbl | 2 +
arch/mips/kernel/syscalls/syscall_o32.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 | 99 +++++++++++++++++++++++++++++
fs/ioctl.c | 16 ++++-
include/linux/fileattr.h | 1 +
include/linux/syscalls.h | 4 ++
include/uapi/asm-generic/unistd.h | 8 ++-
21 files changed, 157 insertions(+), 3 deletions(-)
diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl
index c59d53d6d3f3490f976ca179ddfe02e69265ae4d..4b9e687494c16b60c6fd6ca1dc4d6564706a7e25 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/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
index 49eeb2ad8dbd8e074c6240417693f23fb328afa8..66466257f3c2debb3e2299f0b608c6740c98cab2 100644
--- a/arch/arm/tools/syscall.tbl
+++ b/arch/arm/tools/syscall.tbl
@@ -481,3 +481,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/arm64/tools/syscall_32.tbl b/arch/arm64/tools/syscall_32.tbl
index 69a829912a05eb8a3e21ed701d1030e31c0148bc..9c516118b154811d8d11d5696f32817430320dbf 100644
--- a/arch/arm64/tools/syscall_32.tbl
+++ b/arch/arm64/tools/syscall_32.tbl
@@ -478,3 +478,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/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl
index f5ed71f1910d09769c845c2d062d99ee0449437c..159476387f394a92ee5e29db89b118c630372db2 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 680f568b77f2cbefc3eacb2517f276041f229b1e..a6d59ee740b58cacf823702003cf9bad17c0d3b7 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/mips/kernel/syscalls/syscall_n32.tbl b/arch/mips/kernel/syscalls/syscall_n32.tbl
index 0b9b7e25b69ad592642f8533bee9ccfe95ce9626..cfe38fcebe1a0279e11751378d3e71c5ec6b6569 100644
--- a/arch/mips/kernel/syscalls/syscall_n32.tbl
+++ b/arch/mips/kernel/syscalls/syscall_n32.tbl
@@ -405,3 +405,5 @@
464 n32 getxattrat sys_getxattrat
465 n32 listxattrat sys_listxattrat
466 n32 removexattrat sys_removexattrat
+467 n32 getfsxattrat sys_getfsxattrat
+468 n32 setfsxattrat sys_setfsxattrat
diff --git a/arch/mips/kernel/syscalls/syscall_n64.tbl b/arch/mips/kernel/syscalls/syscall_n64.tbl
index c844cd5cda620b2809a397cdd6f4315ab6a1bfe2..29a0c5974d1aa2f01e33edc0252d75fb97abe230 100644
--- a/arch/mips/kernel/syscalls/syscall_n64.tbl
+++ b/arch/mips/kernel/syscalls/syscall_n64.tbl
@@ -381,3 +381,5 @@
464 n64 getxattrat sys_getxattrat
465 n64 listxattrat sys_listxattrat
466 n64 removexattrat sys_removexattrat
+467 n64 getfsxattrat sys_getfsxattrat
+468 n64 setfsxattrat sys_setfsxattrat
diff --git a/arch/mips/kernel/syscalls/syscall_o32.tbl b/arch/mips/kernel/syscalls/syscall_o32.tbl
index 349b8aad1159f404103bd2057a1e64e9bf309f18..6c00436807c57c492ba957fcd59af1202231cf80 100644
--- a/arch/mips/kernel/syscalls/syscall_o32.tbl
+++ b/arch/mips/kernel/syscalls/syscall_o32.tbl
@@ -454,3 +454,5 @@
464 o32 getxattrat sys_getxattrat
465 o32 listxattrat sys_listxattrat
466 o32 removexattrat sys_removexattrat
+467 o32 getfsxattrat sys_getfsxattrat
+468 o32 setfsxattrat sys_setfsxattrat
diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl
index d9fc94c869657fcfbd7aca1d5f5abc9fae2fb9d8..b3578fac43d6b65167787fcc97d2d09f5a9828e7 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 d8b4ab78bef076bd50d49b87dea5060fd8c1686a..808045d82c9465c3bfa96b15947546efe5851e9a 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 e9115b4d8b635b846e5c9ad6ce229605323723a5..78dfc2c184d4815baf8a9e61c546c9936d58a47c 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 c8cad33bf250ea110de37bd1407f5a43ec5e38f2..d5a5c8339f0ed25ea07c4aba90351d352033c8a0 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 727f99d333b304b3db0711953a3d91ece18a28eb..817dcd8603bcbffc47f3f59aa3b74b16486453d0 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 4d0fb2fba7e208ae9455459afe11e277321d9f74..b4842c027c5d00c0236b2ba89387c5e2267447bd 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 5eb708bff1c791debd6cfc5322583b2ae53f6437..b6f0a7236aaee624cf9b484239a1068085a8ffe1 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 37effc1b134eea061f2c350c1d68b4436b65a4dd..425d56be337d1de22f205ac503df61ff86224fee 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 6b4c77268fc0ecace4ac78a9ca777fbffc277f4a..cdecb793b2ab5ab01e2333da4382919b94c7f65f 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,99 @@ 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 __user *, fsx, unsigned int, at_flags)
+{
+ struct fd dir;
+ struct fileattr fa;
+ struct path filepath;
+ int error;
+ unsigned int lookup_flags = 0;
+
+ if ((at_flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
+ return -EINVAL;
+
+ if (at_flags & AT_SYMLINK_FOLLOW)
+ lookup_flags |= LOOKUP_FOLLOW;
+
+ if (at_flags & AT_EMPTY_PATH)
+ lookup_flags |= LOOKUP_EMPTY;
+
+ 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, lookup_flags, &filepath);
+ if (error)
+ goto out;
+
+ 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 __user *, fsx, unsigned int, at_flags)
+{
+ struct fd dir;
+ struct fileattr fa;
+ struct path filepath;
+ int error;
+ unsigned int lookup_flags = 0;
+
+ if ((at_flags & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH)) != 0)
+ return -EINVAL;
+
+ if (at_flags & AT_SYMLINK_FOLLOW)
+ lookup_flags |= LOOKUP_FOLLOW;
+
+ if (at_flags & AT_EMPTY_PATH)
+ lookup_flags |= LOOKUP_EMPTY;
+
+ 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, lookup_flags, &filepath);
+ if (error)
+ goto out;
+
+ 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 638a36be31c14afc66a7fd6eb237d9545e8ad997..dc160c2ef145e4931d625f1f93c2a8ae7f87abf3 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;
@@ -646,6 +645,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 47c05a9851d0600964b644c9c7218faacfd865f8..8598e94b530b8b280a2697eaf918dd60f573d6ee 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 c6333204d45130eb022f6db460eea34a1f6e91db..3134d463d9af64c6e78adb37bff4b91f77b5305f 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, unsigned int at_flags);
+asmlinkage long sys_setfsxattrat(int dfd, const char __user *filename,
+ struct fsxattr *fsx, unsigned 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 88dc393c2bca38c0fa1b3fae579f7cfe4931223c..50be2e1007bc2779120d05c6e9512a689f86779c 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
---
base-commit: 4c538044ee2d11299cc57ac1e92d343e1e83b847
change-id: 20250114-xattrat-syscall-6a1136d2db59
Best regards,
--
Andrey Albershteyn <aalbersh@kernel.org>
^ permalink raw reply related
* Re: [PATCH v2] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Christian Brauner @ 2025-01-24 9:33 UTC (permalink / raw)
To: Andrey Albershteyn
Cc: linux-m68k, tglx, jcmvbkbc, linux-security-module, arnd,
linux-fsdevel, chris, npiggin, linuxppc-dev, mpe, luto, jack,
monstr, linux-arch, mingo, linux-alpha, christophe.leroy,
linux-sh, linux-parisc, naveen, bp, hpa, sparclinux, linux-kernel,
x86, maddy, dave.hansen, viro, linux-s390, linux-api, linux-xfs
In-Reply-To: <20250122-xattrat-syscall-v2-1-5b360d4fbcb2@kernel.org>
On Wed, Jan 22, 2025 at 03:18:34PM +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
By that you mean that you can use absolute or relative paths instead of
file descriptors?
> 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.
I'm not following this argument currently. In what sense does opening
special files return a VFS special inode and how does that prevent
FS_IOC_FSSEETXATTR from working? The inode in
static int ioctl_fssetxattr(struct file *file, void __user *argp)
{
struct mnt_idmap *idmap = file_mnt_idmap(file);
struct dentry *dentry = file->f_path.dentry;
d_inode(dentry)
and your:
error = user_path_at(dfd, filename, lookup_flags, &filepath);
if (error)
goto out;
d_inode(filepath.dentry)
is the same.
>
> 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
> CC: linux-fsdevel@vger.kernel.org
> CC: linux-xfs@vger.kernel.org
> Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
> ---
> v1:
> https://lore.kernel.org/linuxppc-dev/20250109174540.893098-1-aalbersh@kernel.org/
>
> 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/arm/tools/syscall.tbl | 2 +
> arch/arm64/tools/syscall_32.tbl | 2 +
> arch/m68k/kernel/syscalls/syscall.tbl | 2 +
> arch/microblaze/kernel/syscalls/syscall.tbl | 2 +
> arch/mips/kernel/syscalls/syscall_n32.tbl | 2 +
> arch/mips/kernel/syscalls/syscall_n64.tbl | 2 +
> arch/mips/kernel/syscalls/syscall_o32.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 | 99 +++++++++++++++++++++++++++++
> fs/ioctl.c | 16 ++++-
> include/linux/fileattr.h | 1 +
> include/linux/syscalls.h | 4 ++
> include/uapi/asm-generic/unistd.h | 8 ++-
> 21 files changed, 157 insertions(+), 3 deletions(-)
>
> diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl
> index c59d53d6d3f3490f976ca179ddfe02e69265ae4d..4b9e687494c16b60c6fd6ca1dc4d6564706a7e25 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/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
> index 49eeb2ad8dbd8e074c6240417693f23fb328afa8..66466257f3c2debb3e2299f0b608c6740c98cab2 100644
> --- a/arch/arm/tools/syscall.tbl
> +++ b/arch/arm/tools/syscall.tbl
> @@ -481,3 +481,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/arm64/tools/syscall_32.tbl b/arch/arm64/tools/syscall_32.tbl
> index 69a829912a05eb8a3e21ed701d1030e31c0148bc..9c516118b154811d8d11d5696f32817430320dbf 100644
> --- a/arch/arm64/tools/syscall_32.tbl
> +++ b/arch/arm64/tools/syscall_32.tbl
> @@ -478,3 +478,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/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl
> index f5ed71f1910d09769c845c2d062d99ee0449437c..159476387f394a92ee5e29db89b118c630372db2 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 680f568b77f2cbefc3eacb2517f276041f229b1e..a6d59ee740b58cacf823702003cf9bad17c0d3b7 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/mips/kernel/syscalls/syscall_n32.tbl b/arch/mips/kernel/syscalls/syscall_n32.tbl
> index 0b9b7e25b69ad592642f8533bee9ccfe95ce9626..cfe38fcebe1a0279e11751378d3e71c5ec6b6569 100644
> --- a/arch/mips/kernel/syscalls/syscall_n32.tbl
> +++ b/arch/mips/kernel/syscalls/syscall_n32.tbl
> @@ -405,3 +405,5 @@
> 464 n32 getxattrat sys_getxattrat
> 465 n32 listxattrat sys_listxattrat
> 466 n32 removexattrat sys_removexattrat
> +467 n32 getfsxattrat sys_getfsxattrat
> +468 n32 setfsxattrat sys_setfsxattrat
> diff --git a/arch/mips/kernel/syscalls/syscall_n64.tbl b/arch/mips/kernel/syscalls/syscall_n64.tbl
> index c844cd5cda620b2809a397cdd6f4315ab6a1bfe2..29a0c5974d1aa2f01e33edc0252d75fb97abe230 100644
> --- a/arch/mips/kernel/syscalls/syscall_n64.tbl
> +++ b/arch/mips/kernel/syscalls/syscall_n64.tbl
> @@ -381,3 +381,5 @@
> 464 n64 getxattrat sys_getxattrat
> 465 n64 listxattrat sys_listxattrat
> 466 n64 removexattrat sys_removexattrat
> +467 n64 getfsxattrat sys_getfsxattrat
> +468 n64 setfsxattrat sys_setfsxattrat
> diff --git a/arch/mips/kernel/syscalls/syscall_o32.tbl b/arch/mips/kernel/syscalls/syscall_o32.tbl
> index 349b8aad1159f404103bd2057a1e64e9bf309f18..6c00436807c57c492ba957fcd59af1202231cf80 100644
> --- a/arch/mips/kernel/syscalls/syscall_o32.tbl
> +++ b/arch/mips/kernel/syscalls/syscall_o32.tbl
> @@ -454,3 +454,5 @@
> 464 o32 getxattrat sys_getxattrat
> 465 o32 listxattrat sys_listxattrat
> 466 o32 removexattrat sys_removexattrat
> +467 o32 getfsxattrat sys_getfsxattrat
> +468 o32 setfsxattrat sys_setfsxattrat
> diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl
> index d9fc94c869657fcfbd7aca1d5f5abc9fae2fb9d8..b3578fac43d6b65167787fcc97d2d09f5a9828e7 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 d8b4ab78bef076bd50d49b87dea5060fd8c1686a..808045d82c9465c3bfa96b15947546efe5851e9a 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 e9115b4d8b635b846e5c9ad6ce229605323723a5..78dfc2c184d4815baf8a9e61c546c9936d58a47c 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 c8cad33bf250ea110de37bd1407f5a43ec5e38f2..d5a5c8339f0ed25ea07c4aba90351d352033c8a0 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 727f99d333b304b3db0711953a3d91ece18a28eb..817dcd8603bcbffc47f3f59aa3b74b16486453d0 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 4d0fb2fba7e208ae9455459afe11e277321d9f74..b4842c027c5d00c0236b2ba89387c5e2267447bd 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 5eb708bff1c791debd6cfc5322583b2ae53f6437..b6f0a7236aaee624cf9b484239a1068085a8ffe1 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 37effc1b134eea061f2c350c1d68b4436b65a4dd..425d56be337d1de22f205ac503df61ff86224fee 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 6b4c77268fc0ecace4ac78a9ca777fbffc277f4a..cdecb793b2ab5ab01e2333da4382919b94c7f65f 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,99 @@ 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 __user *, fsx, unsigned int, at_flags)
> +{
> + struct fd dir;
> + struct fileattr fa;
> + struct path filepath;
> + int error;
> + unsigned int lookup_flags = 0;
> +
> + if ((at_flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
> + return -EINVAL;
> +
> + if (at_flags & AT_SYMLINK_FOLLOW)
> + lookup_flags |= LOOKUP_FOLLOW;
> +
> + if (at_flags & AT_EMPTY_PATH)
> + lookup_flags |= LOOKUP_EMPTY;
> +
> + dir = fdget(dfd);
> + if (!fd_file(dir))
> + return -EBADF;
Please rely on scope-based cleanup:
CLASS(fd, f)(dfd);
if (fd_empty(f))
return -EBADF;
> +
> + if (!S_ISDIR(file_inode(fd_file(dir))->i_mode)) {
This isn't needed as user_path_at() will return ENOTDIR.
Your path as is wuld also preclude AT_EMPTY_PATH with directory file
descriptors and afaict there's various filesystems that seem to support
this on directory inodes.
> + error = -EBADF;
> + goto out;
> + }
> +
> + error = user_path_at(dfd, filename, lookup_flags, &filepath);
> + if (error)
> + goto out;
I'm confused. You're using fdget() above but then you don't use the
resulting file and call user_path_at() instead? Don't bother with
fdget() at all and just call into user_path_at() directly.
> +
> + 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 __user *, fsx, unsigned int, at_flags)
> +{
> + struct fd dir;
> + struct fileattr fa;
> + struct path filepath;
> + int error;
> + unsigned int lookup_flags = 0;
> +
> + if ((at_flags & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH)) != 0)
> + return -EINVAL;
> +
> + if (at_flags & AT_SYMLINK_FOLLOW)
> + lookup_flags |= LOOKUP_FOLLOW;
> +
> + if (at_flags & AT_EMPTY_PATH)
> + lookup_flags |= LOOKUP_EMPTY;
> +
> + 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, lookup_flags, &filepath);
> + if (error)
> + goto out;
Same problem as above. fdget() stuff isn't needed if you're calling
user_path_at() anyway.
> +
> + 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);
Just use the pattern:
error = user_path_at(dfd, filename, lookup_flags, &filepath);
if (error)
return error; /* once you've removed the fdget() direct return works fine */
error = mnt_want_write(filepath.mnt);
if (!error) {
error = vfs_fileattr_set(file_mnt_idmap(fd_file(dir)), filepath.dentry, &fa);
mnt_drop_write(filepath.mnt);
}
return error;
> +
> +out_path:
> + path_put(&filepath);
> +out:
> + fdput(dir);
> + return error;
> +}
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 638a36be31c14afc66a7fd6eb237d9545e8ad997..dc160c2ef145e4931d625f1f93c2a8ae7f87abf3 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;
>
> @@ -646,6 +645,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 47c05a9851d0600964b644c9c7218faacfd865f8..8598e94b530b8b280a2697eaf918dd60f573d6ee 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 c6333204d45130eb022f6db460eea34a1f6e91db..3134d463d9af64c6e78adb37bff4b91f77b5305f 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, unsigned int at_flags);
> +asmlinkage long sys_setfsxattrat(int dfd, const char __user *filename,
> + struct fsxattr *fsx, unsigned 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 88dc393c2bca38c0fa1b3fae579f7cfe4931223c..50be2e1007bc2779120d05c6e9512a689f86779c 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
>
> ---
> base-commit: 4c538044ee2d11299cc57ac1e92d343e1e83b847
> change-id: 20250114-xattrat-syscall-6a1136d2db59
>
> Best regards,
> --
> Andrey Albershteyn <aalbersh@kernel.org>
>
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Eyal Birger @ 2025-01-27 19:24 UTC (permalink / raw)
To: Kees Cook
Cc: luto, wad, oleg, ldv, mhiramat, andrii, jolsa, alexei.starovoitov,
olsajiri, cyphar, songliubraving, yhs, john.fastabend, peterz,
tglx, bp, daniel, ast, andrii.nakryiko, rostedt, rafi,
shmulik.ladkani, bpf, linux-api, linux-trace-kernel, x86,
linux-kernel, stable
In-Reply-To: <202501201334.604217B7@keescook>
Hi Kees,
On Mon, Jan 20, 2025 at 1:34 PM Kees Cook <kees@kernel.org> wrote:
>
> On Sat, Jan 18, 2025 at 07:39:25PM -0800, Eyal Birger wrote:
> > Alternatively, maybe this syscall implementation should be reverted?
>
> Honestly, that seems the best choice. I don't think any thought was
> given to how it would interact with syscall interposers (including
> ptrace, strict mode seccomp, etc).
I don't know if you noticed Andrii's and others' comments on this [1].
Given that:
- this issue requires immediate remediation
- there seems to be pushback for reverting the syscall implementation
- filtering uretprobe is not within the capabilities of seccomp without this
syscall (so reverting the syscall is equivalent to just passing it through
seccomp)
is it possible to consider applying this current fix, with the possibility of
extending seccomp in the future to support filtering uretprobe if deemed
necessary (for example by allowing userspace to define a stricter policy)?
Thanks,
Eyal.
[1] https://lore.kernel.org/lkml/20250121182939.33d05470@gandalf.local.home/T/#me2676c378eff2d6a33f3054fed4a5f3afa64e65b
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Kees Cook @ 2025-01-27 19:33 UTC (permalink / raw)
To: Eyal Birger
Cc: luto, wad, oleg, ldv, mhiramat, andrii, jolsa, alexei.starovoitov,
olsajiri, cyphar, songliubraving, yhs, john.fastabend, peterz,
tglx, bp, daniel, ast, andrii.nakryiko, rostedt, rafi,
shmulik.ladkani, bpf, linux-api, linux-trace-kernel, x86,
linux-kernel, stable
In-Reply-To: <CAHsH6Gt4EqSz6TrQa+JKG98y8CUTtOM8=dfCVy0fZ8pwXJr1pw@mail.gmail.com>
On Mon, Jan 27, 2025 at 11:24:02AM -0800, Eyal Birger wrote:
> Hi Kees,
>
> On Mon, Jan 20, 2025 at 1:34 PM Kees Cook <kees@kernel.org> wrote:
> >
> > On Sat, Jan 18, 2025 at 07:39:25PM -0800, Eyal Birger wrote:
> > > Alternatively, maybe this syscall implementation should be reverted?
> >
> > Honestly, that seems the best choice. I don't think any thought was
> > given to how it would interact with syscall interposers (including
> > ptrace, strict mode seccomp, etc).
>
> I don't know if you noticed Andrii's and others' comments on this [1].
>
> Given that:
> - this issue requires immediate remediation
> - there seems to be pushback for reverting the syscall implementation
> - filtering uretprobe is not within the capabilities of seccomp without this
> syscall (so reverting the syscall is equivalent to just passing it through
> seccomp)
>
> is it possible to consider applying this current fix, with the possibility of
> extending seccomp in the future to support filtering uretprobe if deemed
> necessary (for example by allowing userspace to define a stricter policy)?
I still think this is a Docker problem, but I agree that uretprobe
without syscall is just as unfilterable as seccomp ignoring the syscall.
Can you please update the patch to use the existing action_cache bitmaps
instead of adding an open-coded check? We can consider adding
syscall_restart to this as well in the future...
--
Kees Cook
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Eyal Birger @ 2025-01-27 19:39 UTC (permalink / raw)
To: Kees Cook
Cc: luto, wad, oleg, ldv, mhiramat, andrii, jolsa, alexei.starovoitov,
olsajiri, cyphar, songliubraving, yhs, john.fastabend, peterz,
tglx, bp, daniel, ast, andrii.nakryiko, rostedt, rafi,
shmulik.ladkani, bpf, linux-api, linux-trace-kernel, x86,
linux-kernel, stable
In-Reply-To: <202501271131.7B5C22D@keescook>
On Mon, Jan 27, 2025 at 11:33 AM Kees Cook <kees@kernel.org> wrote:
>
> On Mon, Jan 27, 2025 at 11:24:02AM -0800, Eyal Birger wrote:
> > Hi Kees,
> >
> > On Mon, Jan 20, 2025 at 1:34 PM Kees Cook <kees@kernel.org> wrote:
> > >
> > > On Sat, Jan 18, 2025 at 07:39:25PM -0800, Eyal Birger wrote:
> > > > Alternatively, maybe this syscall implementation should be reverted?
> > >
> > > Honestly, that seems the best choice. I don't think any thought was
> > > given to how it would interact with syscall interposers (including
> > > ptrace, strict mode seccomp, etc).
> >
> > I don't know if you noticed Andrii's and others' comments on this [1].
> >
> > Given that:
> > - this issue requires immediate remediation
> > - there seems to be pushback for reverting the syscall implementation
> > - filtering uretprobe is not within the capabilities of seccomp without this
> > syscall (so reverting the syscall is equivalent to just passing it through
> > seccomp)
> >
> > is it possible to consider applying this current fix, with the possibility of
> > extending seccomp in the future to support filtering uretprobe if deemed
> > necessary (for example by allowing userspace to define a stricter policy)?
>
> I still think this is a Docker problem, but I agree that uretprobe
> without syscall is just as unfilterable as seccomp ignoring the syscall.
>
> Can you please update the patch to use the existing action_cache bitmaps
> instead of adding an open-coded check? We can consider adding
> syscall_restart to this as well in the future...
I can. The main difference as far as I can tell is that it would not
apply to strict mode. Is that OK? it means that existing binaries using
strict mode would still crash if uretprobe is attached to them.
Eyal.
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Kees Cook @ 2025-01-27 19:43 UTC (permalink / raw)
To: Eyal Birger
Cc: luto, wad, oleg, ldv, mhiramat, andrii, jolsa, alexei.starovoitov,
olsajiri, cyphar, songliubraving, yhs, john.fastabend, peterz,
tglx, bp, daniel, ast, andrii.nakryiko, rostedt, rafi,
shmulik.ladkani, bpf, linux-api, linux-trace-kernel, x86,
linux-kernel, stable
In-Reply-To: <CAHsH6GtPBt329FeN7K4X4Hqc_uZ=a8uofDN15mqqC4obQ-RK5g@mail.gmail.com>
On Mon, Jan 27, 2025 at 11:39:44AM -0800, Eyal Birger wrote:
> On Mon, Jan 27, 2025 at 11:33 AM Kees Cook <kees@kernel.org> wrote:
> >
> > On Mon, Jan 27, 2025 at 11:24:02AM -0800, Eyal Birger wrote:
> > > Hi Kees,
> > >
> > > On Mon, Jan 20, 2025 at 1:34 PM Kees Cook <kees@kernel.org> wrote:
> > > >
> > > > On Sat, Jan 18, 2025 at 07:39:25PM -0800, Eyal Birger wrote:
> > > > > Alternatively, maybe this syscall implementation should be reverted?
> > > >
> > > > Honestly, that seems the best choice. I don't think any thought was
> > > > given to how it would interact with syscall interposers (including
> > > > ptrace, strict mode seccomp, etc).
> > >
> > > I don't know if you noticed Andrii's and others' comments on this [1].
> > >
> > > Given that:
> > > - this issue requires immediate remediation
> > > - there seems to be pushback for reverting the syscall implementation
> > > - filtering uretprobe is not within the capabilities of seccomp without this
> > > syscall (so reverting the syscall is equivalent to just passing it through
> > > seccomp)
> > >
> > > is it possible to consider applying this current fix, with the possibility of
> > > extending seccomp in the future to support filtering uretprobe if deemed
> > > necessary (for example by allowing userspace to define a stricter policy)?
> >
> > I still think this is a Docker problem, but I agree that uretprobe
> > without syscall is just as unfilterable as seccomp ignoring the syscall.
> >
> > Can you please update the patch to use the existing action_cache bitmaps
> > instead of adding an open-coded check? We can consider adding
> > syscall_restart to this as well in the future...
>
> I can. The main difference as far as I can tell is that it would not
> apply to strict mode. Is that OK? it means that existing binaries using
> strict mode would still crash if uretprobe is attached to them.
Ah, good point. Please also add it to mode1_syscalls for strict. :)
--
Kees Cook
^ permalink raw reply
* [PATCH v3 0/6] ptrace: introduce PTRACE_SET_SYSCALL_INFO API
From: Dmitry V. Levin @ 2025-01-28 9:14 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Alexey Gladkov, Charlie Jenkins, Eugene Syromyatnikov,
Mike Frysinger, Renzo Davoli, Davide Berardi, strace-devel,
Vineet Gupta, Russell King, Will Deacon, Guo Ren, Brian Cain,
Huacai Chen, WANG Xuerui, Geert Uytterhoeven, Michal Simek,
Thomas Bogendoerfer, Dinh Nguyen, Jonas Bonn, Stefan Kristiansson,
Stafford Horne, James E.J. Bottomley, Helge Deller,
Michael Ellerman, Nicholas Piggin, Christophe Leroy, Naveen N Rao,
Madhavan Srinivasan, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
Christian Borntraeger, Sven Schnelle, Yoshinori Sato, Rich Felker,
John Paul Adrian Glaubitz, David S. Miller, Andreas Larsson,
Richard Weinberger, Anton Ivanov, Johannes Berg, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin,
Chris Zankel, Max Filippov, Arnd Bergmann, Shuah Khan,
linux-snps-arc, linux-kernel, linux-arm-kernel, linux-csky,
linux-hexagon, loongarch, linux-m68k, linux-mips, linux-openrisc,
linux-parisc, linuxppc-dev, linux-riscv, linux-s390, linux-sh,
sparclinux, linux-um, linux-arch, linux-kselftest, linux-api
PTRACE_SET_SYSCALL_INFO is a generic ptrace API that complements
PTRACE_GET_SYSCALL_INFO by letting the ptracer modify details of
system calls the tracee is blocked in.
This API allows ptracers to obtain and modify system call details
in a straightforward and architecture-agnostic way.
Current implementation supports changing only those bits of system call
information that are used by strace, namely, syscall number, syscall
arguments, and syscall return value.
Support of changing additional details returned by PTRACE_GET_SYSCALL_INFO,
such as instruction pointer and stack pointer, could be added later if
needed, by using struct ptrace_syscall_info.flags to specify the additional
details that should be set. Currently, "flags", "reserved", and
"seccomp.reserved2" fields of struct ptrace_syscall_info must be
initialized with zeroes; "arch", "instruction_pointer", and "stack_pointer"
fields are ignored.
PTRACE_SET_SYSCALL_INFO currently supports only PTRACE_SYSCALL_INFO_ENTRY,
PTRACE_SYSCALL_INFO_EXIT, and PTRACE_SYSCALL_INFO_SECCOMP operations.
Other operations could be added later if needed.
Ideally, PTRACE_SET_SYSCALL_INFO should have been introduced along with
PTRACE_GET_SYSCALL_INFO, but it didn't happen. The last straw that
convinced me to implement PTRACE_SET_SYSCALL_INFO was apparent failure
to provide an API of changing the first system call argument on riscv
architecture [1].
ptrace(2) man page:
long ptrace(enum __ptrace_request request, pid_t pid, void *addr, void *data);
...
PTRACE_SET_SYSCALL_INFO
Modify information about the system call that caused the stop.
The "data" argument is a pointer to struct ptrace_syscall_info
that specifies the system call information to be set.
The "addr" argument should be set to sizeof(struct ptrace_syscall_info)).
[1] https://lore.kernel.org/all/59505464-c84a-403d-972f-d4b2055eeaac@gmail.com/
Notes:
v3:
* powerpc: Submit syscall_set_return_value fix for "sc" case separately
* mips: Do not introduce erroneous argument truncation on mips n32,
add a detailed description to the commit message of the
mips_get_syscall_arg change
* ptrace: Add explicit padding to the end of struct ptrace_syscall_info,
simplify obtaining of user ptrace_syscall_info,
do not introduce PTRACE_SYSCALL_INFO_SIZE_VER0
* ptrace: Change the return type of ptrace_set_syscall_info_* functions
from "unsigned long" to "int"
* ptrace: Add -ERANGE check to ptrace_set_syscall_info_exit,
add comments to -ERANGE checks
* ptrace: Update comments about supported syscall stops
* selftests: Extend set_syscall_info test, fix for mips n32
* Add Tested-by and Reviewed-by
v2:
* Add patch to fix syscall_set_return_value() on powerpc
* Add patch to fix mips_get_syscall_arg() on mips
* Add syscall_set_return_value() implementation on hexagon
* Add syscall_set_return_value() invocation to syscall_set_nr()
on arm and arm64.
* Fix syscall_set_nr() and mips_set_syscall_arg() on mips
* Add a comment to syscall_set_nr() on arc, powerpc, s390, sh,
and sparc
* Remove redundant ptrace_syscall_info.op assignments in
ptrace_get_syscall_info_*
* Minor style tweaks in ptrace_get_syscall_info_op()
* Remove syscall_set_return_value() invocation from
ptrace_set_syscall_info_entry()
* Skip syscall_set_arguments() invocation in case of syscall number -1
in ptrace_set_syscall_info_entry()
* Split ptrace_syscall_info.reserved into ptrace_syscall_info.reserved
and ptrace_syscall_info.flags
* Use __kernel_ulong_t instead of unsigned long in set_syscall_info test
Dmitry V. Levin (6):
mips: fix mips_get_syscall_arg() for o32
syscall.h: add syscall_set_arguments() and syscall_set_return_value()
syscall.h: introduce syscall_set_nr()
ptrace_get_syscall_info: factor out ptrace_get_syscall_info_op
ptrace: introduce PTRACE_SET_SYSCALL_INFO request
selftests/ptrace: add a test case for PTRACE_SET_SYSCALL_INFO
arch/arc/include/asm/syscall.h | 25 +
arch/arm/include/asm/syscall.h | 37 ++
arch/arm64/include/asm/syscall.h | 29 +
arch/csky/include/asm/syscall.h | 13 +
arch/hexagon/include/asm/syscall.h | 21 +
arch/loongarch/include/asm/syscall.h | 15 +
arch/m68k/include/asm/syscall.h | 7 +
arch/microblaze/include/asm/syscall.h | 7 +
arch/mips/include/asm/syscall.h | 70 ++-
arch/nios2/include/asm/syscall.h | 16 +
arch/openrisc/include/asm/syscall.h | 13 +
arch/parisc/include/asm/syscall.h | 19 +
arch/powerpc/include/asm/syscall.h | 20 +
arch/riscv/include/asm/syscall.h | 16 +
arch/s390/include/asm/syscall.h | 24 +
arch/sh/include/asm/syscall_32.h | 24 +
arch/sparc/include/asm/syscall.h | 22 +
arch/um/include/asm/syscall-generic.h | 19 +
arch/x86/include/asm/syscall.h | 43 ++
arch/xtensa/include/asm/syscall.h | 18 +
include/asm-generic/syscall.h | 30 +
include/uapi/linux/ptrace.h | 7 +-
kernel/ptrace.c | 179 +++++-
tools/testing/selftests/ptrace/Makefile | 2 +-
.../selftests/ptrace/set_syscall_info.c | 514 ++++++++++++++++++
25 files changed, 1143 insertions(+), 47 deletions(-)
create mode 100644 tools/testing/selftests/ptrace/set_syscall_info.c
--
ldv
^ permalink raw reply
* [PATCH v3 5/6] ptrace: introduce PTRACE_SET_SYSCALL_INFO request
From: Dmitry V. Levin @ 2025-01-28 9:16 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Alexey Gladkov, Charlie Jenkins, Eugene Syromyatnikov,
Mike Frysinger, Renzo Davoli, Davide Berardi, strace-devel,
linux-kernel, linux-api
In-Reply-To: <20250128091445.GA8257@strace.io>
PTRACE_SET_SYSCALL_INFO is a generic ptrace API that complements
PTRACE_GET_SYSCALL_INFO by letting the ptracer modify details of
system calls the tracee is blocked in.
This API allows ptracers to obtain and modify system call details
in a straightforward and architecture-agnostic way.
Current implementation supports changing only those bits of system call
information that are used by strace, namely, syscall number, syscall
arguments, and syscall return value.
Support of changing additional details returned by PTRACE_GET_SYSCALL_INFO,
such as instruction pointer and stack pointer, could be added later if
needed, by using struct ptrace_syscall_info.flags to specify the additional
details that should be set. Currently, "flags", "reserved", and
"seccomp.reserved2" fields of struct ptrace_syscall_info must be
initialized with zeroes; "arch", "instruction_pointer", and "stack_pointer"
fields are ignored.
PTRACE_SET_SYSCALL_INFO currently supports only PTRACE_SYSCALL_INFO_ENTRY,
PTRACE_SYSCALL_INFO_EXIT, and PTRACE_SYSCALL_INFO_SECCOMP operations.
Other operations could be added later if needed.
Ideally, PTRACE_SET_SYSCALL_INFO should have been introduced along with
PTRACE_GET_SYSCALL_INFO, but it didn't happen. The last straw that
convinced me to implement PTRACE_SET_SYSCALL_INFO was apparent failure
to provide an API of changing the first system call argument on riscv
architecture.
ptrace(2) man page:
long ptrace(enum __ptrace_request request, pid_t pid, void *addr, void *data);
...
PTRACE_SET_SYSCALL_INFO
Modify information about the system call that caused the stop.
The "data" argument is a pointer to struct ptrace_syscall_info
that specifies the system call information to be set.
The "addr" argument should be set to sizeof(struct ptrace_syscall_info)).
Link: https://lore.kernel.org/all/59505464-c84a-403d-972f-d4b2055eeaac@gmail.com/
Signed-off-by: Dmitry V. Levin <ldv@strace.io>
Reviewed-by: Alexey Gladkov <legion@kernel.org>
Reviewed-by: Charlie Jenkins <charlie@rivosinc.com>
Tested-by: Charlie Jenkins <charlie@rivosinc.com>
---
include/uapi/linux/ptrace.h | 7 ++-
kernel/ptrace.c | 121 +++++++++++++++++++++++++++++++++++-
2 files changed, 126 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
index 72c038fc71d0..5f8ef6156752 100644
--- a/include/uapi/linux/ptrace.h
+++ b/include/uapi/linux/ptrace.h
@@ -74,6 +74,7 @@ struct seccomp_metadata {
};
#define PTRACE_GET_SYSCALL_INFO 0x420e
+#define PTRACE_SET_SYSCALL_INFO 0x4212
#define PTRACE_SYSCALL_INFO_NONE 0
#define PTRACE_SYSCALL_INFO_ENTRY 1
#define PTRACE_SYSCALL_INFO_EXIT 2
@@ -81,7 +82,8 @@ struct seccomp_metadata {
struct ptrace_syscall_info {
__u8 op; /* PTRACE_SYSCALL_INFO_* */
- __u8 pad[3];
+ __u8 reserved;
+ __u16 flags;
__u32 arch;
__u64 instruction_pointer;
__u64 stack_pointer;
@@ -98,6 +100,7 @@ struct ptrace_syscall_info {
__u64 nr;
__u64 args[6];
__u32 ret_data;
+ __u32 reserved2;
} seccomp;
};
};
@@ -142,6 +145,8 @@ struct ptrace_sud_config {
__u64 len;
};
+/* 0x4212 is PTRACE_SET_SYSCALL_INFO */
+
/*
* These values are stored in task->ptrace_message
* by ptrace_stop to describe the current syscall-stop.
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 22e7d74cf4cd..b9c1949186bf 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -944,7 +944,10 @@ ptrace_get_syscall_info_seccomp(struct task_struct *child, struct pt_regs *regs,
ptrace_get_syscall_info_entry(child, regs, info);
info->seccomp.ret_data = child->ptrace_message;
- /* ret_data is the last field in struct ptrace_syscall_info.seccomp */
+ /*
+ * ret_data is the last non-reserved field
+ * in struct ptrace_syscall_info.seccomp
+ */
return offsetofend(struct ptrace_syscall_info, seccomp.ret_data);
}
@@ -1016,6 +1019,118 @@ ptrace_get_syscall_info(struct task_struct *child, unsigned long user_size,
write_size = min(actual_size, user_size);
return copy_to_user(datavp, &info, write_size) ? -EFAULT : actual_size;
}
+
+static int
+ptrace_set_syscall_info_entry(struct task_struct *child, struct pt_regs *regs,
+ struct ptrace_syscall_info *info)
+{
+ unsigned long args[ARRAY_SIZE(info->entry.args)];
+ int nr = info->entry.nr;
+ int i;
+
+ /*
+ * Check that the syscall number specified in info->entry.nr
+ * is either a value of type "int" or a sign-extended value
+ * of type "int".
+ */
+ if (nr != info->entry.nr)
+ return -ERANGE;
+
+ for (i = 0; i < ARRAY_SIZE(args); i++) {
+ args[i] = info->entry.args[i];
+ /*
+ * Check that the syscall argument specified in
+ * info->entry.args[i] is either a value of type
+ * "unsigned long" or a sign-extended value of type "long".
+ */
+ if (args[i] != info->entry.args[i])
+ return -ERANGE;
+ }
+
+ syscall_set_nr(child, regs, nr);
+ /*
+ * If the syscall number is set to -1, setting syscall arguments is not
+ * just pointless, it would also clobber the syscall return value on
+ * those architectures that share the same register both for the first
+ * argument of syscall and its return value.
+ */
+ if (nr != -1)
+ syscall_set_arguments(child, regs, args);
+
+ return 0;
+}
+
+static int
+ptrace_set_syscall_info_seccomp(struct task_struct *child, struct pt_regs *regs,
+ struct ptrace_syscall_info *info)
+{
+ /*
+ * info->entry is currently a subset of info->seccomp,
+ * info->seccomp.ret_data is currently ignored.
+ */
+ return ptrace_set_syscall_info_entry(child, regs, info);
+}
+
+static int
+ptrace_set_syscall_info_exit(struct task_struct *child, struct pt_regs *regs,
+ struct ptrace_syscall_info *info)
+{
+ long rval = info->exit.rval;
+
+ /*
+ * Check that the return value specified in info->exit.rval
+ * is either a value of type "long" or a sign-extended value
+ * of type "long".
+ */
+ if (rval != info->exit.rval)
+ return -ERANGE;
+
+ if (info->exit.is_error)
+ syscall_set_return_value(child, regs, rval, 0);
+ else
+ syscall_set_return_value(child, regs, 0, rval);
+
+ return 0;
+}
+
+static int
+ptrace_set_syscall_info(struct task_struct *child, unsigned long user_size,
+ const void __user *datavp)
+{
+ struct pt_regs *regs = task_pt_regs(child);
+ struct ptrace_syscall_info info;
+
+ if (user_size < sizeof(info))
+ return -EINVAL;
+
+ /*
+ * The compatibility is tracked by info.op and info.flags: if user-space
+ * does not instruct us to use unknown extra bits from future versions
+ * of ptrace_syscall_info, we are not going to read them either.
+ */
+ if (copy_from_user(&info, datavp, sizeof(info)))
+ return -EFAULT;
+
+ /* Reserved for future use. */
+ if (info.flags || info.reserved || info.seccomp.reserved2)
+ return -EINVAL;
+
+ /* Changing the type of the system call stop is not supported yet. */
+ if (ptrace_get_syscall_info_op(child) != info.op)
+ return -EINVAL;
+
+ switch (info.op) {
+ case PTRACE_SYSCALL_INFO_ENTRY:
+ return ptrace_set_syscall_info_entry(child, regs, &info);
+ case PTRACE_SYSCALL_INFO_EXIT:
+ return ptrace_set_syscall_info_exit(child, regs, &info);
+ case PTRACE_SYSCALL_INFO_SECCOMP:
+ return ptrace_set_syscall_info_seccomp(child, regs, &info);
+ default:
+ /* Other types of system call stops are not supported yet. */
+ return -EINVAL;
+ }
+}
#endif /* CONFIG_HAVE_ARCH_TRACEHOOK */
int ptrace_request(struct task_struct *child, long request,
@@ -1234,6 +1349,10 @@ int ptrace_request(struct task_struct *child, long request,
case PTRACE_GET_SYSCALL_INFO:
ret = ptrace_get_syscall_info(child, addr, datavp);
break;
+
+ case PTRACE_SET_SYSCALL_INFO:
+ ret = ptrace_set_syscall_info(child, addr, datavp);
+ break;
#endif
case PTRACE_SECCOMP_GET_FILTER:
--
ldv
^ permalink raw reply related
* [PATCH v2] seccomp: passthrough uretprobe systemcall without filtering
From: Eyal Birger @ 2025-01-28 14:58 UTC (permalink / raw)
To: kees, luto, wad, oleg, mhiramat, andrii, jolsa
Cc: alexei.starovoitov, olsajiri, cyphar, songliubraving, yhs,
john.fastabend, peterz, tglx, bp, daniel, ast, andrii.nakryiko,
rostedt, rafi, shmulik.ladkani, bpf, linux-api,
linux-trace-kernel, x86, linux-kernel, Eyal Birger, stable
When attaching uretprobes to processes running inside docker, the attached
process is segfaulted when encountering the retprobe.
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. This is true for other userspace applications which use seccomp
to control their syscall surface.
Since uretprobe is a "kernel implementation detail" system call which is
not used by userspace application code directly, it is impractical and
there's very little point in forcing all userspace applications to
explicitly allow it in order to avoid crashing tracked processes.
Pass this systemcall through seccomp without depending on configuration.
Note: uretprobe isn't supported in i386 and __NR_ia32_rt_tgsigqueueinfo
uses the same number as __NR_uretprobe so the syscall isn't forced in the
compat bitmap.
Fixes: ff474a78cef5 ("uprobe: Add uretprobe syscall to speed up return probe")
Reported-by: Rafael Buchbinder <rafi@rbk.io>
Link: https://lore.kernel.org/lkml/CAHsH6Gs3Eh8DFU0wq58c_LF8A4_+o6z456J7BidmcVY2AqOnHQ@mail.gmail.com/
Link: https://lore.kernel.org/lkml/20250121182939.33d05470@gandalf.local.home/T/#me2676c378eff2d6a33f3054fed4a5f3afa64e65b
Cc: stable@vger.kernel.org
Signed-off-by: Eyal Birger <eyal.birger@gmail.com>
---
v2: use action_cache bitmap and mode1 array to check the syscall
The following reproduction script synthetically demonstrates the problem
for seccomp filters:
cat > /tmp/x.c << EOF
char *syscalls[] = {
"write",
"exit_group",
"fstat",
};
__attribute__((noinline)) int probed(void)
{
printf("Probed\n");
return 1;
}
void apply_seccomp_filter(char **syscalls, int num_syscalls)
{
scmp_filter_ctx ctx;
ctx = seccomp_init(SCMP_ACT_KILL);
for (int i = 0; i < num_syscalls; i++) {
seccomp_rule_add(ctx, SCMP_ACT_ALLOW,
seccomp_syscall_resolve_name(syscalls[i]), 0);
}
seccomp_load(ctx);
seccomp_release(ctx);
}
int main(int argc, char *argv[])
{
int num_syscalls = sizeof(syscalls) / sizeof(syscalls[0]);
apply_seccomp_filter(syscalls, num_syscalls);
probed();
return 0;
}
EOF
cat > /tmp/trace.bt << EOF
uretprobe:/tmp/x:probed
{
printf("ret=%d\n", retval);
}
EOF
gcc -o /tmp/x /tmp/x.c -lseccomp
/usr/bin/bpftrace /tmp/trace.bt &
sleep 5 # wait for uretprobe attach
/tmp/x
pkill bpftrace
rm /tmp/x /tmp/x.c /tmp/trace.bt
---
kernel/seccomp.c | 24 +++++++++++++++++++++---
1 file changed, 21 insertions(+), 3 deletions(-)
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 385d48293a5f..23b594a68bc0 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -734,13 +734,13 @@ seccomp_prepare_user_filter(const char __user *user_filter)
#ifdef SECCOMP_ARCH_NATIVE
/**
- * seccomp_is_const_allow - check if filter is constant allow with given data
+ * seccomp_is_filter_const_allow - check if filter is constant allow with given data
* @fprog: The BPF programs
* @sd: The seccomp data to check against, only syscall number and arch
* number are considered constant.
*/
-static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
- struct seccomp_data *sd)
+static bool seccomp_is_filter_const_allow(struct sock_fprog_kern *fprog,
+ struct seccomp_data *sd)
{
unsigned int reg_value = 0;
unsigned int pc;
@@ -812,6 +812,21 @@ static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
return false;
}
+static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
+ struct seccomp_data *sd)
+{
+#ifdef __NR_uretprobe
+ if (sd->nr == __NR_uretprobe
+#ifdef SECCOMP_ARCH_COMPAT
+ && sd->arch != SECCOMP_ARCH_COMPAT
+#endif
+ )
+ return true;
+#endif
+
+ return seccomp_is_filter_const_allow(fprog, sd);
+}
+
static void seccomp_cache_prepare_bitmap(struct seccomp_filter *sfilter,
void *bitmap, const void *bitmap_prev,
size_t bitmap_size, int arch)
@@ -1023,6 +1038,9 @@ static inline void seccomp_log(unsigned long syscall, long signr, u32 action,
*/
static const int mode1_syscalls[] = {
__NR_seccomp_read, __NR_seccomp_write, __NR_seccomp_exit, __NR_seccomp_sigreturn,
+#ifdef __NR_uretprobe
+ __NR_uretprobe,
+#endif
-1, /* negative terminated */
};
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v2] seccomp: passthrough uretprobe systemcall without filtering
From: Oleg Nesterov @ 2025-01-28 15:44 UTC (permalink / raw)
To: Eyal Birger
Cc: kees, luto, wad, mhiramat, andrii, jolsa, alexei.starovoitov,
olsajiri, cyphar, songliubraving, yhs, john.fastabend, peterz,
tglx, bp, daniel, ast, andrii.nakryiko, rostedt, rafi,
shmulik.ladkani, bpf, linux-api, linux-trace-kernel, x86,
linux-kernel, stable
In-Reply-To: <20250128145806.1849977-1-eyal.birger@gmail.com>
can't review, I know nothing about seccomp_cache, but
On 01/28, Eyal Birger wrote:
>
> +static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
> + struct seccomp_data *sd)
> +{
> +#ifdef __NR_uretprobe
> + if (sd->nr == __NR_uretprobe
> +#ifdef SECCOMP_ARCH_COMPAT
> + && sd->arch != SECCOMP_ARCH_COMPAT
> +#endif
it seems you can check
&& sd->arch == SECCOMP_ARCH_NATIVE
and avoid #ifdef SECCOMP_ARCH_COMPAT
Oleg.
^ permalink raw reply
* Re: [PATCH v3 5/6] ptrace: introduce PTRACE_SET_SYSCALL_INFO request
From: Christophe Leroy @ 2025-01-28 15:21 UTC (permalink / raw)
To: Dmitry V. Levin, Oleg Nesterov
Cc: Alexey Gladkov, Charlie Jenkins, Eugene Syromyatnikov,
Mike Frysinger, Renzo Davoli, Davide Berardi, strace-devel,
linux-kernel, linux-api
In-Reply-To: <20250128091650.GE8601@strace.io>
Le 28/01/2025 à 10:16, Dmitry V. Levin a écrit :
> PTRACE_SET_SYSCALL_INFO is a generic ptrace API that complements
> PTRACE_GET_SYSCALL_INFO by letting the ptracer modify details of
> system calls the tracee is blocked in.
>
> This API allows ptracers to obtain and modify system call details
> in a straightforward and architecture-agnostic way.
>
> Current implementation supports changing only those bits of system call
> information that are used by strace, namely, syscall number, syscall
> arguments, and syscall return value.
How do you handle changes related to syscalls that call
force_successful_syscall_return() ?
>
> Support of changing additional details returned by PTRACE_GET_SYSCALL_INFO,
> such as instruction pointer and stack pointer, could be added later if
> needed, by using struct ptrace_syscall_info.flags to specify the additional
> details that should be set. Currently, "flags", "reserved", and
> "seccomp.reserved2" fields of struct ptrace_syscall_info must be
> initialized with zeroes; "arch", "instruction_pointer", and "stack_pointer"
> fields are ignored.
>
> PTRACE_SET_SYSCALL_INFO currently supports only PTRACE_SYSCALL_INFO_ENTRY,
> PTRACE_SYSCALL_INFO_EXIT, and PTRACE_SYSCALL_INFO_SECCOMP operations.
> Other operations could be added later if needed.
>
> Ideally, PTRACE_SET_SYSCALL_INFO should have been introduced along with
> PTRACE_GET_SYSCALL_INFO, but it didn't happen. The last straw that
> convinced me to implement PTRACE_SET_SYSCALL_INFO was apparent failure
> to provide an API of changing the first system call argument on riscv
> architecture.
>
> ptrace(2) man page:
>
> long ptrace(enum __ptrace_request request, pid_t pid, void *addr, void *data);
> ...
> PTRACE_SET_SYSCALL_INFO
> Modify information about the system call that caused the stop.
> The "data" argument is a pointer to struct ptrace_syscall_info
> that specifies the system call information to be set.
> The "addr" argument should be set to sizeof(struct ptrace_syscall_info)).
>
> Link: https://lore.kernel.org/all/59505464-c84a-403d-972f-d4b2055eeaac@gmail.com/
> Signed-off-by: Dmitry V. Levin <ldv@strace.io>
> Reviewed-by: Alexey Gladkov <legion@kernel.org>
> Reviewed-by: Charlie Jenkins <charlie@rivosinc.com>
> Tested-by: Charlie Jenkins <charlie@rivosinc.com>
> ---
> include/uapi/linux/ptrace.h | 7 ++-
> kernel/ptrace.c | 121 +++++++++++++++++++++++++++++++++++-
> 2 files changed, 126 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
> index 72c038fc71d0..5f8ef6156752 100644
> --- a/include/uapi/linux/ptrace.h
> +++ b/include/uapi/linux/ptrace.h
> @@ -74,6 +74,7 @@ struct seccomp_metadata {
> };
>
> #define PTRACE_GET_SYSCALL_INFO 0x420e
> +#define PTRACE_SET_SYSCALL_INFO 0x4212
> #define PTRACE_SYSCALL_INFO_NONE 0
> #define PTRACE_SYSCALL_INFO_ENTRY 1
> #define PTRACE_SYSCALL_INFO_EXIT 2
> @@ -81,7 +82,8 @@ struct seccomp_metadata {
>
> struct ptrace_syscall_info {
> __u8 op; /* PTRACE_SYSCALL_INFO_* */
> - __u8 pad[3];
> + __u8 reserved;
> + __u16 flags;
> __u32 arch;
> __u64 instruction_pointer;
> __u64 stack_pointer;
> @@ -98,6 +100,7 @@ struct ptrace_syscall_info {
> __u64 nr;
> __u64 args[6];
> __u32 ret_data;
> + __u32 reserved2;
> } seccomp;
> };
> };
> @@ -142,6 +145,8 @@ struct ptrace_sud_config {
> __u64 len;
> };
>
> +/* 0x4212 is PTRACE_SET_SYSCALL_INFO */
> +
> /*
> * These values are stored in task->ptrace_message
> * by ptrace_stop to describe the current syscall-stop.
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 22e7d74cf4cd..b9c1949186bf 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -944,7 +944,10 @@ ptrace_get_syscall_info_seccomp(struct task_struct *child, struct pt_regs *regs,
> ptrace_get_syscall_info_entry(child, regs, info);
> info->seccomp.ret_data = child->ptrace_message;
>
> - /* ret_data is the last field in struct ptrace_syscall_info.seccomp */
> + /*
> + * ret_data is the last non-reserved field
> + * in struct ptrace_syscall_info.seccomp
> + */
> return offsetofend(struct ptrace_syscall_info, seccomp.ret_data);
> }
>
> @@ -1016,6 +1019,118 @@ ptrace_get_syscall_info(struct task_struct *child, unsigned long user_size,
> write_size = min(actual_size, user_size);
> return copy_to_user(datavp, &info, write_size) ? -EFAULT : actual_size;
> }
> +
> +static int
> +ptrace_set_syscall_info_entry(struct task_struct *child, struct pt_regs *regs,
> + struct ptrace_syscall_info *info)
> +{
> + unsigned long args[ARRAY_SIZE(info->entry.args)];
> + int nr = info->entry.nr;
> + int i;
> +
> + /*
> + * Check that the syscall number specified in info->entry.nr
> + * is either a value of type "int" or a sign-extended value
> + * of type "int".
> + */
> + if (nr != info->entry.nr)
> + return -ERANGE;
> +
> + for (i = 0; i < ARRAY_SIZE(args); i++) {
> + args[i] = info->entry.args[i];
> + /*
> + * Check that the syscall argument specified in
> + * info->entry.args[i] is either a value of type
> + * "unsigned long" or a sign-extended value of type "long".
> + */
> + if (args[i] != info->entry.args[i])
> + return -ERANGE;
> + }
> +
> + syscall_set_nr(child, regs, nr);
> + /*
> + * If the syscall number is set to -1, setting syscall arguments is not
> + * just pointless, it would also clobber the syscall return value on
> + * those architectures that share the same register both for the first
> + * argument of syscall and its return value.
> + */
> + if (nr != -1)
> + syscall_set_arguments(child, regs, args);
> +
> + return 0;
> +}
> +
> +static int
> +ptrace_set_syscall_info_seccomp(struct task_struct *child, struct pt_regs *regs,
> + struct ptrace_syscall_info *info)
> +{
> + /*
> + * info->entry is currently a subset of info->seccomp,
> + * info->seccomp.ret_data is currently ignored.
> + */
> + return ptrace_set_syscall_info_entry(child, regs, info);
> +}
> +
> +static int
> +ptrace_set_syscall_info_exit(struct task_struct *child, struct pt_regs *regs,
> + struct ptrace_syscall_info *info)
> +{
> + long rval = info->exit.rval;
> +
> + /*
> + * Check that the return value specified in info->exit.rval
> + * is either a value of type "long" or a sign-extended value
> + * of type "long".
> + */
> + if (rval != info->exit.rval)
> + return -ERANGE;
> +
> + if (info->exit.is_error)
> + syscall_set_return_value(child, regs, rval, 0);
> + else
> + syscall_set_return_value(child, regs, 0, rval);
> +
> + return 0;
> +}
> +
> +static int
> +ptrace_set_syscall_info(struct task_struct *child, unsigned long user_size,
> + const void __user *datavp)
> +{
> + struct pt_regs *regs = task_pt_regs(child);
> + struct ptrace_syscall_info info;
> +
> + if (user_size < sizeof(info))
> + return -EINVAL;
> +
> + /*
> + * The compatibility is tracked by info.op and info.flags: if user-space
> + * does not instruct us to use unknown extra bits from future versions
> + * of ptrace_syscall_info, we are not going to read them either.
> + */
> + if (copy_from_user(&info, datavp, sizeof(info)))
> + return -EFAULT;
> +
> + /* Reserved for future use. */
> + if (info.flags || info.reserved || info.seccomp.reserved2)
> + return -EINVAL;
> +
> + /* Changing the type of the system call stop is not supported yet. */
> + if (ptrace_get_syscall_info_op(child) != info.op)
> + return -EINVAL;
> +
> + switch (info.op) {
> + case PTRACE_SYSCALL_INFO_ENTRY:
> + return ptrace_set_syscall_info_entry(child, regs, &info);
> + case PTRACE_SYSCALL_INFO_EXIT:
> + return ptrace_set_syscall_info_exit(child, regs, &info);
> + case PTRACE_SYSCALL_INFO_SECCOMP:
> + return ptrace_set_syscall_info_seccomp(child, regs, &info);
> + default:
> + /* Other types of system call stops are not supported yet. */
> + return -EINVAL;
> + }
> +}
> #endif /* CONFIG_HAVE_ARCH_TRACEHOOK */
>
> int ptrace_request(struct task_struct *child, long request,
> @@ -1234,6 +1349,10 @@ int ptrace_request(struct task_struct *child, long request,
> case PTRACE_GET_SYSCALL_INFO:
> ret = ptrace_get_syscall_info(child, addr, datavp);
> break;
> +
> + case PTRACE_SET_SYSCALL_INFO:
> + ret = ptrace_set_syscall_info(child, addr, datavp);
> + break;
> #endif
>
> case PTRACE_SECCOMP_GET_FILTER:
^ permalink raw reply
* Re: [PATCH v2] seccomp: passthrough uretprobe systemcall without filtering
From: Oleg Nesterov @ 2025-01-28 15:54 UTC (permalink / raw)
To: Eyal Birger
Cc: kees, luto, wad, mhiramat, andrii, jolsa, alexei.starovoitov,
olsajiri, cyphar, songliubraving, yhs, john.fastabend, peterz,
tglx, bp, daniel, ast, andrii.nakryiko, rostedt, rafi,
shmulik.ladkani, bpf, linux-api, linux-trace-kernel, x86,
linux-kernel, stable
In-Reply-To: <20250128154424.GB24845@redhat.com>
On 01/28, Oleg Nesterov wrote:
>
> can't review, I know nothing about seccomp_cache, but
>
> On 01/28, Eyal Birger wrote:
> >
> > +static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
> > + struct seccomp_data *sd)
> > +{
> > +#ifdef __NR_uretprobe
> > + if (sd->nr == __NR_uretprobe
> > +#ifdef SECCOMP_ARCH_COMPAT
> > + && sd->arch != SECCOMP_ARCH_COMPAT
> > +#endif
>
> it seems you can check
>
> && sd->arch == SECCOMP_ARCH_NATIVE
>
> and avoid #ifdef SECCOMP_ARCH_COMPAT
Although perhaps you added this ifdef to avoid the unnecessary
sd->arch check if !CONFIG_COMPAT ...
Oleg.
^ permalink raw reply
* Re: [PATCH v3 5/6] ptrace: introduce PTRACE_SET_SYSCALL_INFO request
From: Dmitry V. Levin @ 2025-01-28 15:58 UTC (permalink / raw)
To: Christophe Leroy
Cc: Oleg Nesterov, Alexey Gladkov, Charlie Jenkins,
Eugene Syromyatnikov, Mike Frysinger, Renzo Davoli,
Davide Berardi, strace-devel, linux-kernel, linux-api
In-Reply-To: <3ee13371-0c7c-4264-b561-eceb4a7d7976@csgroup.eu>
On Tue, Jan 28, 2025 at 04:21:13PM +0100, Christophe Leroy wrote:
> Le 28/01/2025 à 10:16, Dmitry V. Levin a écrit :
> > PTRACE_SET_SYSCALL_INFO is a generic ptrace API that complements
> > PTRACE_GET_SYSCALL_INFO by letting the ptracer modify details of
> > system calls the tracee is blocked in.
> >
> > This API allows ptracers to obtain and modify system call details
> > in a straightforward and architecture-agnostic way.
> >
> > Current implementation supports changing only those bits of system call
> > information that are used by strace, namely, syscall number, syscall
> > arguments, and syscall return value.
>
> How do you handle changes related to syscalls that call
> force_successful_syscall_return() ?
I don't see why these syscalls would need any special treatment.
If a tracer wants to set an error status for a syscall that cannot return
an error, it's up to the tracer to face the consequences.
Tracers can do it now via PTRACE_SETREGS* anyway.
--
ldv
^ permalink raw reply
* Re: [PATCH v2] seccomp: passthrough uretprobe systemcall without filtering
From: Kees Cook @ 2025-01-29 1:41 UTC (permalink / raw)
To: Eyal Birger
Cc: luto, wad, oleg, mhiramat, andrii, jolsa, alexei.starovoitov,
olsajiri, cyphar, songliubraving, yhs, john.fastabend, peterz,
tglx, bp, daniel, ast, andrii.nakryiko, rostedt, rafi,
shmulik.ladkani, bpf, linux-api, linux-trace-kernel, x86,
linux-kernel, stable
In-Reply-To: <20250128145806.1849977-1-eyal.birger@gmail.com>
On Tue, Jan 28, 2025 at 06:58:06AM -0800, Eyal Birger wrote:
> Note: uretprobe isn't supported in i386 and __NR_ia32_rt_tgsigqueueinfo
> uses the same number as __NR_uretprobe so the syscall isn't forced in the
> compat bitmap.
So a 64-bit tracer cannot use uretprobe on a 32-bit process? Also is
uretprobe strictly an x86_64 feature?
> [...]
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 385d48293a5f..23b594a68bc0 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -734,13 +734,13 @@ seccomp_prepare_user_filter(const char __user *user_filter)
>
> #ifdef SECCOMP_ARCH_NATIVE
> /**
> - * seccomp_is_const_allow - check if filter is constant allow with given data
> + * seccomp_is_filter_const_allow - check if filter is constant allow with given data
> * @fprog: The BPF programs
> * @sd: The seccomp data to check against, only syscall number and arch
> * number are considered constant.
> */
> -static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
> - struct seccomp_data *sd)
> +static bool seccomp_is_filter_const_allow(struct sock_fprog_kern *fprog,
> + struct seccomp_data *sd)
> {
> unsigned int reg_value = 0;
> unsigned int pc;
> @@ -812,6 +812,21 @@ static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
> return false;
> }
>
> +static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
> + struct seccomp_data *sd)
> +{
> +#ifdef __NR_uretprobe
> + if (sd->nr == __NR_uretprobe
> +#ifdef SECCOMP_ARCH_COMPAT
> + && sd->arch != SECCOMP_ARCH_COMPAT
> +#endif
I don't like this because it's not future-proof enough. __NR_uretprobe
may collide with other syscalls at some point. And if __NR_uretprobe_32
is ever implemented, the seccomp logic will be missing. I think this
will work now and in the future:
#ifdef __NR_uretprobe
# ifdef SECCOMP_ARCH_COMPAT
if (sd->arch == SECCOMP_ARCH_COMPAT) {
# ifdef __NR_uretprobe_32
if (sd->nr == __NR_uretprobe_32)
return true;
# endif
} else
# endif
if (sd->nr == __NR_uretprobe)
return true;
#endif
Instead of doing a function rename dance, I think you can just stick
the above into seccomp_is_const_allow() after the WARN().
Also please add a KUnit tests to cover this in
tools/testing/selftests/seccomp/seccomp_bpf.c
With at least these cases combinations below. Check each of:
- not using uretprobe passes
- using uretprobe passes (and validates that uretprobe did work)
in each of the following conditions:
- default-allow filter
- default-block filter
- filter explicitly blocking __NR_uretprobe and nothing else
- filter explicitly allowing __NR_uretprobe (and only other
required syscalls)
Hm, is uretprobe expected to work on mips? Because if so, you'll need to
do something similar to the mode1 checking in the !SECCOMP_ARCH_NATIVE
version of seccomp_cache_check_allow().
(You can see why I really dislike having policy baked into seccomp!)
> + )
> + return true;
> +#endif
> +
> + return seccomp_is_filter_const_allow(fprog, sd);
> +}
> +
> static void seccomp_cache_prepare_bitmap(struct seccomp_filter *sfilter,
> void *bitmap, const void *bitmap_prev,
> size_t bitmap_size, int arch)
> @@ -1023,6 +1038,9 @@ static inline void seccomp_log(unsigned long syscall, long signr, u32 action,
> */
> static const int mode1_syscalls[] = {
> __NR_seccomp_read, __NR_seccomp_write, __NR_seccomp_exit, __NR_seccomp_sigreturn,
> +#ifdef __NR_uretprobe
> + __NR_uretprobe,
> +#endif
It'd be nice to update mode1_syscalls_32 with __NR_uretprobe_32 even
though it doesn't exist. (Is it _never_ planned to be implemented?) But
then, maybe the chances of a compat mode1 seccomp process running under
uretprobe is vanishingly small.
> -1, /* negative terminated */
> };
>
> --
> 2.43.0
>
-Kees
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Andrey Albershteyn @ 2025-01-29 13:01 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-m68k, tglx, jcmvbkbc, linux-security-module, arnd,
linux-fsdevel, chris, npiggin, linuxppc-dev, mpe, luto, jack,
monstr, linux-arch, mingo, linux-alpha, christophe.leroy,
linux-sh, linux-parisc, naveen, bp, hpa, sparclinux, linux-kernel,
x86, maddy, dave.hansen, viro, linux-s390, linux-api, linux-xfs
In-Reply-To: <20250124-wasser-kopfsache-3dc12cb7f7ab@brauner>
On 2025-01-24 10:33:54, Christian Brauner wrote:
> On Wed, Jan 22, 2025 at 03:18:34PM +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
>
> By that you mean that you can use absolute or relative paths instead of
> file descriptors?
yes
>
> > 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.
>
> I'm not following this argument currently. In what sense does opening
> special files return a VFS special inode and how does that prevent
> FS_IOC_FSSEETXATTR from working? The inode in
>
> static int ioctl_fssetxattr(struct file *file, void __user *argp)
> {
> struct mnt_idmap *idmap = file_mnt_idmap(file);
> struct dentry *dentry = file->f_path.dentry;
>
> d_inode(dentry)
>
>
> and your:
>
> error = user_path_at(dfd, filename, lookup_flags, &filepath);
> if (error)
> goto out;
>
> d_inode(filepath.dentry)
>
> is the same.
This is probably not a good description. With these special files,
like pipe, we can not just open them to call FS_IOC_FSSETXATTR, as
with regular ones. As you asked above, with filepath it can be done
without opening files in userspace. I don't see how we can open()
special files and then call ioctl() on them and with paths we can do
it pretty easily.
>
> >
> > 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
> > CC: linux-fsdevel@vger.kernel.org
> > CC: linux-xfs@vger.kernel.org
> > Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
> > ---
> > v1:
> > https://lore.kernel.org/linuxppc-dev/20250109174540.893098-1-aalbersh@kernel.org/
> >
> > 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/arm/tools/syscall.tbl | 2 +
> > arch/arm64/tools/syscall_32.tbl | 2 +
> > arch/m68k/kernel/syscalls/syscall.tbl | 2 +
> > arch/microblaze/kernel/syscalls/syscall.tbl | 2 +
> > arch/mips/kernel/syscalls/syscall_n32.tbl | 2 +
> > arch/mips/kernel/syscalls/syscall_n64.tbl | 2 +
> > arch/mips/kernel/syscalls/syscall_o32.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 | 99 +++++++++++++++++++++++++++++
> > fs/ioctl.c | 16 ++++-
> > include/linux/fileattr.h | 1 +
> > include/linux/syscalls.h | 4 ++
> > include/uapi/asm-generic/unistd.h | 8 ++-
> > 21 files changed, 157 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl
> > index c59d53d6d3f3490f976ca179ddfe02e69265ae4d..4b9e687494c16b60c6fd6ca1dc4d6564706a7e25 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/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
> > index 49eeb2ad8dbd8e074c6240417693f23fb328afa8..66466257f3c2debb3e2299f0b608c6740c98cab2 100644
> > --- a/arch/arm/tools/syscall.tbl
> > +++ b/arch/arm/tools/syscall.tbl
> > @@ -481,3 +481,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/arm64/tools/syscall_32.tbl b/arch/arm64/tools/syscall_32.tbl
> > index 69a829912a05eb8a3e21ed701d1030e31c0148bc..9c516118b154811d8d11d5696f32817430320dbf 100644
> > --- a/arch/arm64/tools/syscall_32.tbl
> > +++ b/arch/arm64/tools/syscall_32.tbl
> > @@ -478,3 +478,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/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl
> > index f5ed71f1910d09769c845c2d062d99ee0449437c..159476387f394a92ee5e29db89b118c630372db2 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 680f568b77f2cbefc3eacb2517f276041f229b1e..a6d59ee740b58cacf823702003cf9bad17c0d3b7 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/mips/kernel/syscalls/syscall_n32.tbl b/arch/mips/kernel/syscalls/syscall_n32.tbl
> > index 0b9b7e25b69ad592642f8533bee9ccfe95ce9626..cfe38fcebe1a0279e11751378d3e71c5ec6b6569 100644
> > --- a/arch/mips/kernel/syscalls/syscall_n32.tbl
> > +++ b/arch/mips/kernel/syscalls/syscall_n32.tbl
> > @@ -405,3 +405,5 @@
> > 464 n32 getxattrat sys_getxattrat
> > 465 n32 listxattrat sys_listxattrat
> > 466 n32 removexattrat sys_removexattrat
> > +467 n32 getfsxattrat sys_getfsxattrat
> > +468 n32 setfsxattrat sys_setfsxattrat
> > diff --git a/arch/mips/kernel/syscalls/syscall_n64.tbl b/arch/mips/kernel/syscalls/syscall_n64.tbl
> > index c844cd5cda620b2809a397cdd6f4315ab6a1bfe2..29a0c5974d1aa2f01e33edc0252d75fb97abe230 100644
> > --- a/arch/mips/kernel/syscalls/syscall_n64.tbl
> > +++ b/arch/mips/kernel/syscalls/syscall_n64.tbl
> > @@ -381,3 +381,5 @@
> > 464 n64 getxattrat sys_getxattrat
> > 465 n64 listxattrat sys_listxattrat
> > 466 n64 removexattrat sys_removexattrat
> > +467 n64 getfsxattrat sys_getfsxattrat
> > +468 n64 setfsxattrat sys_setfsxattrat
> > diff --git a/arch/mips/kernel/syscalls/syscall_o32.tbl b/arch/mips/kernel/syscalls/syscall_o32.tbl
> > index 349b8aad1159f404103bd2057a1e64e9bf309f18..6c00436807c57c492ba957fcd59af1202231cf80 100644
> > --- a/arch/mips/kernel/syscalls/syscall_o32.tbl
> > +++ b/arch/mips/kernel/syscalls/syscall_o32.tbl
> > @@ -454,3 +454,5 @@
> > 464 o32 getxattrat sys_getxattrat
> > 465 o32 listxattrat sys_listxattrat
> > 466 o32 removexattrat sys_removexattrat
> > +467 o32 getfsxattrat sys_getfsxattrat
> > +468 o32 setfsxattrat sys_setfsxattrat
> > diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl
> > index d9fc94c869657fcfbd7aca1d5f5abc9fae2fb9d8..b3578fac43d6b65167787fcc97d2d09f5a9828e7 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 d8b4ab78bef076bd50d49b87dea5060fd8c1686a..808045d82c9465c3bfa96b15947546efe5851e9a 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 e9115b4d8b635b846e5c9ad6ce229605323723a5..78dfc2c184d4815baf8a9e61c546c9936d58a47c 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 c8cad33bf250ea110de37bd1407f5a43ec5e38f2..d5a5c8339f0ed25ea07c4aba90351d352033c8a0 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 727f99d333b304b3db0711953a3d91ece18a28eb..817dcd8603bcbffc47f3f59aa3b74b16486453d0 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 4d0fb2fba7e208ae9455459afe11e277321d9f74..b4842c027c5d00c0236b2ba89387c5e2267447bd 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 5eb708bff1c791debd6cfc5322583b2ae53f6437..b6f0a7236aaee624cf9b484239a1068085a8ffe1 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 37effc1b134eea061f2c350c1d68b4436b65a4dd..425d56be337d1de22f205ac503df61ff86224fee 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 6b4c77268fc0ecace4ac78a9ca777fbffc277f4a..cdecb793b2ab5ab01e2333da4382919b94c7f65f 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,99 @@ 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 __user *, fsx, unsigned int, at_flags)
> > +{
> > + struct fd dir;
> > + struct fileattr fa;
> > + struct path filepath;
> > + int error;
> > + unsigned int lookup_flags = 0;
> > +
> > + if ((at_flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
> > + return -EINVAL;
> > +
> > + if (at_flags & AT_SYMLINK_FOLLOW)
> > + lookup_flags |= LOOKUP_FOLLOW;
> > +
> > + if (at_flags & AT_EMPTY_PATH)
> > + lookup_flags |= LOOKUP_EMPTY;
> > +
> > + dir = fdget(dfd);
> > + if (!fd_file(dir))
> > + return -EBADF;
>
> Please rely on scope-based cleanup:
>
> CLASS(fd, f)(dfd);
> if (fd_empty(f))
> return -EBADF;
aha, thanks, didn't know about this
>
> > +
> > + if (!S_ISDIR(file_inode(fd_file(dir))->i_mode)) {
>
> This isn't needed as user_path_at() will return ENOTDIR.
> Your path as is wuld also preclude AT_EMPTY_PATH with directory file
> descriptors and afaict there's various filesystems that seem to support
> this on directory inodes.
I see, thanks, I will remove that
>
> > + error = -EBADF;
> > + goto out;
> > + }
> > +
> > + error = user_path_at(dfd, filename, lookup_flags, &filepath);
> > + if (error)
> > + goto out;
>
> I'm confused. You're using fdget() above but then you don't use the
> resulting file and call user_path_at() instead? Don't bother with
> fdget() at all and just call into user_path_at() directly.
sure, doesn't needed anymore with your suggestions above
>
> > +
> > + 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 __user *, fsx, unsigned int, at_flags)
> > +{
> > + struct fd dir;
> > + struct fileattr fa;
> > + struct path filepath;
> > + int error;
> > + unsigned int lookup_flags = 0;
> > +
> > + if ((at_flags & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH)) != 0)
> > + return -EINVAL;
> > +
> > + if (at_flags & AT_SYMLINK_FOLLOW)
> > + lookup_flags |= LOOKUP_FOLLOW;
> > +
> > + if (at_flags & AT_EMPTY_PATH)
> > + lookup_flags |= LOOKUP_EMPTY;
> > +
> > + 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, lookup_flags, &filepath);
> > + if (error)
> > + goto out;
>
> Same problem as above. fdget() stuff isn't needed if you're calling
> user_path_at() anyway.
>
> > +
> > + 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);
>
> Just use the pattern:
>
> error = user_path_at(dfd, filename, lookup_flags, &filepath);
> if (error)
> return error; /* once you've removed the fdget() direct return works fine */
>
>
> error = mnt_want_write(filepath.mnt);
> if (!error) {
> error = vfs_fileattr_set(file_mnt_idmap(fd_file(dir)), filepath.dentry, &fa);
> mnt_drop_write(filepath.mnt);
> }
>
> return error;
sure, I will changed it as suggested
--
- Andrey
>
> > +
> > +out_path:
> > + path_put(&filepath);
> > +out:
> > + fdput(dir);
> > + return error;
> > +}
> > diff --git a/fs/ioctl.c b/fs/ioctl.c
> > index 638a36be31c14afc66a7fd6eb237d9545e8ad997..dc160c2ef145e4931d625f1f93c2a8ae7f87abf3 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;
> >
> > @@ -646,6 +645,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 47c05a9851d0600964b644c9c7218faacfd865f8..8598e94b530b8b280a2697eaf918dd60f573d6ee 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 c6333204d45130eb022f6db460eea34a1f6e91db..3134d463d9af64c6e78adb37bff4b91f77b5305f 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, unsigned int at_flags);
> > +asmlinkage long sys_setfsxattrat(int dfd, const char __user *filename,
> > + struct fsxattr *fsx, unsigned 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 88dc393c2bca38c0fa1b3fae579f7cfe4931223c..50be2e1007bc2779120d05c6e9512a689f86779c 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
> >
> > ---
> > base-commit: 4c538044ee2d11299cc57ac1e92d343e1e83b847
> > change-id: 20250114-xattrat-syscall-6a1136d2db59
> >
> > Best regards,
> > --
> > Andrey Albershteyn <aalbersh@kernel.org>
> >
>
^ permalink raw reply
* Re: [PATCH v2] seccomp: passthrough uretprobe systemcall without filtering
From: Eyal Birger @ 2025-01-29 17:27 UTC (permalink / raw)
To: Kees Cook
Cc: luto, wad, oleg, mhiramat, andrii, jolsa, alexei.starovoitov,
olsajiri, cyphar, songliubraving, yhs, john.fastabend, peterz,
tglx, bp, daniel, ast, andrii.nakryiko, rostedt, rafi,
shmulik.ladkani, bpf, linux-api, linux-trace-kernel, x86,
linux-kernel, stable
In-Reply-To: <202501281634.7F398CEA87@keescook>
Hi,
Thanks for the review!
On Tue, Jan 28, 2025 at 5:41 PM Kees Cook <kees@kernel.org> wrote:
>
> On Tue, Jan 28, 2025 at 06:58:06AM -0800, Eyal Birger wrote:
> > Note: uretprobe isn't supported in i386 and __NR_ia32_rt_tgsigqueueinfo
> > uses the same number as __NR_uretprobe so the syscall isn't forced in the
> > compat bitmap.
>
> So a 64-bit tracer cannot use uretprobe on a 32-bit process? Also is
> uretprobe strictly an x86_64 feature?
>
My understanding is that they'd be able to do so, but use the int3 trap
instead of the uretprobe syscall.
> > [...]
> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > index 385d48293a5f..23b594a68bc0 100644
> > --- a/kernel/seccomp.c
> > +++ b/kernel/seccomp.c
> > @@ -734,13 +734,13 @@ seccomp_prepare_user_filter(const char __user *user_filter)
> >
> > #ifdef SECCOMP_ARCH_NATIVE
> > /**
> > - * seccomp_is_const_allow - check if filter is constant allow with given data
> > + * seccomp_is_filter_const_allow - check if filter is constant allow with given data
> > * @fprog: The BPF programs
> > * @sd: The seccomp data to check against, only syscall number and arch
> > * number are considered constant.
> > */
> > -static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
> > - struct seccomp_data *sd)
> > +static bool seccomp_is_filter_const_allow(struct sock_fprog_kern *fprog,
> > + struct seccomp_data *sd)
> > {
> > unsigned int reg_value = 0;
> > unsigned int pc;
> > @@ -812,6 +812,21 @@ static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
> > return false;
> > }
> >
> > +static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
> > + struct seccomp_data *sd)
> > +{
> > +#ifdef __NR_uretprobe
> > + if (sd->nr == __NR_uretprobe
> > +#ifdef SECCOMP_ARCH_COMPAT
> > + && sd->arch != SECCOMP_ARCH_COMPAT
> > +#endif
>
> I don't like this because it's not future-proof enough. __NR_uretprobe
> may collide with other syscalls at some point.
I'm not sure I got this point.
> And if __NR_uretprobe_32
> is ever implemented, the seccomp logic will be missing. I think this
> will work now and in the future:
>
> #ifdef __NR_uretprobe
> # ifdef SECCOMP_ARCH_COMPAT
> if (sd->arch == SECCOMP_ARCH_COMPAT) {
> # ifdef __NR_uretprobe_32
> if (sd->nr == __NR_uretprobe_32)
> return true;
> # endif
> } else
> # endif
> if (sd->nr == __NR_uretprobe)
> return true;
> #endif
I don't know if implementing uretprobe syscall for compat binaries is
planned or makes sense - I'd appreciate Jiri's and others opinion on that.
That said, I don't mind adding this code for the sake of future proofing.
>
> Instead of doing a function rename dance, I think you can just stick
> the above into seccomp_is_const_allow() after the WARN().
My motivation for the renaming dance was that you mentioned we might add
new syscalls to this as well, so I wanted to avoid cluttering the existing
function which seems to be well defined.
>
> Also please add a KUnit tests to cover this in
> tools/testing/selftests/seccomp/seccomp_bpf.c
I think this would mean that this test suite would need to run as
privileged. Is that Ok? or maybe it'd be better to have a new suite?
> With at least these cases combinations below. Check each of:
>
> - not using uretprobe passes
> - using uretprobe passes (and validates that uretprobe did work)
>
> in each of the following conditions:
>
> - default-allow filter
> - default-block filter
> - filter explicitly blocking __NR_uretprobe and nothing else
> - filter explicitly allowing __NR_uretprobe (and only other
> required syscalls)
Ok.
>
> Hm, is uretprobe expected to work on mips? Because if so, you'll need to
> do something similar to the mode1 checking in the !SECCOMP_ARCH_NATIVE
> version of seccomp_cache_check_allow().
I don't know if uretprobe syscall is expected to run on mips. Personally
I'd avoid adding this dead code.
>
> (You can see why I really dislike having policy baked into seccomp!)
I definitely understand :)
>
> > + )
> > + return true;
> > +#endif
> > +
> > + return seccomp_is_filter_const_allow(fprog, sd);
> > +}
> > +
> > static void seccomp_cache_prepare_bitmap(struct seccomp_filter *sfilter,
> > void *bitmap, const void *bitmap_prev,
> > size_t bitmap_size, int arch)
> > @@ -1023,6 +1038,9 @@ static inline void seccomp_log(unsigned long syscall, long signr, u32 action,
> > */
> > static const int mode1_syscalls[] = {
> > __NR_seccomp_read, __NR_seccomp_write, __NR_seccomp_exit, __NR_seccomp_sigreturn,
> > +#ifdef __NR_uretprobe
> > + __NR_uretprobe,
> > +#endif
>
> It'd be nice to update mode1_syscalls_32 with __NR_uretprobe_32 even
> though it doesn't exist. (Is it _never_ planned to be implemented?) But
> then, maybe the chances of a compat mode1 seccomp process running under
> uretprobe is vanishingly small.
It seems to me very unlikely. BTW, when I tested the "strict" mode change
my program was killed by seccomp. The reason wasn't the uretprobe syscall
(which I added to the list), it was actually the exit_group syscall which
libc uses instead of the exit syscall.
Thanks again,
Eyal.
^ 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