* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Kees Cook @ 2025-01-18 20:21 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: <20250117005539.325887-1-eyal.birger@gmail.com>
On Thu, Jan 16, 2025 at 04:55:39PM -0800, Eyal Birger wrote:
> 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.
How is this any different from sigreturn, rt_sigreturn, or
restart_syscall? These are all handled explicitly by userspace filters
already, and I don't see why uretprobe should be any different. Docker
has had plenty of experience with fixing their seccomp filters for new
syscalls. For example, many times already a given libc will suddenly
start using a new syscall when it sees its available, etc.
Basically, this is a Docker issue, not a kernel issue. Seccomp is
behaving correctly. I don't want to start making syscalls invisible
without an extremely good reason. If _anything_ should be invisible, it
is restart_syscall (which actually IS invisible under certain
architectures).
-Kees
--
Kees Cook
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Darrick J. Wong @ 2025-01-18 20:31 UTC (permalink / raw)
To: Kees Cook
Cc: Eyal Birger, 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: <202501181212.4C515DA02@keescook>
On Sat, Jan 18, 2025 at 12:21:51PM -0800, Kees Cook wrote:
> On Thu, Jan 16, 2025 at 04:55:39PM -0800, Eyal Birger wrote:
> > 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.
>
> How is this any different from sigreturn, rt_sigreturn, or
> restart_syscall? These are all handled explicitly by userspace filters
> already, and I don't see why uretprobe should be any different. Docker
> has had plenty of experience with fixing their seccomp filters for new
> syscalls. For example, many times already a given libc will suddenly
> start using a new syscall when it sees its available, etc.
>
> Basically, this is a Docker issue, not a kernel issue. Seccomp is
> behaving correctly. I don't want to start making syscalls invisible
> without an extremely good reason. If _anything_ should be invisible, it
> is restart_syscall (which actually IS invisible under certain
> architectures).
I was wondering that too -- if ______'s security policy is to disallow
by default, then fix the security policy. Don't blow a hole in seccomp
for all users. Maybe someone *wants* to block uretprobe. Maybe doing
so will be needed some day as a crude mitigation for a zeroday.
--D
> -Kees
>
> --
> Kees Cook
>
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Eyal Birger @ 2025-01-18 20:45 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: <202501181212.4C515DA02@keescook>
Hןת
On Sat, Jan 18, 2025 at 12:21 PM Kees Cook <kees@kernel.org> wrote:
>
> On Thu, Jan 16, 2025 at 04:55:39PM -0800, Eyal Birger wrote:
> > 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.
>
> How is this any different from sigreturn, rt_sigreturn, or
> restart_syscall? These are all handled explicitly by userspace filters
> already, and I don't see why uretprobe should be any different. Docker
> has had plenty of experience with fixing their seccomp filters for new
> syscalls. For example, many times already a given libc will suddenly
> start using a new syscall when it sees its available, etc.
I think the difference is that this syscall is not part of the process's
code - it is inserted there by another process tracing it.
So this is different than desiring to deploy a new version of a binary
that uses a new libc or a new syscall. Here the case is that there are
three players - the tracer running out of docker, the tracee running in docker,
and docker itself. All three were running fine in a specific kernel version,
but upgrading the kernel now crashes the traced process.
>
> Basically, this is a Docker issue, not a kernel issue.
As mentione above, for all three given binaries, nothing changed - only the
kernel version.
> Seccomp is behaving correctly. I don't want to start making syscalls invisible
> without an extremely good reason. If _anything_ should be invisible, it
> is restart_syscall (which actually IS invisible under certain
> architectures).
I think this syscall is different in that respect for the reasons described.
I don't know if seccomp is behaving correctly when it blocks a kernel
implementation detail that isn't user created. IMHO the fact that this
implementation detail is implemented as a syscall is unfortunate, and I'm
trying to mitigate the result.
Eyal.
>
> -Kees
>
> --
> Kees Cook
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Kees Cook @ 2025-01-19 2:24 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: <CAHsH6GuifA9nUzNR-eW5ZaXyhzebJOCjBSpfZCksoiyCuG=yYw@mail.gmail.com>
On January 18, 2025 12:45:47 PM PST, Eyal Birger <eyal.birger@gmail.com> wrote:
>I think the difference is that this syscall is not part of the process's
>code - it is inserted there by another process tracing it.
Well that's nothing like syscall_restart, and now I'm convinced seccomp must never ignore uretprobe -- a process might want to block uretprobe!
So, no, sorry, this needs to be handled by the seccomp policy that is applied to the process.
>So this is different than desiring to deploy a new version of a binary
>that uses a new libc or a new syscall.
Uh, no, the case I used as an example was no changes to anything except the kernel. Libc noticed the available syscall, uses it, and is instantly killed by the Docker seccomp policy which didn't know about that syscall.
> Here the case is that there are
>three players - the tracer running out of docker, the tracee running in docker,
>and docker itself. All three were running fine in a specific kernel version,
>but upgrading the kernel now crashes the traced process.
If uretprobe used to work without a syscall, then that seems to be the problem. But I think easiest is just fixing the Docker policy. (Which is a text file configuration change; no new binaries, no rebuilds!).
>I think this syscall is different in that respect for the reasons described.
I don't agree, sorry. Seccomp has a really singular and specific purpose, which is explicitly *externalizing* policy. I do not want to have policy within seccomp itself.
>I don't know if seccomp is behaving correctly when it blocks a kernel
>implementation detail that isn't user created.
But it is user created? Something added a uretprobe to a process who's seccomp policy is not expecting it. This seems precisely by design.
-Kees
--
Kees Cook
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Eyal Birger @ 2025-01-19 3: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: <8B2624AC-E739-4BBE-8725-010C2344F61C@kernel.org>
Hi,
Thank you for the detailed response.
On Sat, Jan 18, 2025 at 6:25 PM Kees Cook <kees@kernel.org> wrote:
>
> On January 18, 2025 12:45:47 PM PST, Eyal Birger <eyal.birger@gmail.com> wrote:
> >I think the difference is that this syscall is not part of the process's
> >code - it is inserted there by another process tracing it.
>
> Well that's nothing like syscall_restart, and now I'm convinced seccomp must never ignore uretprobe -- a process might want to block uretprobe!
>
I think I understand your point. But do you think this is intentional?
i.e. seccomp couldn't have been used to block uretprobes before this
syscall implementation afaict.
> So, no, sorry, this needs to be handled by the seccomp policy that is applied to the process.
>
The problem we're facing is that existing workloads are breaking, and
as mentioned I'm not sure how practical it is to demand replacing a
working docker environment because of a new syscall that was added for
performance reasons.
> >So this is different than desiring to deploy a new version of a binary
> >that uses a new libc or a new syscall.
>
> Uh, no, the case I used as an example was no changes to anything except the kernel. Libc noticed the available syscall, uses it, and is instantly killed by the Docker seccomp policy which didn't know about that syscall.
>
That's an interesting situation and quite unexpected :) I'm glad I didn't
have to face that one in production.
> > Here the case is that there are
> >three players - the tracer running out of docker, the tracee running in docker,
> >and docker itself. All three were running fine in a specific kernel version,
> >but upgrading the kernel now crashes the traced process.
>
> If uretprobe used to work without a syscall, then that seems to be the problem.
I agree.
> But I think easiest is just fixing the Docker policy. (Which is a text file configuration change; no new binaries, no rebuilds!).
As far as I can tell libseccomp needs to provide support for this new
syscall and a new docker version would need to be deployed, so It's not
just a configuration change. Also the default policy which comes packed in
docker would probably need to be changed to avoid having to explicitly
provide a seccomp configuration for each deployment.
>
> >I think this syscall is different in that respect for the reasons described.
>
> I don't agree, sorry. Seccomp has a really singular and specific purpose, which is explicitly *externalizing* policy. I do not want to have policy within seccomp itself.
>
Understood.
> >I don't know if seccomp is behaving correctly when it blocks a kernel
> >implementation detail that isn't user created.
>
> But it is user created? Something added a uretprobe to a process who's seccomp policy is not expecting it. This seems precisely by design.
I think I wasn't accurate in my wording.
The uretprobe syscall is added to the tracee by the kernel.
The tracer itself is merely requesting to attach a uretprobe bpf
function. In previous versions, this was implemented by the kernel
installing an int3 instruction, and in the new implementation the kernel
is installing a uretprobe syscall.
The "user" in this case - the tracer program - didn't deliberately install
the syscall, but anyway this is semantics.
I think I understand your point that it is regarded as "policy", only that
it creates a problem in actual deployments, where in order to be able to
run the tracer software which has been working on newer kernels a new docker
has to be deployed.
I'm trying to find a pragmatic solution to this problem, and I understand
the motivation to avoid policy in seccomp.
Alternatively, maybe this syscall implementation should be reverted?
Thanks again,
Eyal.
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Jiri Olsa @ 2025-01-19 10:44 UTC (permalink / raw)
To: Eyal Birger
Cc: Kees Cook, luto, wad, oleg, ldv, mhiramat, andrii,
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: <CAHsH6GtpXMswVKytv7_JMGca=3wxKRUK4rZmBBxJPRh1WYdObg@mail.gmail.com>
On Sat, Jan 18, 2025 at 07:39:25PM -0800, Eyal Birger wrote:
> Hi,
>
> Thank you for the detailed response.
>
> On Sat, Jan 18, 2025 at 6:25 PM Kees Cook <kees@kernel.org> wrote:
>
> >
> > On January 18, 2025 12:45:47 PM PST, Eyal Birger <eyal.birger@gmail.com> wrote:
> > >I think the difference is that this syscall is not part of the process's
> > >code - it is inserted there by another process tracing it.
> >
> > Well that's nothing like syscall_restart, and now I'm convinced seccomp must never ignore uretprobe -- a process might want to block uretprobe!
> >
>
> I think I understand your point. But do you think this is intentional?
> i.e. seccomp couldn't have been used to block uretprobes before this
> syscall implementation afaict.
>
> > So, no, sorry, this needs to be handled by the seccomp policy that is applied to the process.
> >
>
> The problem we're facing is that existing workloads are breaking, and
> as mentioned I'm not sure how practical it is to demand replacing a
> working docker environment because of a new syscall that was added for
> performance reasons.
>
> > >So this is different than desiring to deploy a new version of a binary
> > >that uses a new libc or a new syscall.
> >
> > Uh, no, the case I used as an example was no changes to anything except the kernel. Libc noticed the available syscall, uses it, and is instantly killed by the Docker seccomp policy which didn't know about that syscall.
> >
>
> That's an interesting situation and quite unexpected :) I'm glad I didn't
> have to face that one in production.
>
> > > Here the case is that there are
> > >three players - the tracer running out of docker, the tracee running in docker,
> > >and docker itself. All three were running fine in a specific kernel version,
> > >but upgrading the kernel now crashes the traced process.
> >
> > If uretprobe used to work without a syscall, then that seems to be the problem.
>
> I agree.
>
> > But I think easiest is just fixing the Docker policy. (Which is a text file configuration change; no new binaries, no rebuilds!).
>
> As far as I can tell libseccomp needs to provide support for this new
> syscall and a new docker version would need to be deployed, so It's not
> just a configuration change. Also the default policy which comes packed in
> docker would probably need to be changed to avoid having to explicitly
> provide a seccomp configuration for each deployment.
>
> >
> > >I think this syscall is different in that respect for the reasons described.
> >
> > I don't agree, sorry. Seccomp has a really singular and specific purpose, which is explicitly *externalizing* policy. I do not want to have policy within seccomp itself.
> >
>
> Understood.
>
> > >I don't know if seccomp is behaving correctly when it blocks a kernel
> > >implementation detail that isn't user created.
> >
> > But it is user created? Something added a uretprobe to a process who's seccomp policy is not expecting it. This seems precisely by design.
>
> I think I wasn't accurate in my wording.
> The uretprobe syscall is added to the tracee by the kernel.
> The tracer itself is merely requesting to attach a uretprobe bpf
> function. In previous versions, this was implemented by the kernel
> installing an int3 instruction, and in the new implementation the kernel
> is installing a uretprobe syscall.
> The "user" in this case - the tracer program - didn't deliberately install
> the syscall, but anyway this is semantics.
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
so at the end the process executes the uretprobe syscall, but it's up to
kernel to decide that and set it up.. but I don't know if that's strong
enough reason for seccomp to ignore the syscall
>
> I think I understand your point that it is regarded as "policy", only that
> it creates a problem in actual deployments, where in order to be able to
> run the tracer software which has been working on newer kernels a new docker
> has to be deployed.
>
> I'm trying to find a pragmatic solution to this problem, and I understand
> the motivation to avoid policy in seccomp.
I could think of sysctl for that.. you complained earlier about weird
semantics for that [1], but I think it's better than to remove it
jirka
>
> Alternatively, maybe this syscall implementation should be reverted?
>
> Thanks again,
> Eyal.
[1] https://lore.kernel.org/bpf/CAHsH6Gs03iJt-ziWt5Bye_DuqCbk3TpMmgPbkYh64XBvpGaDtw@mail.gmail.com/
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Oleg Nesterov @ 2025-01-19 12:40 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: <202501181212.4C515DA02@keescook>
On 01/18, Kees Cook wrote:
>
> On Thu, Jan 16, 2025 at 04:55:39PM -0800, Eyal Birger wrote:
> > 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.
>
> How is this any different from sigreturn, rt_sigreturn, or
> restart_syscall? These are all handled explicitly by userspace filters
> already, and I don't see why uretprobe should be any different.
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...
Oleg.
^ permalink raw reply
* Re: [PATCH v2 6/7] ptrace: introduce PTRACE_SET_SYSCALL_INFO request
From: Dmitry V. Levin @ 2025-01-19 12:44 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Eugene Syromyatnikov, Mike Frysinger, Renzo Davoli,
Davide Berardi, strace-devel, linux-kernel, linux-api
In-Reply-To: <20250118141341.GA21464@redhat.com>
On Sat, Jan 18, 2025 at 03:13:42PM +0100, Oleg Nesterov wrote:
> On 01/17, Dmitry V. Levin wrote:
[...]
> > For example, on x86_64 sizeof(struct ptrace_syscall_info) is currently 88,
> > while on x86 it is 84.
>
> Not good, but too late to complain...
Actually, I don't think it's too late to add an extra __u32 padding
there since it wouldn't affect PTRACE_GET_SYSCALL_INFO.
I can add an explicit padding to the structure if you say
you like it better this way.
[...]
> Thats said... Can't resist,
>
> > An absolutely artificial example: let's say we're adding an optional
> > 64-bit field "artificial" to ptrace_syscall_info.seccomp, this means
> > sizeof(ptrace_syscall_info) grows by 8 bytes. When userspace wants
> > to set this optional field, it sets a bit in ptrace_syscall_info.flags,
> > this tells the kernel to look into this new "artificial" field.
> > When userspace is not interested in setting new optional fields,
> > it just keeps ptrace_syscall_info.flags == 0. Remember, however, that
> > by adding the new optional field sizeof(ptrace_syscall_info) grew by 8 bytes.
> >
> > What we need is to make sure that an older kernel that has no idea of this
> > new field would still accept the bigger size, so that userspace would be
> > able to continue doing its
> > ptrace(PTRACE_SET_SYSCALL_INFO, pid, sizeof(info), &info)
> > despite of potential growth of sizeof(info) until it actually starts using
> > new optional fields.
>
> This is clear, but personally I don't really like this pattern... Consider
>
> void set_syscall_info(int unlikely_condition)
> {
> struct ptrace_syscall_info info;
>
> fill_info(&info);
> if (unlikely_condition) {
> info.flags = USE_ARTIFICIAL;
> info.artificial = 1;
> }
>
> assert(ptrace(PTRACE_SET_SYSCALL_INFO, sizeof(info), &info) == 0);
> }
>
> Now this application (running on the older kernel) can fail or not, depending
> on "unlikely_condition". To me it would be better to always fail in this case.
In practice, user-space programs rarely have the luxury to assume that
some new kernel API is available. For example, strace still performs a
runtime check for PTRACE_GET_SYSCALL_INFO (introduced more than 5 years
ago) and falls back to pre-PTRACE_GET_SYSCALL_INFO interfaces when the
kernel lacks support. Consequently, user-space programs would have to
keep track of PTRACE_SET_SYSCALL_INFO interfaces supported by the kernel,
so ...
> That is why I tried to suggest to use "user_size" as a version number.
> Currently we have PTRACE_SYSCALL_INFO_SIZE_VER0, when we add the new
> "artificial" member we will have PTRACE_SYSCALL_INFO_SIZE_VER1. Granted,
> this way set_syscall_info() can't use sizeof(info), it should do
>
> ptrace(PTRACE_SET_SYSCALL_INFO, PTRACE_SYSCALL_INFO_SIZE_VER1, info);
>
> and the kernel needs more checks, but this is what I had in mind when I said
> that the 1st version can just require "user_size == PTRACE_SYSCALL_INFO_SIZE_VER0".
... it wouldn't be a big deal for user-space to specify also an
appropriate "user_size", e.g. PTRACE_SYSCALL_INFO_SIZE_VER1 when it starts
using the interface available since VER1, but it wouldn't help user-space
programs either as they would have to update "op" and/or "flags" anyway,
and "user_size" would become just yet another detail they have to care
about.
At the same time, "flags" is needed anyway because the most likely
extension of PTRACE_SET_SYSCALL_INFO would be support of setting some
fields that are present in the structure already, e.g.
instruction_pointer.
--
ldv
^ permalink raw reply
* Re: [PATCH v2 6/7] ptrace: introduce PTRACE_SET_SYSCALL_INFO request
From: Aleksa Sarai @ 2025-01-19 14:38 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Dmitry V. Levin, Eugene Syromyatnikov, Mike Frysinger,
Renzo Davoli, Davide Berardi, strace-devel, linux-kernel,
linux-api
In-Reply-To: <20250118141341.GA21464@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 4362 bytes --]
On 2025-01-18, Oleg Nesterov <oleg@redhat.com> wrote:
> On 01/17, Dmitry V. Levin wrote:
> >
>
> (reordered)
>
> > struct ptrace_syscall_info has members of type __u64, and it currently
> > ends with "__u32 ret_data". So depending on the alignment, the structure
> > either has extra 4 trailing padding bytes, or it doesn't.
>
> Ah, I didn't realize that the last member is __u32, so I completely
> misunderstood your "it depends on the alignment of __u64" note.
>
> > For example, on x86_64 sizeof(struct ptrace_syscall_info) is currently 88,
> > while on x86 it is 84.
>
> Not good, but too late to complain...
>
> OK, I see your point now and I won't argue with approach you outlined in your
> previous email
>
> size_t min_size = offsetofend(struct ptrace_syscall_info, seccomp.ret_data);
> size_t copy_size = min(sizeof(info), user_size);
>
> if (copy_size < min_size)
> return -EINVAL;
>
> if (copy_from_user(&info, datavp, copy_size))
> return -EFAULT;
>
> -------------------------------------------------------------------------------
> Thats said... Can't resist,
>
> > An absolutely artificial example: let's say we're adding an optional
> > 64-bit field "artificial" to ptrace_syscall_info.seccomp, this means
> > sizeof(ptrace_syscall_info) grows by 8 bytes. When userspace wants
> > to set this optional field, it sets a bit in ptrace_syscall_info.flags,
> > this tells the kernel to look into this new "artificial" field.
> > When userspace is not interested in setting new optional fields,
> > it just keeps ptrace_syscall_info.flags == 0. Remember, however, that
> > by adding the new optional field sizeof(ptrace_syscall_info) grew by 8 bytes.
> >
> > What we need is to make sure that an older kernel that has no idea of this
> > new field would still accept the bigger size, so that userspace would be
> > able to continue doing its
> > ptrace(PTRACE_SET_SYSCALL_INFO, pid, sizeof(info), &info)
> > despite of potential growth of sizeof(info) until it actually starts using
> > new optional fields.
>
> This is clear, but personally I don't really like this pattern... Consider
>
> void set_syscall_info(int unlikely_condition)
> {
> struct ptrace_syscall_info info;
>
> fill_info(&info);
> if (unlikely_condition) {
> info.flags = USE_ARTIFICIAL;
> info.artificial = 1;
> }
>
> assert(ptrace(PTRACE_SET_SYSCALL_INFO, sizeof(info), &info) == 0);
> }
>
> Now this application (running on the older kernel) can fail or not, depending
> on "unlikely_condition". To me it would be better to always fail in this case.
>
> That is why I tried to suggest to use "user_size" as a version number.
> Currently we have PTRACE_SYSCALL_INFO_SIZE_VER0, when we add the new
> "artificial" member we will have PTRACE_SYSCALL_INFO_SIZE_VER1. Granted,
> this way set_syscall_info() can't use sizeof(info), it should do
user_size *is* a version number, it's just that copy_struct_from_user()
allows programs built with newer headers to run on older kernels (if
they don't use the new features). The alternative is that programs that
build with a newer set of kernel headers will implicitly have a larger
ptrace_syscall_info struct, which will cause them to start failing after
the binary is rebuilt.
*Strictly speaking* this wouldn't be a kernel regression (because it's a
new binary, the old binary would still work), but the risk of these
kinds of APIs being incredibly fragile is the reason why I went with the
check_zeroed_user() approach in copy_struct_from_user().
(I haven't looked at the details of this patchset, this is just a
general comment about copy_struct_from_user() and why this feature is
useful to userspace programs. Not all APIs need the extensibility of
copy_struct_from_user().)
> ptrace(PTRACE_SET_SYSCALL_INFO, PTRACE_SYSCALL_INFO_SIZE_VER1, info);
>
> and the kernel needs more checks, but this is what I had in mind when I said
> that the 1st version can just require "user_size == PTRACE_SYSCALL_INFO_SIZE_VER0".
>
> But I won't insist, I do not pretend I understand the user-space needs.
>
> Thanks!
>
> Oleg.
>
>
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
https://www.cyphar.com/
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Andy Lutomirski @ 2025-01-19 18:36 UTC (permalink / raw)
To: Kees Cook
Cc: Eyal Birger, 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: <8B2624AC-E739-4BBE-8725-010C2344F61C@kernel.org>
On Sat, Jan 18, 2025 at 6:25 PM Kees Cook <kees@kernel.org> wrote:
>
>
>
> On January 18, 2025 12:45:47 PM PST, Eyal Birger <eyal.birger@gmail.com> wrote:
> >I think the difference is that this syscall is not part of the process's
> >code - it is inserted there by another process tracing it.
>
> Well that's nothing like syscall_restart, and now I'm convinced seccomp must never ignore uretprobe -- a process might want to block uretprobe!
>
I've been contemplating this. uretprobe is a very odd syscall: it's a
syscall that emulates a breakpoint. So, before uretprobe-the-syscall
was added, the process would breakpoint via a non-syscall vector, and
the tracing code would do its thing, and seccomp would be none the
wiser.
There's a distinction between different types of operations that
seccomp is entirely unaware of right now: is the task trying to:
a) do something *to itself*
b) do something that doesn't have meaningful side effects on the rest
of the world, at least in a non-buggy kernel, but where the process is
actually trying to restrict its own actions
c) interacting with something outside the process, that has privilege
over the process, where the interaction in question should absolutely
be subject to security policy, but that security policy really ought
to apply to the outside process.
uretprobe is very much in category c, and it's kind of unique in this
sense *as a syscall*. But there are plenty of other examples that
just happen to not be syscalls. For example, ptrace breakpoints use
the #DB vector, which isn't a syscall.
Here are few factors that may be vaguely relevant:
- uretprobe is conceptually a bit like sigreturn in the sense that
both of them are having the kernel help with something that process
can kind-of-sort-of do all by itself.
- BUT: sigreturn is not supposed to have side effects reaching
outside the calling task. uretprobe does, and that's the whole point.
- uretprobe-the-syscall is, in a rather optimistic sense, obsolete.
Once FRED becomes common (if ever...), it won't really serve much
purpose any more. FRED, for those not watching, at least in theory,
makes "event delivery" and "return" fast, for all (hah!) events.
Including breakpoints. And returns to usermode where rcx != rip, etc.
So I don't know what the right answer is. There's a real argument to
be made that seccomp ought to decide that its policy permits whomever
installed the uretprobe to do so, and that this decision means that
the uretprobe machinery is therefore permissible.
^ permalink raw reply
* [RFC] Extension to POSIX API for zero-copy data transfers
From: mcaju95 @ 2025-01-19 20:21 UTC (permalink / raw)
To: linux-api, alx, serge
Greetings,
I've been thinking about a POSIX-like API that would allow
read/write/send/recv to be zero-copy instead of being buffered. As
such, storage devices and network sockets can have data transferred to
and from them directly to a user-space application's buffers.
My focus was initially on network stacks and I drew inspiration from
DPDK. I'm also aware of some work underway on extending io_uring to
support zero copy.
A draft API would work as follows:
* The application fills-out a series of iovec's with buffers in its own
memory that can store data from protocols such as TCP or UDP. These
iovec's will serve as hints that will tell the network stack that it
can definitely map a part of a frame's contents into the described
buffers. For example, an iovec may contain { .iov_base = 0x4000,
.iov_len = 0xa000 }. In this case, the data payload may end-up anywhere
between 0x4000 and 0xe000 and after the syscall, its fields will be
overwritten to something like { .iov_base = 0x4036, .iov_len = 1460 }
* In order to receive packets, the application calls readv or a
readv-like syscall and its array of iovec will be modified to point to
data payloads. Given that their pages will be mapped directly to
user-space, some header fields or tail-room may have to be zero-ed out
before being mapped, in order to prevent information leaks. Anny array
of iovec's passed to one such readv syscall should be checked for
sanity such as being able to hold data payloads in corner cases, not
overlap with each-other and hold values that would prove to map pages
to.
* The return value would be the number of data payloads that have been
populated. Only the first such elements in the provided array would
end-up containing data payloads.
* The syscall's prototype would be quite identical to that of readv,
except that iov would not be a const struct iovec *, but just a struct
iovec * and the return type would be modified. Like so:
int zc_readv(int fd, struct iovec *iov, int iovcnt);
* In the case of write's a struct iovec may not suffice as the provided
buffers should not only provide the location and size of data to be
sent, but also the guarantee that the buffers have sufficient head and
tail room. A hackish syscall would look like so:
int zc_writev(int fd, const struct iovec (*iov)[2], int iovcnt);
* While the first iovec should describe the entire memory area
available to a packet, including enough head and tail room for headers
and CRC's or other fields specific to the NIC, the second should
describe a sub-buffer that holds the data to be written.
* Again, sanity checks should be performed on the entire array, for
things like having enough room for other fields, not overlapping,
proper alignment, ability to DMA to a device, etc.
* After calling zc_writev the pages associated with the provided
iovec's are immediately swapped for zero-pages to avoid data-leaks.
* For writes, arbitrary physical pages may not work for every NIC as
some are bound by 32bit addressing constrains on the PCIe bus, etc. As
such the application would have to manage a memory pool associated with
each file-descriptor(possibly NIC) that would contain memory that is
physically mapped to areas that can be DMA'ed to the proper devices.
For example one may mmap the file-descriptor to obtain a pool of a
certain size for this purpose.
This concept can be extended to storage devices, unfortunately I am
unfamiliar with NVMe and SCSI so I can only guess that they work in a
similar manner to NIC rings, in that data can be written and read to
arbitrary physical RAM(as allowed by the IOMMU). Syscalls similar to
zc_read and zc_write can be used on file descriptors pointing to
storage devices to fetch or write sectors that contain data belonging
to files. Some data should be zeroed-out in this case as well, as
sectors more often that not will contain data that does not belong to
the intended files.
For example one can mix such syscalls to read directly from storage
into NIC buffers, providing in-place encryption on the way(via TLS) and
send them to a client in a similar way that Netflix does with in-kernel
TLS and sendfile.
All the best,
Mihai
^ permalink raw reply
* Re: [PATCH RFC v3 02/10] sched_getattr: port to copy_struct_to_user
From: Florian Weimer @ 2025-01-20 5:28 UTC (permalink / raw)
To: Xi Ruoyao
Cc: Christian Brauner, Aleksa Sarai, Ingo Molnar, Peter Zijlstra,
Juri Lelli, Vincent Guittot, Dietmar Eggemann, Steven Rostedt,
Ben Segall, Mel Gorman, Valentin Schneider, Alexander Viro,
Jan Kara, Arnd Bergmann, Shuah Khan, Kees Cook, Mark Rutland,
linux-kernel, linux-api, linux-fsdevel, linux-arch,
linux-kselftest, libc-alpha
In-Reply-To: <82ee186ae5580548fe6b0edd2720359c18f6fa9a.camel@xry111.site>
* Xi Ruoyao:
> On Wed, 2024-12-11 at 11:23 +0100, Christian Brauner wrote:
>> On Tue, Dec 10, 2024 at 07:14:07PM +0100, Florian Weimer wrote:
>> > * Aleksa Sarai:
>> >
>> > > sched_getattr(2) doesn't care about trailing non-zero bytes in the
>> > > (ksize > usize) case, so just use copy_struct_to_user() without checking
>> > > ignored_trailing.
>> >
>> > I think this is what causes glibc's misc/tst-sched_setattr test to fail
>> > on recent kernels. The previous non-modifying behavior was documented
>> > in the manual page:
>> >
>> > If the caller-provided attr buffer is larger than the kernel's
>> > sched_attr structure, the additional bytes in the user-space
>> > structure are not touched.
>> >
>> > I can just drop this part of the test if the kernel deems both behaviors
>> > valid.
>
>> I think in general both behaviors are valid but I would consider zeroing
>> the unknown parts of the provided buffer to be the safer option. And all
>> newer extensible struct system calls do that.
>
> Florian,
>
> So should we drop the test before Glibc-2.41 release? I'm seeing the
> failure during my machine test.
I was waiting for a verdict from the kernel developers. I didn't expect
such a change to happen given the alleged UAPI policy.
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH RFC v3 02/10] sched_getattr: port to copy_struct_to_user
From: Xi Ruoyao @ 2025-01-20 9:21 UTC (permalink / raw)
To: Florian Weimer, Christian Brauner
Cc: Aleksa Sarai, Ingo Molnar, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Alexander Viro, Jan Kara,
Arnd Bergmann, Shuah Khan, Kees Cook, Mark Rutland, linux-kernel,
linux-api, linux-fsdevel, linux-arch, linux-kselftest, libc-alpha
In-Reply-To: <87jzaqdpfe.fsf@oldenburg.str.redhat.com>
On Mon, 2025-01-20 at 06:28 +0100, Florian Weimer wrote:
> * Xi Ruoyao:
>
> > On Wed, 2024-12-11 at 11:23 +0100, Christian Brauner wrote:
> > > On Tue, Dec 10, 2024 at 07:14:07PM +0100, Florian Weimer wrote:
> > > > * Aleksa Sarai:
> > > >
> > > > > sched_getattr(2) doesn't care about trailing non-zero bytes in the
> > > > > (ksize > usize) case, so just use copy_struct_to_user() without checking
> > > > > ignored_trailing.
> > > >
> > > > I think this is what causes glibc's misc/tst-sched_setattr test to fail
> > > > on recent kernels. The previous non-modifying behavior was documented
> > > > in the manual page:
> > > >
> > > > If the caller-provided attr buffer is larger than the kernel's
> > > > sched_attr structure, the additional bytes in the user-space
> > > > structure are not touched.
> > > >
> > > > I can just drop this part of the test if the kernel deems both behaviors
> > > > valid.
> >
> > > I think in general both behaviors are valid but I would consider zeroing
> > > the unknown parts of the provided buffer to be the safer option. And all
> > > newer extensible struct system calls do that.
> >
> > Florian,
> >
> > So should we drop the test before Glibc-2.41 release? I'm seeing the
> > failure during my machine test.
> I was waiting for a verdict from the kernel developers. I didn't expect
> such a change to happen given the alleged UAPI policy.
But 6.13 is already released without reverting the behavior change
now... So is this the "final" verdict?
--
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University
^ permalink raw reply
* Re: [PATCH RFC v3 02/10] sched_getattr: port to copy_struct_to_user
From: Florian Weimer @ 2025-01-20 9:51 UTC (permalink / raw)
To: Xi Ruoyao
Cc: Christian Brauner, Aleksa Sarai, Ingo Molnar, Peter Zijlstra,
Juri Lelli, Vincent Guittot, Dietmar Eggemann, Steven Rostedt,
Ben Segall, Mel Gorman, Valentin Schneider, Alexander Viro,
Jan Kara, Arnd Bergmann, Shuah Khan, Kees Cook, Mark Rutland,
linux-kernel, linux-api, linux-fsdevel, linux-arch,
linux-kselftest, libc-alpha
In-Reply-To: <b66580447aa94593136186a4046fd350e598943a.camel@xry111.site>
* Xi Ruoyao:
>> > So should we drop the test before Glibc-2.41 release? I'm seeing the
>> > failure during my machine test.
>> I was waiting for a verdict from the kernel developers. I didn't expect
>> such a change to happen given the alleged UAPI policy.
>
> But 6.13 is already released without reverting the behavior change
> now... So is this the "final" verdict?
I had originally missed the Linux 6.13 release. I've submitted a glibc
patch:
[PATCH] Linux: Do not check unused bytes after sched_getattr in
tst-sched_setattr
<https://inbox.sourceware.org/libc-alpha/87v7u9ddas.fsf@oldenburg.str.redhat.com/>
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH v2 6/7] ptrace: introduce PTRACE_SET_SYSCALL_INFO request
From: Oleg Nesterov @ 2025-01-20 19:56 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Eugene Syromyatnikov, Mike Frysinger, Renzo Davoli,
Davide Berardi, strace-devel, linux-kernel, linux-api
In-Reply-To: <20250119124427.GA3487@strace.io>
On 01/19, Dmitry V. Levin wrote:
>
> On Sat, Jan 18, 2025 at 03:13:42PM +0100, Oleg Nesterov wrote:
> > On 01/17, Dmitry V. Levin wrote:
> [...]
> > > For example, on x86_64 sizeof(struct ptrace_syscall_info) is currently 88,
> > > while on x86 it is 84.
> >
> > Not good, but too late to complain...
>
> Actually, I don't think it's too late to add an extra __u32 padding
> there since it wouldn't affect PTRACE_GET_SYSCALL_INFO.
Hmm, indeed thanks for correcting me. I forgot that ptrace_get_syscall_info()
returns actual_size, not sizeof().
> I can add an explicit padding to the structure if you say
> you like it better this way.
I dunno, up to you...
Well if we add "__u32 padding" at the end, we can probably use sizeof(info)
instead of min_size = offsetofend(struct ptrace_syscall_info, seccomp.ret_data)
in ptrace_set_syscall_info(), but then it probably makes sense to check
info->padding == 0 (just like info.flags || info.reserved) and rename this
member to reserved2.
Again, up to you, I don't know.
> > Currently we have PTRACE_SYSCALL_INFO_SIZE_VER0, when we add the new
> > "artificial" member we will have PTRACE_SYSCALL_INFO_SIZE_VER1. Granted,
> > this way set_syscall_info() can't use sizeof(info), it should do
> >
> > ptrace(PTRACE_SET_SYSCALL_INFO, PTRACE_SYSCALL_INFO_SIZE_VER1, info);
> >
> > and the kernel needs more checks, but this is what I had in mind when I said
> > that the 1st version can just require "user_size == PTRACE_SYSCALL_INFO_SIZE_VER0".
>
> ... it wouldn't be a big deal for user-space to specify also an
> appropriate "user_size", e.g. PTRACE_SYSCALL_INFO_SIZE_VER1 when it starts
> using the interface available since VER1, but it wouldn't help user-space
> programs either as they would have to update "op" and/or "flags" anyway,
Sure, and yes, "flags" is needed anyway.
> and "user_size" would become just yet another detail they have to care
> about.
True.
It is not that I ever thought that my suggestion could "help user-space".
Not at all. Just imo it would be better to fail "early" on the older kernel
in the case when user-space expects the "extended" API, even if flags == 0.
And no, it is not that I am 100% sure it would be always better.
So let me repeat: please do what you think is right, I won't argue. I just
tried to understand your points and explain mine to ensure we more or less
understand each other.
Oleg.
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Kees Cook @ 2025-01-20 21:32 UTC (permalink / raw)
To: Oleg Nesterov
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: <20250119123955.GA5281@redhat.com>
On Sun, Jan 19, 2025 at 01:40:22PM +0100, Oleg Nesterov wrote:
> On 01/18, Kees Cook wrote:
> >
> > On Thu, Jan 16, 2025 at 04:55:39PM -0800, Eyal Birger wrote:
> > > 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.
> >
> > How is this any different from sigreturn, rt_sigreturn, or
> > restart_syscall? These are all handled explicitly by userspace filters
> > already, and I don't see why uretprobe should be any different.
>
> 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.
SECCOMP_SET_MODE_STRICT will also crash in the face of syscall_restart...
--
Kees Cook
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Kees Cook @ 2025-01-20 21:34 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: <CAHsH6GtpXMswVKytv7_JMGca=3wxKRUK4rZmBBxJPRh1WYdObg@mail.gmail.com>
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).
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v6 01/15] lib: Add TLV parser
From: Thomas Weißschuh @ 2025-01-21 13:29 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: <20241119104922.2772571-2-roberto.sassu@huaweicloud.com>
Hi Robert,
On 2024-11-19 11:49:08+0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
>
> Add a parser of a generic Type-Length-Value (TLV) format:
>
> +--------------+--+---------+--------+---------+
> | field1 (u16) | len1 (u32) | value1 (u8 len1) |
> +--------------+------------+------------------+
> | ... | ... | ... |
> +--------------+------------+------------------+
> | fieldN (u16) | lenN (u32) | valueN (u8 lenN) |
> +--------------+------------+------------------+
Should mention that its big endian.
> Each adopter can define its own fields. The TLV parser does not need to be
> aware of those, but lets the adopter obtain the data and decide how to
"adopter" -> "user".
> continue.
>
> After processing a TLV entry, call the callback function also with the
> callback data provided by the adopter. The latter can decide how to
> interpret the TLV entry depending on the field ID.
>
> Nesting TLVs is also possible, the callback function can call tlv_parse()
> to parse the inner structure.
Given that we already have the netlink data structures, helpers and
infrastructure, what is the advantage over those?
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
> MAINTAINERS | 8 +++
> include/linux/tlv_parser.h | 32 ++++++++++++
> include/uapi/linux/tlv_parser.h | 41 ++++++++++++++++
> lib/Kconfig | 3 ++
> lib/Makefile | 2 +
> lib/tlv_parser.c | 87 +++++++++++++++++++++++++++++++++
> lib/tlv_parser.h | 18 +++++++
> 7 files changed, 191 insertions(+)
> create mode 100644 include/linux/tlv_parser.h
> create mode 100644 include/uapi/linux/tlv_parser.h
> create mode 100644 lib/tlv_parser.c
> create mode 100644 lib/tlv_parser.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a097afd76ded..1f7ffa1c9dbd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -23388,6 +23388,14 @@ W: http://sourceforge.net/projects/tlan/
> F: Documentation/networking/device_drivers/ethernet/ti/tlan.rst
> F: drivers/net/ethernet/ti/tlan.*
>
> +TLV PARSER
> +M: Roberto Sassu <roberto.sassu@huawei.com>
> +L: linux-kernel@vger.kernel.org
> +S: Maintained
> +F: include/linux/tlv_parser.h
> +F: include/uapi/linux/tlv_parser.h
> +F: lib/tlv_parser.*
> +
> TMIO/SDHI MMC DRIVER
> M: Wolfram Sang <wsa+renesas@sang-engineering.com>
> L: linux-mmc@vger.kernel.org
> diff --git a/include/linux/tlv_parser.h b/include/linux/tlv_parser.h
> new file mode 100644
> index 000000000000..0c72742af548
> --- /dev/null
> +++ b/include/linux/tlv_parser.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2023-2024 Huawei Technologies Duesseldorf GmbH
> + *
> + * Author: Roberto Sassu <roberto.sassu@huawei.com>
> + *
> + * Header file of TLV parser.
> + */
> +
> +#ifndef _LINUX_TLV_PARSER_H
> +#define _LINUX_TLV_PARSER_H
> +
> +#include <uapi/linux/tlv_parser.h>
> +
> +/**
> + * typedef callback - Callback after parsing TLV entry
> + * @callback_data: Opaque data to supply to the callback function
> + * @field: Field identifier
> + * @field_data: Field data
> + * @field_len: Length of @field_data
> + *
> + * This callback is invoked after a TLV entry is parsed.
> + *
> + * Return: Zero on success, a negative value on error.
It's not explained what happens on error.
> + */
> +typedef int (*callback)(void *callback_data, __u16 field,
> + const __u8 *field_data, __u32 field_len);
No need for __underscore types in kernel-only signatures.
> +
> +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?
> +
> +#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;
Use __be16 and __be32 here.
> + __u8 data[];
__counted_by()?
Not sure how this interacts with __be.
> +} __attribute__((packed));
> +
> +#endif /* _UAPI_LINUX_TLV_PARSER_H */
> diff --git a/lib/Kconfig b/lib/Kconfig
> index b38849af6f13..9141dcfc1704 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -777,3 +777,6 @@ config POLYNOMIAL
>
> config FIRMWARE_TABLE
> bool
> +
> +config TLV_PARSER
> + bool
> diff --git a/lib/Makefile b/lib/Makefile
> index 773adf88af41..630de494eab5 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -393,5 +393,7 @@ obj-$(CONFIG_USERCOPY_KUNIT_TEST) += usercopy_kunit.o
> obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o
>
> obj-$(CONFIG_FIRMWARE_TABLE) += fw_table.o
> +obj-$(CONFIG_TLV_PARSER) += tlv_parser.o
> +CFLAGS_tlv_parser.o += -I lib
Does this work with out of tree builds?
>
> subdir-$(CONFIG_FORTIFY_SOURCE) += test_fortify
> diff --git a/lib/tlv_parser.c b/lib/tlv_parser.c
> new file mode 100644
> index 000000000000..dbbe08018b4d
> --- /dev/null
> +++ b/lib/tlv_parser.c
> @@ -0,0 +1,87 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2023-2024 Huawei Technologies Duesseldorf GmbH
> + *
> + * Author: Roberto Sassu <roberto.sassu@huawei.com>
> + *
> + * Implement the TLV parser.
> + */
> +
> +#define pr_fmt(fmt) "tlv_parser: "fmt
> +#include <tlv_parser.h>
This should be "tlv_parser.h",
but the header files looks unnecessary in the first place.
> +
> +/**
> + * tlv_parse - Parse TLV data
> + * @callback: Callback function to call to parse the entries
> + * @callback_data: Opaque data to supply to the callback function
> + * @data: Data to parse
> + * @data_len: Length of @data
> + * @fields: Array of field strings
> + * @num_fields: Number of elements of @fields
> + *
> + * Parse the TLV data format and call the supplied callback function for each
> + * entry, passing also the opaque data pointer.
> + *
> + * The callback function decides how to process data depending on the field.
Mention that a callback return an error will abort the whole parsing.
> + *
> + * Return: Zero on success, a negative value on error.
> + */
> +int tlv_parse(callback callback, void *callback_data, const __u8 *data,
> + size_t data_len, const char **fields, __u32 num_fields)
No need for __underscore types in kernel-only functions.
"num_fields" and "fields" are accessed without checking for validity.
"fields" is only every used for debug logging, so can be removed.
"num_fields" probably, too.
> +{
> + const __u8 *data_ptr = data;
> + struct tlv_entry *entry;
This comes from the input data, should also be const.
> + __u16 parsed_field;
> + __u32 len;
field_len
> + int ret;
> +
> + if (data_len > U32_MAX) {
> + pr_debug("Data too big, size: %zd\n", data_len);
> + return -E2BIG;
> + }
> +
> + while (data_len) {
> + if (data_len < sizeof(*entry))
> + return -EBADMSG;
> +
> + entry = (struct tlv_entry *)data_ptr;
> + data_ptr += sizeof(*entry);
> + data_len -= sizeof(*entry);
> +
> + parsed_field = __be16_to_cpu(entry->field);
This doesn't seem to handle invalid alignment, some architectures will
trap unaligned accesses.
Depending on the size and usage patterns it may make sense to document
some alignment recommendations/requirements.
(Not sure how big of a performance difference it would make)
> + if (parsed_field >= num_fields) {
> + pr_debug("Invalid field %u, max: %u\n",
> + parsed_field, num_fields - 1);
> + return -EBADMSG;
> + }
> +
> + len = __be32_to_cpu(entry->length);
> +
> + if (data_len < len)
> + return -EBADMSG;
> +
> + pr_debug("Data: field: %s, len: %u\n", fields[parsed_field],
> + len);
> +
> + if (!len)
> + continue;
Empty fields are discarded silently, is this intentional?
It should be documented. Those fields could be useful for flag data.
> +
> + ret = callback(callback_data, parsed_field, data_ptr, len);
> + if (ret < 0) {
> + pr_debug("Parsing of field %s failed, ret: %d\n",
> + fields[parsed_field], ret);
> + return ret;
> + }
> +
> + data_ptr += len;
> + data_len -= len;
> + }
> +
> + if (data_len) {
Can this ever happen?
The check at the beginning of the loop should have caught it already.
> + pr_debug("Excess data: %zu bytes\n", data_len);
> + return -EBADMSG;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(tlv_parse);
Some kunit tests would be great.
> diff --git a/lib/tlv_parser.h b/lib/tlv_parser.h
> new file mode 100644
> index 000000000000..e663966deac5
> --- /dev/null
> +++ b/lib/tlv_parser.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2023-2024 Huawei Technologies Duesseldorf GmbH
> + *
> + * Author: Roberto Sassu <roberto.sassu@huawei.com>
> + *
> + * Header file of TLV parser.
> + */
> +
> +#ifndef _LIB_TLV_PARSER_H
> +#define _LIB_TLV_PARSER_H
> +
> +#include <linux/kernel.h>
> +#include <linux/err.h>
> +#include <linux/limits.h>
> +#include <linux/tlv_parser.h>
The #includes should move to the .c file and the header be removed.
> +
> +#endif /* _LIB_TLV_PARSER_H */
> --
> 2.47.0.118.gfd3785337b
>
^ permalink raw reply
* Re: [PATCH v6 01/15] lib: Add TLV parser
From: Roberto Sassu @ 2025-01-21 13:48 UTC (permalink / raw)
To: Thomas Weißschuh
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: <c316b1be-d18f-4bb0-8434-bcc9236619df@t-8ch.de>
On Tue, 2025-01-21 at 14:29 +0100, Thomas Weißschuh wrote:
> Hi Robert,
>
> On 2024-11-19 11:49:08+0100, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> >
> > Add a parser of a generic Type-Length-Value (TLV) format:
> >
> > +--------------+--+---------+--------+---------+
> > > field1 (u16) | len1 (u32) | value1 (u8 len1) |
> > +--------------+------------+------------------+
> > > ... | ... | ... |
> > +--------------+------------+------------------+
> > > fieldN (u16) | lenN (u32) | valueN (u8 lenN) |
> > +--------------+------------+------------------+
>
> Should mention that its big endian.
Ok.
> > Each adopter can define its own fields. The TLV parser does not need to be
> > aware of those, but lets the adopter obtain the data and decide how to
>
> "adopter" -> "user".
Ok.
> > continue.
> >
> > After processing a TLV entry, call the callback function also with the
> > callback data provided by the adopter. The latter can decide how to
> > interpret the TLV entry depending on the field ID.
> >
> > Nesting TLVs is also possible, the callback function can call tlv_parse()
> > to parse the inner structure.
>
> Given that we already have the netlink data structures, helpers and
> infrastructure, what is the advantage over those?
Sorry, I'm not too familiar on how netlink works, so I might not
understand your point.
I think the benefit of this data structure is the retrocompatibility.
If you add new data fields, you don't need to introduce a v2, v3 data
format.
New versions of the parser can consume the new information, while the
older can still take the ones they are able to understand.
Thanks
Roberto
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> > MAINTAINERS | 8 +++
> > include/linux/tlv_parser.h | 32 ++++++++++++
> > include/uapi/linux/tlv_parser.h | 41 ++++++++++++++++
> > lib/Kconfig | 3 ++
> > lib/Makefile | 2 +
> > lib/tlv_parser.c | 87 +++++++++++++++++++++++++++++++++
> > lib/tlv_parser.h | 18 +++++++
> > 7 files changed, 191 insertions(+)
> > create mode 100644 include/linux/tlv_parser.h
> > create mode 100644 include/uapi/linux/tlv_parser.h
> > create mode 100644 lib/tlv_parser.c
> > create mode 100644 lib/tlv_parser.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index a097afd76ded..1f7ffa1c9dbd 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -23388,6 +23388,14 @@ W: http://sourceforge.net/projects/tlan/
> > F: Documentation/networking/device_drivers/ethernet/ti/tlan.rst
> > F: drivers/net/ethernet/ti/tlan.*
> >
> > +TLV PARSER
> > +M: Roberto Sassu <roberto.sassu@huawei.com>
> > +L: linux-kernel@vger.kernel.org
> > +S: Maintained
> > +F: include/linux/tlv_parser.h
> > +F: include/uapi/linux/tlv_parser.h
> > +F: lib/tlv_parser.*
> > +
> > TMIO/SDHI MMC DRIVER
> > M: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > L: linux-mmc@vger.kernel.org
> > diff --git a/include/linux/tlv_parser.h b/include/linux/tlv_parser.h
> > new file mode 100644
> > index 000000000000..0c72742af548
> > --- /dev/null
> > +++ b/include/linux/tlv_parser.h
> > @@ -0,0 +1,32 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2023-2024 Huawei Technologies Duesseldorf GmbH
> > + *
> > + * Author: Roberto Sassu <roberto.sassu@huawei.com>
> > + *
> > + * Header file of TLV parser.
> > + */
> > +
> > +#ifndef _LINUX_TLV_PARSER_H
> > +#define _LINUX_TLV_PARSER_H
> > +
> > +#include <uapi/linux/tlv_parser.h>
> > +
> > +/**
> > + * typedef callback - Callback after parsing TLV entry
> > + * @callback_data: Opaque data to supply to the callback function
> > + * @field: Field identifier
> > + * @field_data: Field data
> > + * @field_len: Length of @field_data
> > + *
> > + * This callback is invoked after a TLV entry is parsed.
> > + *
> > + * Return: Zero on success, a negative value on error.
>
> It's not explained what happens on error.
>
> > + */
> > +typedef int (*callback)(void *callback_data, __u16 field,
> > + const __u8 *field_data, __u32 field_len);
>
> No need for __underscore types in kernel-only signatures.
>
> > +
> > +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?
>
> > +
> > +#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;
>
> Use __be16 and __be32 here.
>
> > + __u8 data[];
>
> __counted_by()?
> Not sure how this interacts with __be.
>
> > +} __attribute__((packed));
> > +
> > +#endif /* _UAPI_LINUX_TLV_PARSER_H */
> > diff --git a/lib/Kconfig b/lib/Kconfig
> > index b38849af6f13..9141dcfc1704 100644
> > --- a/lib/Kconfig
> > +++ b/lib/Kconfig
> > @@ -777,3 +777,6 @@ config POLYNOMIAL
> >
> > config FIRMWARE_TABLE
> > bool
> > +
> > +config TLV_PARSER
> > + bool
> > diff --git a/lib/Makefile b/lib/Makefile
> > index 773adf88af41..630de494eab5 100644
> > --- a/lib/Makefile
> > +++ b/lib/Makefile
> > @@ -393,5 +393,7 @@ obj-$(CONFIG_USERCOPY_KUNIT_TEST) += usercopy_kunit.o
> > obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o
> >
> > obj-$(CONFIG_FIRMWARE_TABLE) += fw_table.o
> > +obj-$(CONFIG_TLV_PARSER) += tlv_parser.o
> > +CFLAGS_tlv_parser.o += -I lib
>
> Does this work with out of tree builds?
>
> >
> > subdir-$(CONFIG_FORTIFY_SOURCE) += test_fortify
> > diff --git a/lib/tlv_parser.c b/lib/tlv_parser.c
> > new file mode 100644
> > index 000000000000..dbbe08018b4d
> > --- /dev/null
> > +++ b/lib/tlv_parser.c
> > @@ -0,0 +1,87 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2023-2024 Huawei Technologies Duesseldorf GmbH
> > + *
> > + * Author: Roberto Sassu <roberto.sassu@huawei.com>
> > + *
> > + * Implement the TLV parser.
> > + */
> > +
> > +#define pr_fmt(fmt) "tlv_parser: "fmt
> > +#include <tlv_parser.h>
>
> This should be "tlv_parser.h",
> but the header files looks unnecessary in the first place.
>
> > +
> > +/**
> > + * tlv_parse - Parse TLV data
> > + * @callback: Callback function to call to parse the entries
> > + * @callback_data: Opaque data to supply to the callback function
> > + * @data: Data to parse
> > + * @data_len: Length of @data
> > + * @fields: Array of field strings
> > + * @num_fields: Number of elements of @fields
> > + *
> > + * Parse the TLV data format and call the supplied callback function for each
> > + * entry, passing also the opaque data pointer.
> > + *
> > + * The callback function decides how to process data depending on the field.
>
> Mention that a callback return an error will abort the whole parsing.
>
> > + *
> > + * Return: Zero on success, a negative value on error.
> > + */
> > +int tlv_parse(callback callback, void *callback_data, const __u8 *data,
> > + size_t data_len, const char **fields, __u32 num_fields)
>
> No need for __underscore types in kernel-only functions.
>
> "num_fields" and "fields" are accessed without checking for validity.
>
> "fields" is only every used for debug logging, so can be removed.
> "num_fields" probably, too.
>
> > +{
> > + const __u8 *data_ptr = data;
> > + struct tlv_entry *entry;
>
> This comes from the input data, should also be const.
>
> > + __u16 parsed_field;
> > + __u32 len;
>
> field_len
>
> > + int ret;
> > +
> > + if (data_len > U32_MAX) {
> > + pr_debug("Data too big, size: %zd\n", data_len);
> > + return -E2BIG;
> > + }
> > +
> > + while (data_len) {
> > + if (data_len < sizeof(*entry))
> > + return -EBADMSG;
> > +
> > + entry = (struct tlv_entry *)data_ptr;
> > + data_ptr += sizeof(*entry);
> > + data_len -= sizeof(*entry);
> > +
> > + parsed_field = __be16_to_cpu(entry->field);
>
> This doesn't seem to handle invalid alignment, some architectures will
> trap unaligned accesses.
> Depending on the size and usage patterns it may make sense to document
> some alignment recommendations/requirements.
> (Not sure how big of a performance difference it would make)
>
> > + if (parsed_field >= num_fields) {
> > + pr_debug("Invalid field %u, max: %u\n",
> > + parsed_field, num_fields - 1);
> > + return -EBADMSG;
> > + }
> > +
> > + len = __be32_to_cpu(entry->length);
> > +
> > + if (data_len < len)
> > + return -EBADMSG;
> > +
> > + pr_debug("Data: field: %s, len: %u\n", fields[parsed_field],
> > + len);
> > +
> > + if (!len)
> > + continue;
>
> Empty fields are discarded silently, is this intentional?
> It should be documented. Those fields could be useful for flag data.
>
> > +
> > + ret = callback(callback_data, parsed_field, data_ptr, len);
> > + if (ret < 0) {
> > + pr_debug("Parsing of field %s failed, ret: %d\n",
> > + fields[parsed_field], ret);
> > + return ret;
> > + }
> > +
> > + data_ptr += len;
> > + data_len -= len;
> > + }
> > +
> > + if (data_len) {
>
> Can this ever happen?
> The check at the beginning of the loop should have caught it already.
>
> > + pr_debug("Excess data: %zu bytes\n", data_len);
> > + return -EBADMSG;
> > + }
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(tlv_parse);
>
> Some kunit tests would be great.
>
> > diff --git a/lib/tlv_parser.h b/lib/tlv_parser.h
> > new file mode 100644
> > index 000000000000..e663966deac5
> > --- /dev/null
> > +++ b/lib/tlv_parser.h
> > @@ -0,0 +1,18 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2023-2024 Huawei Technologies Duesseldorf GmbH
> > + *
> > + * Author: Roberto Sassu <roberto.sassu@huawei.com>
> > + *
> > + * Header file of TLV parser.
> > + */
> > +
> > +#ifndef _LIB_TLV_PARSER_H
> > +#define _LIB_TLV_PARSER_H
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/err.h>
> > +#include <linux/limits.h>
> > +#include <linux/tlv_parser.h>
>
> The #includes should move to the .c file and the header be removed.
>
> > +
> > +#endif /* _LIB_TLV_PARSER_H */
> > --
> > 2.47.0.118.gfd3785337b
> >
^ permalink raw reply
* Re: [PATCH v6 01/15] lib: Add TLV parser
From: Thomas Weißschuh @ 2025-01-21 14:21 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: <fa52e3654a44dd250437ebe3e8397bff95399893.camel@huaweicloud.com>
On 2025-01-21 14:48:09+0100, Roberto Sassu wrote:
> On Tue, 2025-01-21 at 14:29 +0100, Thomas Weißschuh wrote:
> > Hi Robert,
> >
> > On 2024-11-19 11:49:08+0100, Roberto Sassu wrote:
> > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > >
> > > Add a parser of a generic Type-Length-Value (TLV) format:
> > >
> > > +--------------+--+---------+--------+---------+
> > > > field1 (u16) | len1 (u32) | value1 (u8 len1) |
> > > +--------------+------------+------------------+
> > > > ... | ... | ... |
> > > +--------------+------------+------------------+
> > > > fieldN (u16) | lenN (u32) | valueN (u8 lenN) |
> > > +--------------+------------+------------------+
> >
> > Should mention that its big endian.
>
> Ok.
>
> > > Each adopter can define its own fields. The TLV parser does not need to be
> > > aware of those, but lets the adopter obtain the data and decide how to
> >
> > "adopter" -> "user".
>
> Ok.
>
> > > continue.
> > >
> > > After processing a TLV entry, call the callback function also with the
> > > callback data provided by the adopter. The latter can decide how to
> > > interpret the TLV entry depending on the field ID.
> > >
> > > Nesting TLVs is also possible, the callback function can call tlv_parse()
> > > to parse the inner structure.
> >
> > Given that we already have the netlink data structures, helpers and
> > infrastructure, what is the advantage over those?
>
> Sorry, I'm not too familiar on how netlink works, so I might not
> understand your point.
Netlink is a TLV format used by the Linux networking subsystem:
struct nlmsghdr {
__u32 nlmsg_len; /* Length of message including header */
__u16 nlmsg_type; /* Type of message content */
__u16 nlmsg_flags; /* Additional flags */
__u32 nlmsg_seq; /* Sequence number */
__u32 nlmsg_pid; /* Sender port ID */
};
https://man.archlinux.org/man/core/man-pages/netlink.7.en
There are both userspace and in-kernel infrastructures to handle it.
Looking at it again however it has some unnecessary fields, wasting
space and uses "host" byteorder which is a problem for an on-disk
datastructure.
So maybe not a good alternative after all.
> I think the benefit of this data structure is the retrocompatibility.
> If you add new data fields, you don't need to introduce a v2, v3 data
> format.
>
> New versions of the parser can consume the new information, while the
> older can still take the ones they are able to understand.
This is also exactly how netlink is used.
FYI there were also some review comments inside the patch diff itself.
Thomas
^ permalink raw reply
* Re: [PATCH v6 01/15] lib: Add TLV parser
From: Roberto Sassu @ 2025-01-21 14:29 UTC (permalink / raw)
To: Thomas Weißschuh
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: <7b3eaf6d-ca5e-47c9-b26d-8953aece869f@t-8ch.de>
On Tue, 2025-01-21 at 15:21 +0100, Thomas Weißschuh wrote:
> On 2025-01-21 14:48:09+0100, Roberto Sassu wrote:
> > On Tue, 2025-01-21 at 14:29 +0100, Thomas Weißschuh wrote:
> > > Hi Robert,
> > >
> > > On 2024-11-19 11:49:08+0100, Roberto Sassu wrote:
> > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > >
> > > > Add a parser of a generic Type-Length-Value (TLV) format:
> > > >
> > > > +--------------+--+---------+--------+---------+
> > > > > field1 (u16) | len1 (u32) | value1 (u8 len1) |
> > > > +--------------+------------+------------------+
> > > > > ... | ... | ... |
> > > > +--------------+------------+------------------+
> > > > > fieldN (u16) | lenN (u32) | valueN (u8 lenN) |
> > > > +--------------+------------+------------------+
> > >
> > > Should mention that its big endian.
> >
> > Ok.
> >
> > > > Each adopter can define its own fields. The TLV parser does not need to be
> > > > aware of those, but lets the adopter obtain the data and decide how to
> > >
> > > "adopter" -> "user".
> >
> > Ok.
> >
> > > > continue.
> > > >
> > > > After processing a TLV entry, call the callback function also with the
> > > > callback data provided by the adopter. The latter can decide how to
> > > > interpret the TLV entry depending on the field ID.
> > > >
> > > > Nesting TLVs is also possible, the callback function can call tlv_parse()
> > > > to parse the inner structure.
> > >
> > > Given that we already have the netlink data structures, helpers and
> > > infrastructure, what is the advantage over those?
> >
> > Sorry, I'm not too familiar on how netlink works, so I might not
> > understand your point.
>
> Netlink is a TLV format used by the Linux networking subsystem:
>
> struct nlmsghdr {
> __u32 nlmsg_len; /* Length of message including header */
> __u16 nlmsg_type; /* Type of message content */
> __u16 nlmsg_flags; /* Additional flags */
> __u32 nlmsg_seq; /* Sequence number */
> __u32 nlmsg_pid; /* Sender port ID */
> };
>
> https://man.archlinux.org/man/core/man-pages/netlink.7.en
>
> There are both userspace and in-kernel infrastructures to handle it.
> Looking at it again however it has some unnecessary fields, wasting
> space and uses "host" byteorder which is a problem for an on-disk
> datastructure.
> So maybe not a good alternative after all.
>
> > I think the benefit of this data structure is the retrocompatibility.
> > If you add new data fields, you don't need to introduce a v2, v3 data
> > format.
> >
> > New versions of the parser can consume the new information, while the
> > older can still take the ones they are able to understand.
>
> This is also exactly how netlink is used.
Ok, perfect!
> FYI there were also some review comments inside the patch diff itself.
Ops, thanks! Missed them. Will reply to that email.
Roberto
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Jiri Olsa @ 2025-01-21 14:38 UTC (permalink / raw)
To: Eyal Birger
Cc: Kees Cook, luto, wad, oleg, ldv, mhiramat, andrii,
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: <CAHsH6GtpXMswVKytv7_JMGca=3wxKRUK4rZmBBxJPRh1WYdObg@mail.gmail.com>
On Sat, Jan 18, 2025 at 07:39:25PM -0800, Eyal Birger wrote:
SNIP
> I think I wasn't accurate in my wording.
> The uretprobe syscall is added to the tracee by the kernel.
> The tracer itself is merely requesting to attach a uretprobe bpf
> function. In previous versions, this was implemented by the kernel
> installing an int3 instruction, and in the new implementation the kernel
> is installing a uretprobe syscall.
> The "user" in this case - the tracer program - didn't deliberately install
> the syscall, but anyway this is semantics.
>
> I think I understand your point that it is regarded as "policy", only that
> it creates a problem in actual deployments, where in order to be able to
> run the tracer software which has been working on newer kernels a new docker
> has to be deployed.
>
> I'm trying to find a pragmatic solution to this problem, and I understand
> the motivation to avoid policy in seccomp.
>
> Alternatively, maybe this syscall implementation should be reverted?
you mentioned in the previous reply:
> > As far as I can tell libseccomp needs to provide support for this new
> > syscall and a new docker version would need to be deployed, so It's not
> > just a configuration change. Also the default policy which comes packed in
> > docker would probably need to be changed to avoid having to explicitly
> > provide a seccomp configuration for each deployment.
please disregard if this is too stupid.. but could another way out be just
to disable it (easy to do) and meanwhile teach libseccomp to allow uretprobe
(or whatever mechanism needs to be added to libseccomp) plus the needed
docker change ... to minimize the impact ?
or there's just too many other seccomp user space libraries
I'm still trying to come up with some other solution but wanted
to exhaust all the options I could think of
thanks,
jirka
^ permalink raw reply
* Re: [PATCH] seccomp: passthrough uretprobe systemcall without filtering
From: Eyal Birger @ 2025-01-21 14:47 UTC (permalink / raw)
To: Jiri Olsa
Cc: Kees Cook, luto, wad, oleg, ldv, mhiramat, andrii,
alexei.starovoitov, 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: <Z4-xeFH0Mgo3llga@krava>
Hi,
On Tue, Jan 21, 2025 at 6:38 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Sat, Jan 18, 2025 at 07:39:25PM -0800, Eyal Birger wrote:
>
> SNIP
>
> > I think I wasn't accurate in my wording.
> > The uretprobe syscall is added to the tracee by the kernel.
> > The tracer itself is merely requesting to attach a uretprobe bpf
> > function. In previous versions, this was implemented by the kernel
> > installing an int3 instruction, and in the new implementation the kernel
> > is installing a uretprobe syscall.
> > The "user" in this case - the tracer program - didn't deliberately install
> > the syscall, but anyway this is semantics.
> >
> > I think I understand your point that it is regarded as "policy", only that
> > it creates a problem in actual deployments, where in order to be able to
> > run the tracer software which has been working on newer kernels a new docker
> > has to be deployed.
> >
> > I'm trying to find a pragmatic solution to this problem, and I understand
> > the motivation to avoid policy in seccomp.
> >
> > Alternatively, maybe this syscall implementation should be reverted?
>
> you mentioned in the previous reply:
>
> > > As far as I can tell libseccomp needs to provide support for this new
> > > syscall and a new docker version would need to be deployed, so It's not
> > > just a configuration change. Also the default policy which comes packed in
> > > docker would probably need to be changed to avoid having to explicitly
> > > provide a seccomp configuration for each deployment.
>
> please disregard if this is too stupid.. but could another way out be just
> to disable it (easy to do) and meanwhile teach libseccomp to allow uretprobe
> (or whatever mechanism needs to be added to libseccomp) plus the needed
> docker change ... to minimize the impact ?
Right. the patch I was thinking to suggest wouldn't revert the entire
thing, but instead disable its use for now and allow a careful
reconsideration of the available options.
If that makes sense, I'll post it.
>
> or there's just too many other seccomp user space libraries
I think in theory, the example of a simple binary using "restrict" mode
makes it problematic to assume that this can be fixed solely from userspace
i.e. for such binary, uretprobes would still work in one kernel version and
break on another. It's hard to tell how common this is.
>
> I'm still trying to come up with some other solution but wanted
> to exhaust all the options I could think of
>
> thanks,
> jirka
^ permalink raw reply
* Re: [PATCH v6 01/15] lib: Add TLV parser
From: Roberto Sassu @ 2025-01-21 14:55 UTC (permalink / raw)
To: Thomas Weißschuh
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: <c316b1be-d18f-4bb0-8434-bcc9236619df@t-8ch.de>
On Tue, 2025-01-21 at 14:29 +0100, Thomas Weißschuh wrote:
> Hi Robert,
>
> On 2024-11-19 11:49:08+0100, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> >
> > Add a parser of a generic Type-Length-Value (TLV) format:
> >
> > +--------------+--+---------+--------+---------+
> > > field1 (u16) | len1 (u32) | value1 (u8 len1) |
> > +--------------+------------+------------------+
> > > ... | ... | ... |
> > +--------------+------------+------------------+
> > > fieldN (u16) | lenN (u32) | valueN (u8 lenN) |
> > +--------------+------------+------------------+
>
> Should mention that its big endian.
>
> > Each adopter can define its own fields. The TLV parser does not need to be
> > aware of those, but lets the adopter obtain the data and decide how to
>
> "adopter" -> "user".
>
> > continue.
> >
> > After processing a TLV entry, call the callback function also with the
> > callback data provided by the adopter. The latter can decide how to
> > interpret the TLV entry depending on the field ID.
> >
> > Nesting TLVs is also possible, the callback function can call tlv_parse()
> > to parse the inner structure.
>
> Given that we already have the netlink data structures, helpers and
> infrastructure, what is the advantage over those?
>
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> > MAINTAINERS | 8 +++
> > include/linux/tlv_parser.h | 32 ++++++++++++
> > include/uapi/linux/tlv_parser.h | 41 ++++++++++++++++
> > lib/Kconfig | 3 ++
> > lib/Makefile | 2 +
> > lib/tlv_parser.c | 87 +++++++++++++++++++++++++++++++++
> > lib/tlv_parser.h | 18 +++++++
> > 7 files changed, 191 insertions(+)
> > create mode 100644 include/linux/tlv_parser.h
> > create mode 100644 include/uapi/linux/tlv_parser.h
> > create mode 100644 lib/tlv_parser.c
> > create mode 100644 lib/tlv_parser.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index a097afd76ded..1f7ffa1c9dbd 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -23388,6 +23388,14 @@ W: http://sourceforge.net/projects/tlan/
> > F: Documentation/networking/device_drivers/ethernet/ti/tlan.rst
> > F: drivers/net/ethernet/ti/tlan.*
> >
> > +TLV PARSER
> > +M: Roberto Sassu <roberto.sassu@huawei.com>
> > +L: linux-kernel@vger.kernel.org
> > +S: Maintained
> > +F: include/linux/tlv_parser.h
> > +F: include/uapi/linux/tlv_parser.h
> > +F: lib/tlv_parser.*
> > +
> > TMIO/SDHI MMC DRIVER
> > M: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > L: linux-mmc@vger.kernel.org
> > diff --git a/include/linux/tlv_parser.h b/include/linux/tlv_parser.h
> > new file mode 100644
> > index 000000000000..0c72742af548
> > --- /dev/null
> > +++ b/include/linux/tlv_parser.h
> > @@ -0,0 +1,32 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2023-2024 Huawei Technologies Duesseldorf GmbH
> > + *
> > + * Author: Roberto Sassu <roberto.sassu@huawei.com>
> > + *
> > + * Header file of TLV parser.
> > + */
> > +
> > +#ifndef _LINUX_TLV_PARSER_H
> > +#define _LINUX_TLV_PARSER_H
> > +
> > +#include <uapi/linux/tlv_parser.h>
> > +
> > +/**
> > + * typedef callback - Callback after parsing TLV entry
> > + * @callback_data: Opaque data to supply to the callback function
> > + * @field: Field identifier
> > + * @field_data: Field data
> > + * @field_len: Length of @field_data
> > + *
> > + * This callback is invoked after a TLV entry is parsed.
> > + *
> > + * Return: Zero on success, a negative value on error.
>
> It's not explained what happens on error.
Ok, will be more specific.
> > + */
> > +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.
> > +
> > +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.
> > +
> > +#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;
>
> Use __be16 and __be32 here.
Yes, right.
> > + __u8 data[];
>
> __counted_by()?
> Not sure how this interacts with __be.
Ok, will have a look.
> > +} __attribute__((packed));
> > +
> > +#endif /* _UAPI_LINUX_TLV_PARSER_H */
> > diff --git a/lib/Kconfig b/lib/Kconfig
> > index b38849af6f13..9141dcfc1704 100644
> > --- a/lib/Kconfig
> > +++ b/lib/Kconfig
> > @@ -777,3 +777,6 @@ config POLYNOMIAL
> >
> > config FIRMWARE_TABLE
> > bool
> > +
> > +config TLV_PARSER
> > + bool
> > diff --git a/lib/Makefile b/lib/Makefile
> > index 773adf88af41..630de494eab5 100644
> > --- a/lib/Makefile
> > +++ b/lib/Makefile
> > @@ -393,5 +393,7 @@ obj-$(CONFIG_USERCOPY_KUNIT_TEST) += usercopy_kunit.o
> > obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o
> >
> > obj-$(CONFIG_FIRMWARE_TABLE) += fw_table.o
> > +obj-$(CONFIG_TLV_PARSER) += tlv_parser.o
> > +CFLAGS_tlv_parser.o += -I lib
>
> Does this work with out of tree builds?
Good question, need to check.
> >
> > subdir-$(CONFIG_FORTIFY_SOURCE) += test_fortify
> > diff --git a/lib/tlv_parser.c b/lib/tlv_parser.c
> > new file mode 100644
> > index 000000000000..dbbe08018b4d
> > --- /dev/null
> > +++ b/lib/tlv_parser.c
> > @@ -0,0 +1,87 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2023-2024 Huawei Technologies Duesseldorf GmbH
> > + *
> > + * Author: Roberto Sassu <roberto.sassu@huawei.com>
> > + *
> > + * Implement the TLV parser.
> > + */
> > +
> > +#define pr_fmt(fmt) "tlv_parser: "fmt
> > +#include <tlv_parser.h>
>
> This should be "tlv_parser.h",
> but the header files looks unnecessary in the first place.
Ah. The point was to reuse the same file and add different includes for
the kernel and user space.
> > +
> > +/**
> > + * tlv_parse - Parse TLV data
> > + * @callback: Callback function to call to parse the entries
> > + * @callback_data: Opaque data to supply to the callback function
> > + * @data: Data to parse
> > + * @data_len: Length of @data
> > + * @fields: Array of field strings
> > + * @num_fields: Number of elements of @fields
> > + *
> > + * Parse the TLV data format and call the supplied callback function for each
> > + * entry, passing also the opaque data pointer.
> > + *
> > + * The callback function decides how to process data depending on the field.
>
> Mention that a callback return an error will abort the whole parsing.
Ok.
> > + *
> > + * Return: Zero on success, a negative value on error.
> > + */
> > +int tlv_parse(callback callback, void *callback_data, const __u8 *data,
> > + size_t data_len, const char **fields, __u32 num_fields)
>
> No need for __underscore types in kernel-only functions.
Same comment as above (used in user space).
> "num_fields" and "fields" are accessed without checking for validity.
I think it was Paul Moore suggesting that there should not be too many
checks, and that the developer should do the right thing.
> "fields" is only every used for debug logging, so can be removed.
> "num_fields" probably, too.
Ok.
> > +{
> > + const __u8 *data_ptr = data;
> > + struct tlv_entry *entry;
>
> This comes from the input data, should also be const.
Ok.
> > + __u16 parsed_field;
> > + __u32 len;
>
> field_len
Ok.
> > + int ret;
> > +
> > + if (data_len > U32_MAX) {
> > + pr_debug("Data too big, size: %zd\n", data_len);
> > + return -E2BIG;
> > + }
> > +
> > + while (data_len) {
> > + if (data_len < sizeof(*entry))
> > + return -EBADMSG;
> > +
> > + entry = (struct tlv_entry *)data_ptr;
> > + data_ptr += sizeof(*entry);
> > + data_len -= sizeof(*entry);
> > +
> > + parsed_field = __be16_to_cpu(entry->field);
>
> This doesn't seem to handle invalid alignment, some architectures will
> trap unaligned accesses.
> Depending on the size and usage patterns it may make sense to document
> some alignment recommendations/requirements.
> (Not sure how big of a performance difference it would make)
Thanks, will have a look.
> > + if (parsed_field >= num_fields) {
> > + pr_debug("Invalid field %u, max: %u\n",
> > + parsed_field, num_fields - 1);
> > + return -EBADMSG;
> > + }
> > +
> > + len = __be32_to_cpu(entry->length);
> > +
> > + if (data_len < len)
> > + return -EBADMSG;
> > +
> > + pr_debug("Data: field: %s, len: %u\n", fields[parsed_field],
> > + len);
> > +
> > + if (!len)
> > + continue;
>
> Empty fields are discarded silently, is this intentional?
> It should be documented. Those fields could be useful for flag data.
I don't remember exactly the case. Yes, I can keep them and document
them.
> > +
> > + ret = callback(callback_data, parsed_field, data_ptr, len);
> > + if (ret < 0) {
> > + pr_debug("Parsing of field %s failed, ret: %d\n",
> > + fields[parsed_field], ret);
> > + return ret;
> > + }
> > +
> > + data_ptr += len;
> > + data_len -= len;
> > + }
> > +
> > + if (data_len) {
>
> Can this ever happen?
> The check at the beginning of the loop should have caught it already.
Not anymore, it is a leftover of the previous version where I was
looping on the number of TLV data entries. Now the number of remaining
entries is part of TLV data, so only the data length is available in
tlv_parse(). Will remove, good catch!
> > + pr_debug("Excess data: %zu bytes\n", data_len);
> > + return -EBADMSG;
> > + }
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(tlv_parse);
>
> Some kunit tests would be great.
I implemented kselftests also injecting errors (patch 13). If it is not
enough, I implement kunit tests too.
> > diff --git a/lib/tlv_parser.h b/lib/tlv_parser.h
> > new file mode 100644
> > index 000000000000..e663966deac5
> > --- /dev/null
> > +++ b/lib/tlv_parser.h
> > @@ -0,0 +1,18 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2023-2024 Huawei Technologies Duesseldorf GmbH
> > + *
> > + * Author: Roberto Sassu <roberto.sassu@huawei.com>
> > + *
> > + * Header file of TLV parser.
> > + */
> > +
> > +#ifndef _LIB_TLV_PARSER_H
> > +#define _LIB_TLV_PARSER_H
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/err.h>
> > +#include <linux/limits.h>
> > +#include <linux/tlv_parser.h>
>
> The #includes should move to the .c file and the header be removed.
They are here for the reason of reusing tlv_parser.c in user space.
Thanks a lot, this was a very detailed review!
Roberto
^ permalink raw reply
* 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
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