* Re: [PATCH v2 6/7] ptrace: introduce PTRACE_SET_SYSCALL_INFO request
From: Charlie Jenkins @ 2025-01-16 21:47 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Oleg Nesterov, Eugene Syromyatnikov, Mike Frysinger, Renzo Davoli,
Davide Berardi, Celeste Liu, strace-devel, linux-kernel,
linux-api
In-Reply-To: <Z4l1L4OLoN7-lTkD@ghost>
On Thu, Jan 16, 2025 at 01:07:59PM -0800, Charlie Jenkins wrote:
> On Thu, Jan 16, 2025 at 10:33:28AM +0200, Dmitry V. Levin wrote:
> > On Wed, Jan 15, 2025 at 05:55:31PM -0800, Charlie Jenkins wrote:
> > > On Mon, Jan 13, 2025 at 07:12:08PM +0200, Dmitry V. Levin wrote:
> > [...]
> > > > + /* Changing the type of the system call stop is not supported. */
> > > > + if (ptrace_get_syscall_info_op(child) != info.op)
> > >
> > > Since this isn't supported anyway, would it make sense to set the
> > > info.op to ptrace_get_syscall_info_op(child) like is done for
> > > get_syscall_info? The usecase I see for this is simplifying when the
> > > user doesn't call PTRACE_GET_SYSCALL_INFO before calling
> > > PTRACE_SET_SYSCALL_INFO.
> >
> > struct ptrace_syscall_info.op is a field that specifies how to interpret
> > the union fields of the structure, so if "op" is ignored, then the
> > kernel would infer the meaning of the structure specified by the userspace
> > tracer from the kernel state of the tracee. This looks a bit too
> > error-prone to allow. For example, nothing good is expected to happen
> > if syscall entry information is applied in a syscall exit stop.
>
> Yes that's a good point.
>
> >
> > The tracer is not obliged to call PTRACE_GET_SYSCALL_INFO to set
> > struct ptrace_syscall_info.op. If the tracer keeps track of ptrace stops
> > by other means, it can assign the right value by itself.
> >
> > And, btw, the comment should say "is not currently supported",
> > I'll update it in the next iteration.
> >
> > An idea mentioned in prior discussions was that it would make sense to
> > specify syscall return value along with skipping the syscall in seccomp stop,
> > and this would require a different value for "op" field, but
> > I decided not to introduce this extra complexity yet.
>
> Makes sense, thank you!
>
> - Charlie
I am no longer convinced that we need Celeste's patch that solves this
problem on riscv [1]. That patch is necessary without this change, but
PTRACE_SET_SYSCALL_INFO seems like a cleaner solution.
Reviewed-by: Charlie Jenkins <charlie@rivosinc.com>
Tested-by: Charlie Jenkins <charlie@rivosinc.com>
- Charlie
[1] https://lore.kernel.org/lkml/20250115-13cc73c36c7bb3b9f046f614@orel/T/
>
> >
> >
> > --
> > ldv
^ permalink raw reply
* Re: [PATCH v2 6/7] ptrace: introduce PTRACE_SET_SYSCALL_INFO request
From: Charlie Jenkins @ 2025-01-16 21:07 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Oleg Nesterov, Eugene Syromyatnikov, Mike Frysinger, Renzo Davoli,
Davide Berardi, strace-devel, linux-kernel, linux-api
In-Reply-To: <20250116083328.GA32173@strace.io>
On Thu, Jan 16, 2025 at 10:33:28AM +0200, Dmitry V. Levin wrote:
> On Wed, Jan 15, 2025 at 05:55:31PM -0800, Charlie Jenkins wrote:
> > On Mon, Jan 13, 2025 at 07:12:08PM +0200, Dmitry V. Levin wrote:
> [...]
> > > + /* Changing the type of the system call stop is not supported. */
> > > + if (ptrace_get_syscall_info_op(child) != info.op)
> >
> > Since this isn't supported anyway, would it make sense to set the
> > info.op to ptrace_get_syscall_info_op(child) like is done for
> > get_syscall_info? The usecase I see for this is simplifying when the
> > user doesn't call PTRACE_GET_SYSCALL_INFO before calling
> > PTRACE_SET_SYSCALL_INFO.
>
> struct ptrace_syscall_info.op is a field that specifies how to interpret
> the union fields of the structure, so if "op" is ignored, then the
> kernel would infer the meaning of the structure specified by the userspace
> tracer from the kernel state of the tracee. This looks a bit too
> error-prone to allow. For example, nothing good is expected to happen
> if syscall entry information is applied in a syscall exit stop.
Yes that's a good point.
>
> The tracer is not obliged to call PTRACE_GET_SYSCALL_INFO to set
> struct ptrace_syscall_info.op. If the tracer keeps track of ptrace stops
> by other means, it can assign the right value by itself.
>
> And, btw, the comment should say "is not currently supported",
> I'll update it in the next iteration.
>
> An idea mentioned in prior discussions was that it would make sense to
> specify syscall return value along with skipping the syscall in seccomp stop,
> and this would require a different value for "op" field, but
> I decided not to introduce this extra complexity yet.
Makes sense, thank you!
- Charlie
>
>
> --
> ldv
^ permalink raw reply
* Re: [PATCH 1/2] f2fs: register inodes which is able to donate pages
From: Jaegeuk Kim @ 2025-01-16 17:19 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-kernel, linux-f2fs-devel, Al Viro, Jan Kara,
Christian Brauner, linux-fsdevel, linux-api, linux-man
In-Reply-To: <Z4imEs-Se-VWcpBG@infradead.org>
On 01/15, Christoph Hellwig wrote:
> On Wed, Jan 15, 2025 at 10:16:51PM +0000, Jaegeuk Kim wrote:
> > This patch introduces an inode list to keep the page cache ranges that users
> > can donate pages together.
> >
> > #define F2FS_IOC_DONATE_RANGE _IOW(F2FS_IOCTL_MAGIC, 27, \
> > struct f2fs_donate_range)
> > struct f2fs_donate_range {
> > __u64 start;
> > __u64 len;
> > };
>
> > e.g., ioctl(F2FS_IOC_DONATE_RANGE, &range);
>
> This is not a very good description. "donate" here seems to basically
> mean a invalidate_inode_pages2_range. Which is a strange use of the
> word. what are the use cases? Why is this queued up to a thread and
> not done inline? Why is this in f2fs and not in common code.
The idea is let apps register some file ranges for page donation and admin
recliam such pages all togehter if they expect to see memory pressure soon.
We can rely on LRU, but this is more user-given trigger. I'm not sure whether
there's a need in general, hence, wanted to put it in f2fs first to get more
concrete use-cases beyond this Android case.
>
> I also which file systems wouldn't just add random fs specific ioctls
> all the time without any kinds of discussion of the API. f2fs is by
> far the worst offender there, but not the only one.
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Eyal Birger @ 2025-01-16 17:11 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Alexei Starovoitov, Jiri Olsa, Aleksa Sarai, Masami Hiramatsu,
linux-kernel, linux-trace-kernel, BPF-dev-list, Song Liu,
Yonghong Song, John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov, X86 ML, Linux API, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
rostedt@goodmis.org, rafi, Shmulik Ladkani
In-Reply-To: <20250116153044.GF21801@redhat.com>
On Thu, Jan 16, 2025 at 7:31 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> On 01/16, Eyal Birger wrote:
> >
> > Ack. I agree.
> >
> > Do you want to send a formal patch, or should I?
>
> Please send the patch ;)
Will do. If it's ok I'll put you in a "Suggested-by" tag (or
co-developed - as you see fit).
Eyal.
^ permalink raw reply
* Re: [PATCH v2 6/7] ptrace: introduce PTRACE_SET_SYSCALL_INFO request
From: Dmitry V. Levin @ 2025-01-16 16:40 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Eugene Syromyatnikov, Mike Frysinger, Renzo Davoli,
Davide Berardi, strace-devel, linux-kernel, linux-api
In-Reply-To: <20250116160403.GA3554@strace.io>
On Thu, Jan 16, 2025 at 06:04:03PM +0200, Dmitry V. Levin wrote:
> On Thu, Jan 16, 2025 at 04:21:38PM +0100, Oleg Nesterov wrote:
> > On 01/13, Dmitry V. Levin wrote:
> > >
> > > +static int
> > > +ptrace_set_syscall_info(struct task_struct *child, unsigned long user_size,
> > > + void __user *datavp)
> > > +{
> > > + struct pt_regs *regs = task_pt_regs(child);
> > > + struct ptrace_syscall_info info;
> > > + int error;
> > > +
> > > + BUILD_BUG_ON(sizeof(struct ptrace_syscall_info) < PTRACE_SYSCALL_INFO_SIZE_VER0);
> > > +
> > > + if (user_size < PTRACE_SYSCALL_INFO_SIZE_VER0 || user_size > PAGE_SIZE)
> > > + return -EINVAL;
> > > +
> > > + error = copy_struct_from_user(&info, sizeof(info), datavp, user_size);
> >
> > OK, I certainly can't understand why copy_struct_from_user/check_zeroed_user
> > is useful, at least in this case. In particular, this won't allow to run the
> > new code (which uses the "extended" ptrace_syscall_info) on the older kernels?
> >
> > Can't we just use user_size as a version number?
> >
> > We can also turn info->reserved into info->version filled by
> > ptrace_get_syscall_info().
> >
> > ptrace_set_syscall_info() can check that info->version matches user_size.
>
> The idea is to use "op" to specify the operation, and "flags" to specify
> future extensions to the operation. For example, we could later add
> PTRACE_SYSCALL_INFO_SECCOMP_SKIP operation to specify an exit-like
> data for seccomp stops, or some flag to set instruction_pointer or
> stack_pointer. I don't think any of these would require a version field,
> though.
>
> That is, the zero check implied by copy_struct_from_user() is not really
> needed here since the compatibility is tracked by "op" and "flags":
> if "op" and "flags" do not instruct the kernel to use these unknown
> extra bits, the kernel is not obliged to check them either.
> For the same reason I don't think the kernel is obliged to read more
> than sizeof(info) from userspace.
>
> What would you recommend using instead of copy_struct_from_user in this
> case?
Something like this?
if (user_size < PTRACE_SYSCALL_INFO_SIZE_VER0 || user_size > PAGE_SIZE)
return -EINVAL;
if (copy_from_user(&info, datavp, min(sizeof(info), user_size)))
return -EFAULT;
if (user_size < sizeof(info))
memset((void *)&info + user_size, 0, sizeof(info) - user_size);
--
ldv
^ permalink raw reply
* Re: [PATCH v2 6/7] ptrace: introduce PTRACE_SET_SYSCALL_INFO request
From: Dmitry V. Levin @ 2025-01-16 16:04 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Eugene Syromyatnikov, Mike Frysinger, Renzo Davoli,
Davide Berardi, strace-devel, linux-kernel, linux-api
In-Reply-To: <20250116152137.GE21801@redhat.com>
On Thu, Jan 16, 2025 at 04:21:38PM +0100, Oleg Nesterov wrote:
> On 01/13, Dmitry V. Levin wrote:
> >
> > +static int
> > +ptrace_set_syscall_info(struct task_struct *child, unsigned long user_size,
> > + void __user *datavp)
> > +{
> > + struct pt_regs *regs = task_pt_regs(child);
> > + struct ptrace_syscall_info info;
> > + int error;
> > +
> > + BUILD_BUG_ON(sizeof(struct ptrace_syscall_info) < PTRACE_SYSCALL_INFO_SIZE_VER0);
> > +
> > + if (user_size < PTRACE_SYSCALL_INFO_SIZE_VER0 || user_size > PAGE_SIZE)
> > + return -EINVAL;
> > +
> > + error = copy_struct_from_user(&info, sizeof(info), datavp, user_size);
>
> OK, I certainly can't understand why copy_struct_from_user/check_zeroed_user
> is useful, at least in this case. In particular, this won't allow to run the
> new code (which uses the "extended" ptrace_syscall_info) on the older kernels?
>
> Can't we just use user_size as a version number?
>
> We can also turn info->reserved into info->version filled by
> ptrace_get_syscall_info().
>
> ptrace_set_syscall_info() can check that info->version matches user_size.
The idea is to use "op" to specify the operation, and "flags" to specify
future extensions to the operation. For example, we could later add
PTRACE_SYSCALL_INFO_SECCOMP_SKIP operation to specify an exit-like
data for seccomp stops, or some flag to set instruction_pointer or
stack_pointer. I don't think any of these would require a version field,
though.
That is, the zero check implied by copy_struct_from_user() is not really
needed here since the compatibility is tracked by "op" and "flags":
if "op" and "flags" do not instruct the kernel to use these unknown
extra bits, the kernel is not obliged to check them either.
For the same reason I don't think the kernel is obliged to read more
than sizeof(info) from userspace.
What would you recommend using instead of copy_struct_from_user in this
case?
--
ldv
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Oleg Nesterov @ 2025-01-16 15:31 UTC (permalink / raw)
To: Eyal Birger
Cc: Alexei Starovoitov, Jiri Olsa, Aleksa Sarai, Masami Hiramatsu,
linux-kernel, linux-trace-kernel, BPF-dev-list, Song Liu,
Yonghong Song, John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov, X86 ML, Linux API, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
rostedt@goodmis.org, rafi, Shmulik Ladkani
In-Reply-To: <CAHsH6GukV+ydR+hw_-RF=0=_x6aO7xZzkCmbc53=Pk0Kv=8hUQ@mail.gmail.com>
On 01/16, Eyal Birger wrote:
>
> Ack. I agree.
>
> Do you want to send a formal patch, or should I?
Please send the patch ;)
Oleg.
^ permalink raw reply
* Re: [PATCH v2 6/7] ptrace: introduce PTRACE_SET_SYSCALL_INFO request
From: Oleg Nesterov @ 2025-01-16 15:21 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: <20250113171208.GF589@strace.io>
On 01/13, Dmitry V. Levin wrote:
>
> +static int
> +ptrace_set_syscall_info(struct task_struct *child, unsigned long user_size,
> + void __user *datavp)
> +{
> + struct pt_regs *regs = task_pt_regs(child);
> + struct ptrace_syscall_info info;
> + int error;
> +
> + BUILD_BUG_ON(sizeof(struct ptrace_syscall_info) < PTRACE_SYSCALL_INFO_SIZE_VER0);
> +
> + if (user_size < PTRACE_SYSCALL_INFO_SIZE_VER0 || user_size > PAGE_SIZE)
> + return -EINVAL;
> +
> + error = copy_struct_from_user(&info, sizeof(info), datavp, user_size);
OK, I certainly can't understand why copy_struct_from_user/check_zeroed_user
is useful, at least in this case. In particular, this won't allow to run the
new code (which uses the "extended" ptrace_syscall_info) on the older kernels?
Can't we just use user_size as a version number?
We can also turn info->reserved into info->version filled by
ptrace_get_syscall_info().
ptrace_set_syscall_info() can check that info->version matches user_size.
Oleg.
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Eyal Birger @ 2025-01-16 14:47 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Alexei Starovoitov, Jiri Olsa, Aleksa Sarai, Masami Hiramatsu,
linux-kernel, linux-trace-kernel, BPF-dev-list, Song Liu,
Yonghong Song, John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov, X86 ML, Linux API, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
rostedt@goodmis.org, rafi, Shmulik Ladkani
In-Reply-To: <20250116143956.GD21801@redhat.com>
On Thu, Jan 16, 2025 at 6:40 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> On 01/15, Eyal Birger wrote:
> >
> > On Wed, Jan 15, 2025 at 11:03 AM Oleg Nesterov <oleg@redhat.com> wrote:
> > >
> > > On 01/15, Eyal Birger wrote:
> > > >
> > > > --- a/kernel/seccomp.c
> > > > +++ b/kernel/seccomp.c
> > > > @@ -1359,6 +1359,9 @@ int __secure_computing(const struct seccomp_data *sd)
> > > > this_syscall = sd ? sd->nr :
> > > > syscall_get_nr(current, current_pt_regs());
> > > >
> > > > + if (this_syscall == __NR_uretprobe)
> > > > + return 0;
> > > > +
> > >
> > > Yes, this is what I meant. But we need the new arch-dependent helper.
> >
> > Do you mean because __NR_uretprobe is not defined for other architectures?
>
> Yes, and see below,
>
> > Is there an existing helper? I wasn't able to find one...
>
> No,
>
> > If not, would it just make sense to just wrap this check in
> > #ifdef __NR_uretprobe ?
>
> Given that we need a simple fix for -stable, I won't argue.
> Up to seccomp maintainers.
>
> But please note that this_syscall == __NR_uretprobe can be false
> positive if is_compat_task().
>
> __NR_uretprobe == __NR_ia32_rt_tgsigqueueinfo, so I guess we need
>
> #ifdef CONFIG_X86_64
> if (this_syscall == __NR_uretprobe && !in_ia32_syscall())
> return 0;
> #endif
>
> I don't think we need to worry about the X86_X32 tasks...
Ack. I agree.
Do you want to send a formal patch, or should I?
Eyal.
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Oleg Nesterov @ 2025-01-16 14:39 UTC (permalink / raw)
To: Eyal Birger
Cc: Alexei Starovoitov, Jiri Olsa, Aleksa Sarai, Masami Hiramatsu,
linux-kernel, linux-trace-kernel, BPF-dev-list, Song Liu,
Yonghong Song, John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov, X86 ML, Linux API, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
rostedt@goodmis.org, rafi, Shmulik Ladkani
In-Reply-To: <CAHsH6Gtd5kYPife3hK+uKafjBMx=-23UzvQgnOnqNDzSZgHyqw@mail.gmail.com>
On 01/15, Eyal Birger wrote:
>
> On Wed, Jan 15, 2025 at 11:03 AM Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > On 01/15, Eyal Birger wrote:
> > >
> > > --- a/kernel/seccomp.c
> > > +++ b/kernel/seccomp.c
> > > @@ -1359,6 +1359,9 @@ int __secure_computing(const struct seccomp_data *sd)
> > > this_syscall = sd ? sd->nr :
> > > syscall_get_nr(current, current_pt_regs());
> > >
> > > + if (this_syscall == __NR_uretprobe)
> > > + return 0;
> > > +
> >
> > Yes, this is what I meant. But we need the new arch-dependent helper.
>
> Do you mean because __NR_uretprobe is not defined for other architectures?
Yes, and see below,
> Is there an existing helper? I wasn't able to find one...
No,
> If not, would it just make sense to just wrap this check in
> #ifdef __NR_uretprobe ?
Given that we need a simple fix for -stable, I won't argue.
Up to seccomp maintainers.
But please note that this_syscall == __NR_uretprobe can be false
positive if is_compat_task().
__NR_uretprobe == __NR_ia32_rt_tgsigqueueinfo, so I guess we need
#ifdef CONFIG_X86_64
if (this_syscall == __NR_uretprobe && !in_ia32_syscall())
return 0;
#endif
I don't think we need to worry about the X86_X32 tasks...
Oleg.
^ permalink raw reply
* Re: [PATCH v2 6/7] ptrace: introduce PTRACE_SET_SYSCALL_INFO request
From: Dmitry V. Levin @ 2025-01-16 8:33 UTC (permalink / raw)
To: Charlie Jenkins
Cc: Oleg Nesterov, Eugene Syromyatnikov, Mike Frysinger, Renzo Davoli,
Davide Berardi, strace-devel, linux-kernel, linux-api
In-Reply-To: <Z4hnEzFUgN9N0pSF@ghost>
On Wed, Jan 15, 2025 at 05:55:31PM -0800, Charlie Jenkins wrote:
> On Mon, Jan 13, 2025 at 07:12:08PM +0200, Dmitry V. Levin wrote:
[...]
> > + /* Changing the type of the system call stop is not supported. */
> > + if (ptrace_get_syscall_info_op(child) != info.op)
>
> Since this isn't supported anyway, would it make sense to set the
> info.op to ptrace_get_syscall_info_op(child) like is done for
> get_syscall_info? The usecase I see for this is simplifying when the
> user doesn't call PTRACE_GET_SYSCALL_INFO before calling
> PTRACE_SET_SYSCALL_INFO.
struct ptrace_syscall_info.op is a field that specifies how to interpret
the union fields of the structure, so if "op" is ignored, then the
kernel would infer the meaning of the structure specified by the userspace
tracer from the kernel state of the tracee. This looks a bit too
error-prone to allow. For example, nothing good is expected to happen
if syscall entry information is applied in a syscall exit stop.
The tracer is not obliged to call PTRACE_GET_SYSCALL_INFO to set
struct ptrace_syscall_info.op. If the tracer keeps track of ptrace stops
by other means, it can assign the right value by itself.
And, btw, the comment should say "is not currently supported",
I'll update it in the next iteration.
An idea mentioned in prior discussions was that it would make sense to
specify syscall return value along with skipping the syscall in seccomp stop,
and this would require a different value for "op" field, but
I decided not to introduce this extra complexity yet.
--
ldv
^ permalink raw reply
* Re: [PATCH 1/2] f2fs: register inodes which is able to donate pages
From: Christoph Hellwig @ 2025-01-16 6:24 UTC (permalink / raw)
To: Jaegeuk Kim
Cc: linux-kernel, linux-f2fs-devel, Al Viro, Jan Kara,
Christian Brauner, linux-fsdevel, linux-api, linux-man
In-Reply-To: <20250115221814.1920703-2-jaegeuk@kernel.org>
On Wed, Jan 15, 2025 at 10:16:51PM +0000, Jaegeuk Kim wrote:
> This patch introduces an inode list to keep the page cache ranges that users
> can donate pages together.
>
> #define F2FS_IOC_DONATE_RANGE _IOW(F2FS_IOCTL_MAGIC, 27, \
> struct f2fs_donate_range)
> struct f2fs_donate_range {
> __u64 start;
> __u64 len;
> };
> e.g., ioctl(F2FS_IOC_DONATE_RANGE, &range);
This is not a very good description. "donate" here seems to basically
mean a invalidate_inode_pages2_range. Which is a strange use of the
word. what are the use cases? Why is this queued up to a thread and
not done inline? Why is this in f2fs and not in common code.
I also which file systems wouldn't just add random fs specific ioctls
all the time without any kinds of discussion of the API. f2fs is by
far the worst offender there, but not the only one.
^ permalink raw reply
* Re: [PATCH v2 6/7] ptrace: introduce PTRACE_SET_SYSCALL_INFO request
From: Charlie Jenkins @ 2025-01-16 1:55 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Oleg Nesterov, Eugene Syromyatnikov, Mike Frysinger, Renzo Davoli,
Davide Berardi, strace-devel, linux-kernel, linux-api
In-Reply-To: <20250113171208.GF589@strace.io>
On Mon, Jan 13, 2025 at 07:12:08PM +0200, Dmitry V. Levin wrote:
> PTRACE_SET_SYSCALL_INFO is a generic ptrace API that complements
> PTRACE_GET_SYSCALL_INFO by letting the ptracer modify details of
> system calls the tracee is blocked in.
>
> This API allows ptracers to obtain and modify system call details
> in a straightforward and architecture-agnostic way.
>
> Current implementation supports changing only those bits of system call
> information that are used by strace, namely, syscall number, syscall
> arguments, and syscall return value.
>
> Support of changing additional details returned by PTRACE_GET_SYSCALL_INFO,
> such as instruction pointer and stack pointer, could be added later
> if needed, by using struct ptrace_syscall_info.flags to specify
> the additional details that should be set. Currently, flags and reserved
> fields of struct ptrace_syscall_info must be initialized with zeroes;
> arch, instruction_pointer, and stack_pointer fields are ignored.
>
> PTRACE_SET_SYSCALL_INFO currently supports only PTRACE_SYSCALL_INFO_ENTRY,
> PTRACE_SYSCALL_INFO_EXIT, and PTRACE_SYSCALL_INFO_SECCOMP operations.
> Other operations could be added later if needed.
>
> Ideally, PTRACE_SET_SYSCALL_INFO should have been introduced along with
> PTRACE_GET_SYSCALL_INFO, but it didn't happen. The last straw that
> convinced me to implement PTRACE_SET_SYSCALL_INFO was apparent failure
> to provide an API of changing the first system call argument on riscv
> architecture.
>
> ptrace(2) man page:
>
> long ptrace(enum __ptrace_request request, pid_t pid, void *addr, void *data);
> ...
> PTRACE_SET_SYSCALL_INFO
> Modify information about the system call that caused the stop.
> The "data" argument is a pointer to struct ptrace_syscall_info
> that specifies the system call information to be set.
> The "addr" argument should be set to sizeof(struct ptrace_syscall_info)).
>
> Link: https://lore.kernel.org/all/59505464-c84a-403d-972f-d4b2055eeaac@gmail.com/
> Signed-off-by: Dmitry V. Levin <ldv@strace.io>
> ---
> include/linux/ptrace.h | 3 ++
> include/uapi/linux/ptrace.h | 4 +-
> kernel/ptrace.c | 95 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 101 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
> index 90507d4afcd6..c8dbf1e498bf 100644
> --- a/include/linux/ptrace.h
> +++ b/include/linux/ptrace.h
> @@ -17,6 +17,9 @@ struct syscall_info {
> struct seccomp_data data;
> };
>
> +/* sizeof() the first published struct ptrace_syscall_info */
> +#define PTRACE_SYSCALL_INFO_SIZE_VER0 84
> +
> extern int ptrace_access_vm(struct task_struct *tsk, unsigned long addr,
> void *buf, int len, unsigned int gup_flags);
>
> diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
> index 72c038fc71d0..ca75b3ab5d22 100644
> --- a/include/uapi/linux/ptrace.h
> +++ b/include/uapi/linux/ptrace.h
> @@ -74,6 +74,7 @@ struct seccomp_metadata {
> };
>
> #define PTRACE_GET_SYSCALL_INFO 0x420e
> +#define PTRACE_SET_SYSCALL_INFO 0x4212
> #define PTRACE_SYSCALL_INFO_NONE 0
> #define PTRACE_SYSCALL_INFO_ENTRY 1
> #define PTRACE_SYSCALL_INFO_EXIT 2
> @@ -81,7 +82,8 @@ struct seccomp_metadata {
>
> struct ptrace_syscall_info {
> __u8 op; /* PTRACE_SYSCALL_INFO_* */
> - __u8 pad[3];
> + __u8 reserved;
> + __u16 flags;
> __u32 arch;
> __u64 instruction_pointer;
> __u64 stack_pointer;
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 22e7d74cf4cd..41d37cb8f74a 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -1016,6 +1016,97 @@ ptrace_get_syscall_info(struct task_struct *child, unsigned long user_size,
> write_size = min(actual_size, user_size);
> return copy_to_user(datavp, &info, write_size) ? -EFAULT : actual_size;
> }
> +
> +static unsigned long
> +ptrace_set_syscall_info_entry(struct task_struct *child, struct pt_regs *regs,
> + struct ptrace_syscall_info *info)
> +{
> + unsigned long args[ARRAY_SIZE(info->entry.args)];
> + int nr = info->entry.nr;
> + int i;
> +
> + if (nr != info->entry.nr)
> + return -ERANGE;
> +
> + for (i = 0; i < ARRAY_SIZE(args); i++) {
> + args[i] = info->entry.args[i];
> + if (args[i] != info->entry.args[i])
> + return -ERANGE;
> + }
> +
> + syscall_set_nr(child, regs, nr);
> + /*
> + * If the syscall number is set to -1, setting syscall arguments is not
> + * just pointless, it would also clobber the syscall return value on
> + * those architectures that share the same register both for the first
> + * argument of syscall and its return value.
> + */
> + if (nr != -1)
> + syscall_set_arguments(child, regs, args);
> +
> + return 0;
> +}
> +
> +static unsigned long
> +ptrace_set_syscall_info_seccomp(struct task_struct *child, struct pt_regs *regs,
> + struct ptrace_syscall_info *info)
> +{
> + /*
> + * info->entry is currently a subset of info->seccomp,
> + * info->seccomp.ret_data is currently ignored.
> + */
> + return ptrace_set_syscall_info_entry(child, regs, info);
> +}
> +
> +static unsigned long
> +ptrace_set_syscall_info_exit(struct task_struct *child, struct pt_regs *regs,
> + struct ptrace_syscall_info *info)
> +{
> + if (info->exit.is_error)
> + syscall_set_return_value(child, regs, info->exit.rval, 0);
> + else
> + syscall_set_return_value(child, regs, 0, info->exit.rval);
> +
> + return 0;
> +}
> +
> +static int
> +ptrace_set_syscall_info(struct task_struct *child, unsigned long user_size,
> + void __user *datavp)
> +{
> + struct pt_regs *regs = task_pt_regs(child);
> + struct ptrace_syscall_info info;
> + int error;
> +
> + BUILD_BUG_ON(sizeof(struct ptrace_syscall_info) < PTRACE_SYSCALL_INFO_SIZE_VER0);
> +
> + if (user_size < PTRACE_SYSCALL_INFO_SIZE_VER0 || user_size > PAGE_SIZE)
> + return -EINVAL;
> +
> + error = copy_struct_from_user(&info, sizeof(info), datavp, user_size);
> + if (error)
> + return error;
> +
> + /* Reserved for future use. */
> + if (info.flags || info.reserved)
> + return -EINVAL;
> +
> + /* Changing the type of the system call stop is not supported. */
> + if (ptrace_get_syscall_info_op(child) != info.op)
Since this isn't supported anyway, would it make sense to set the
info.op to ptrace_get_syscall_info_op(child) like is done for
get_syscall_info? The usecase I see for this is simplifying when the
user doesn't call PTRACE_GET_SYSCALL_INFO before calling
PTRACE_SET_SYSCALL_INFO.
- Charlie
> + return -EINVAL;
> +
> + switch (info.op) {
> + case PTRACE_SYSCALL_INFO_ENTRY:
> + return ptrace_set_syscall_info_entry(child, regs, &info);
> + case PTRACE_SYSCALL_INFO_EXIT:
> + return ptrace_set_syscall_info_exit(child, regs, &info);
> + case PTRACE_SYSCALL_INFO_SECCOMP:
> + return ptrace_set_syscall_info_seccomp(child, regs, &info);
> + default:
> + /* Other types of system call stops are not supported. */
> + return -EINVAL;
> + }
> +}
> #endif /* CONFIG_HAVE_ARCH_TRACEHOOK */
>
> int ptrace_request(struct task_struct *child, long request,
> @@ -1234,6 +1325,10 @@ int ptrace_request(struct task_struct *child, long request,
> case PTRACE_GET_SYSCALL_INFO:
> ret = ptrace_get_syscall_info(child, addr, datavp);
> break;
> +
> + case PTRACE_SET_SYSCALL_INFO:
> + ret = ptrace_set_syscall_info(child, addr, datavp);
> + break;
> #endif
>
> case PTRACE_SECCOMP_GET_FILTER:
> --
> ldv
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Eyal Birger @ 2025-01-15 21:14 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Alexei Starovoitov, Jiri Olsa, Aleksa Sarai, Masami Hiramatsu,
linux-kernel, linux-trace-kernel, BPF-dev-list, Song Liu,
Yonghong Song, John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov, X86 ML, Linux API, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
rostedt@goodmis.org, rafi, Shmulik Ladkani
In-Reply-To: <20250115190304.GB21801@redhat.com>
On Wed, Jan 15, 2025 at 11:03 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> On 01/15, Eyal Birger wrote:
> >
> > --- a/kernel/seccomp.c
> > +++ b/kernel/seccomp.c
> > @@ -1359,6 +1359,9 @@ int __secure_computing(const struct seccomp_data *sd)
> > this_syscall = sd ? sd->nr :
> > syscall_get_nr(current, current_pt_regs());
> >
> > + if (this_syscall == __NR_uretprobe)
> > + return 0;
> > +
>
> Yes, this is what I meant. But we need the new arch-dependent helper.
Do you mean because __NR_uretprobe is not defined for other architectures?
Is there an existing helper? I wasn't able to find one...
If not, would it just make sense to just wrap this check in
#ifdef __NR_uretprobe ?
Eyal.
>
> Oleg.
>
^ permalink raw reply
* Re: [PATCH v2 6/7] ptrace: introduce PTRACE_SET_SYSCALL_INFO request
From: Oleg Nesterov @ 2025-01-15 19:10 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Alexey Gladkov, Eugene Syromyatnikov, Mike Frysinger,
Renzo Davoli, Davide Berardi, strace-devel, linux-kernel,
linux-api
In-Reply-To: <20250115173642.GA25129@strace.io>
On 01/15, Dmitry V. Levin wrote:
>
> On Wed, Jan 15, 2025 at 05:38:09PM +0100, Oleg Nesterov wrote:
> >
> > But may be
> >
> > if (syscall_get_nr() != -1)
> > syscall_set_arguments(...);
> >
> > will look a bit more consistent?
>
> I'm sorry, but I didn't follow. As we've just set the syscall number with
> syscall_set_nr(), why would we want to call syscall_get_nr() right after
> that to obtain the syscall number?
Mostly for grep. We have more syscall_get_nr() != -1 checks. Even right after
syscall_set_nr-like code, see putreg32().
I think this needs another helper (which can have more users) and some cleanups.
But this is another issue, so please forget. I agree that syscall_get_nr() in
this code will probably just add the unnecessary confusion.
Oleg.
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Oleg Nesterov @ 2025-01-15 19:03 UTC (permalink / raw)
To: Eyal Birger
Cc: Alexei Starovoitov, Jiri Olsa, Aleksa Sarai, Masami Hiramatsu,
linux-kernel, linux-trace-kernel, BPF-dev-list, Song Liu,
Yonghong Song, John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov, X86 ML, Linux API, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
rostedt@goodmis.org, rafi, Shmulik Ladkani
In-Reply-To: <CAHsH6Gu1kXZ=m3eoTeZcZ9n=n2scxw7z074PnY5oTsXfTqZ=vQ@mail.gmail.com>
On 01/15, Eyal Birger wrote:
>
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -1359,6 +1359,9 @@ int __secure_computing(const struct seccomp_data *sd)
> this_syscall = sd ? sd->nr :
> syscall_get_nr(current, current_pt_regs());
>
> + if (this_syscall == __NR_uretprobe)
> + return 0;
> +
Yes, this is what I meant. But we need the new arch-dependent helper.
Oleg.
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Eyal Birger @ 2025-01-15 18:48 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Alexei Starovoitov, Jiri Olsa, Aleksa Sarai, Masami Hiramatsu,
linux-kernel, linux-trace-kernel, BPF-dev-list, Song Liu,
Yonghong Song, John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov, X86 ML, Linux API, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
rostedt@goodmis.org, rafi, Shmulik Ladkani
In-Reply-To: <20250115184011.GA21801@redhat.com>
On Wed, Jan 15, 2025 at 10:40 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> On 01/15, Alexei Starovoitov wrote:
> >
> > On Wed, Jan 15, 2025 at 7:06 AM Oleg Nesterov <oleg@redhat.com> wrote:
> > >
> > > Or we can change __secure_computing() to do nothing if
> > > this_syscall == __NR_uretprobe.
> >
> > I think that's the best way forward.
> > seccomp already allowlists sigreturn syscall.
>
> Only if SECCOMP_MODE_STRICT. But it won't help if we add __NR_uretprobe
> into into mode1_syscalls/mode1_syscalls_32.
>
> SECCOMP_MODE_FILTER can do anything. Just I guess nobody tries to offend
> sigreturn for obvious reasons.
>
> But yes, perhaps we do not have a better solution.
>
Indeed - doing the check in __secure_computing_strict() doesn't seem to be
enough.
In __secure_computing(), i.e. the below hack it works.
Eyal.
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 385d48293a5f..5739482036ce 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -1359,6 +1359,9 @@ int __secure_computing(const struct seccomp_data *sd)
this_syscall = sd ? sd->nr :
syscall_get_nr(current, current_pt_regs());
+ if (this_syscall == __NR_uretprobe)
+ return 0;
+
switch (mode) {
case SECCOMP_MODE_STRICT:
__secure_computing_strict(this_syscall); /* may call do_exit */
^ permalink raw reply related
* Re: Crash when attaching uretprobes to processes running in Docker
From: Oleg Nesterov @ 2025-01-15 18:40 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Jiri Olsa, Eyal Birger, Aleksa Sarai, Masami Hiramatsu,
linux-kernel, linux-trace-kernel, BPF-dev-list, Song Liu,
Yonghong Song, John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov, X86 ML, Linux API, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
rostedt@goodmis.org, rafi, Shmulik Ladkani
In-Reply-To: <CAADnVQJjroiR0SRp69f1NbomEH-riw53e_-TioqT4aEt3GSKGg@mail.gmail.com>
On 01/15, Alexei Starovoitov wrote:
>
> On Wed, Jan 15, 2025 at 7:06 AM Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > Or we can change __secure_computing() to do nothing if
> > this_syscall == __NR_uretprobe.
>
> I think that's the best way forward.
> seccomp already allowlists sigreturn syscall.
Only if SECCOMP_MODE_STRICT. But it won't help if we add __NR_uretprobe
into into mode1_syscalls/mode1_syscalls_32.
SECCOMP_MODE_FILTER can do anything. Just I guess nobody tries to offend
sigreturn for obvious reasons.
But yes, perhaps we do not have a better solution.
Oleg.
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Andrii Nakryiko @ 2025-01-15 18:20 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Oleg Nesterov, Jiri Olsa, Eyal Birger, Aleksa Sarai,
Masami Hiramatsu, linux-kernel, linux-trace-kernel, BPF-dev-list,
Song Liu, Yonghong Song, John Fastabend, Peter Zijlstra,
Thomas Gleixner, Borislav Petkov, X86 ML, Linux API,
Andrii Nakryiko, Daniel Borkmann, Alexei Starovoitov,
rostedt@goodmis.org, rafi, Shmulik Ladkani
In-Reply-To: <CAADnVQJjroiR0SRp69f1NbomEH-riw53e_-TioqT4aEt3GSKGg@mail.gmail.com>
On Wed, Jan 15, 2025 at 9:56 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, Jan 15, 2025 at 7:06 AM Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > Or we can change __secure_computing() to do nothing if
> > this_syscall == __NR_uretprobe.
>
> I think that's the best way forward.
> seccomp already allowlists sigreturn syscall.
> uretprobe syscall is in the same category.
+1, we will have a similar problem with sys_uprobe (when it's added).
Just like rt_sigreturn, these are special kernel-only mechanisms, and
the kernel already protects itself from any user abuse. So I think we
should have a way to ensure those special syscalls can go through
regardless of seccomp.
> See __secure_computing_strict.
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Alexei Starovoitov @ 2025-01-15 17:56 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Jiri Olsa, Eyal Birger, Aleksa Sarai, Masami Hiramatsu,
linux-kernel, linux-trace-kernel, BPF-dev-list, Song Liu,
Yonghong Song, John Fastabend, Peter Zijlstra, Thomas Gleixner,
Borislav Petkov, X86 ML, Linux API, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
rostedt@goodmis.org, rafi, Shmulik Ladkani
In-Reply-To: <20250115150607.GA11980@redhat.com>
On Wed, Jan 15, 2025 at 7:06 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> Or we can change __secure_computing() to do nothing if
> this_syscall == __NR_uretprobe.
I think that's the best way forward.
seccomp already allowlists sigreturn syscall.
uretprobe syscall is in the same category.
See __secure_computing_strict.
^ permalink raw reply
* Re: [PATCH v2 6/7] ptrace: introduce PTRACE_SET_SYSCALL_INFO request
From: Dmitry V. Levin @ 2025-01-15 17:36 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Alexey Gladkov, Eugene Syromyatnikov, Mike Frysinger,
Renzo Davoli, Davide Berardi, strace-devel, linux-kernel,
linux-api
In-Reply-To: <20250115163809.GC11980@redhat.com>
On Wed, Jan 15, 2025 at 05:38:09PM +0100, Oleg Nesterov wrote:
[...]
> > + syscall_set_nr(child, regs, nr);
> > + /*
> > + * If the syscall number is set to -1, setting syscall arguments is not
> > + * just pointless, it would also clobber the syscall return value on
> > + * those architectures that share the same register both for the first
> > + * argument of syscall and its return value.
> > + */
> > + if (nr != -1)
> > + syscall_set_arguments(child, regs, args);
>
> Thanks, much better than I tried to suggest in my reply to V1.
>
> But may be
>
> if (syscall_get_nr() != -1)
> syscall_set_arguments(...);
>
> will look a bit more consistent?
I'm sorry, but I didn't follow. As we've just set the syscall number with
syscall_set_nr(), why would we want to call syscall_get_nr() right after
that to obtain the syscall number?
--
ldv
^ permalink raw reply
* Re: [PATCH v2 6/7] ptrace: introduce PTRACE_SET_SYSCALL_INFO request
From: Oleg Nesterov @ 2025-01-15 16:38 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: <20250113171208.GF589@strace.io>
Dmitry,
I can't review the non-x86 changes in 1/7 - 4/7.
As for this and the previous patch I see nothing bad after a quick glance.
Just I have some concerns about the "future extensions", I'll write another
email tomorrow. In particualar, I personally hate the very idea of
copy_struct_from_user/check_zeroed_user ;)
On 01/13, Dmitry V. Levin wrote:
>
> +ptrace_set_syscall_info_entry(struct task_struct *child, struct pt_regs *regs,
> + struct ptrace_syscall_info *info)
> +{
> + unsigned long args[ARRAY_SIZE(info->entry.args)];
> + int nr = info->entry.nr;
> + int i;
> +
> + if (nr != info->entry.nr)
> + return -ERANGE;
> +
> + for (i = 0; i < ARRAY_SIZE(args); i++) {
> + args[i] = info->entry.args[i];
> + if (args[i] != info->entry.args[i])
> + return -ERANGE;
> + }
> +
> + syscall_set_nr(child, regs, nr);
> + /*
> + * If the syscall number is set to -1, setting syscall arguments is not
> + * just pointless, it would also clobber the syscall return value on
> + * those architectures that share the same register both for the first
> + * argument of syscall and its return value.
> + */
> + if (nr != -1)
> + syscall_set_arguments(child, regs, args);
Thanks, much better than I tried to suggest in my reply to V1.
But may be
if (syscall_get_nr() != -1)
syscall_set_arguments(...);
will look a bit more consistent?
Oleg.
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Oleg Nesterov @ 2025-01-15 15:51 UTC (permalink / raw)
To: Shmulik Ladkani
Cc: Eyal Birger, Andrii Nakryiko, Jiri Olsa, Sarai Aleksa, mhiramat,
linux-kernel, linux-trace-kernel, BPF-dev-list, Song Liu,
Yonghong Song, John Fastabend, peterz, tglx, bp, x86, linux-api,
Andrii Nakryiko, Daniel Borkmann, Alexei Starovoitov, rostedt,
rafi
In-Reply-To: <67874b84.7b0a0220.3935f4.1f48@mx.google.com>
On 01/15, Shmulik Ladkani wrote:
>
> IMO There are 2 problematic aspects with ff474a78cef5
> ("uprobe: Add uretprobe syscall to speed up return probe").
>
> The first, as Eyal mentioned, is the kernel regression: There are
> endless systems out there (iaas and paas) that have both
> telementry/instrumentation/tracing software (utilizing uprobes) and
> container environments (duch as docker) that enforce syscall
> restrictions on their workloads.
> These systems worked so far, and with kernels having ff474a78cef5 the
> workloads processes fault.
Again, I have to agree. The kernel should not break userspace.
But,
> The second, is the fact that ff474a78cef5 (which adds a new syscall
> invocation to the uretprobe trampoline) *exposes an internal kernel
> implementation* to the userspace system:
I disagree...
> There are millions of binaries/libraries out there that *never issue*
> the new syscall: they simply do not have that call in their
> instructions. Take for example hello-world.
And they should never use this syscall,
> However, once hello-world is traced (with software utilizing
> uprobes) hello-world *unknowingly* DO issue the new syscall, just
> because the kernel decided to implement its uretprobe trampoline using
> a new syscall - a mechanism that should be completely transparent and
> seamless to the user program.
IMO, sys_uretprobe() doesn't really differ from sys_sigreturn() in this
respect.
> This is totally unexpected, and to ask a system admin to "guess" whether
> hello-world is "going to issue the syscall despite the fact that
> such invocation does not exist in its own code at all" (and set seccomp
> permissions accordingly) is asking for the admin to know the exact
> *internal mechanisms* that the kernel use for implemeting the
> trampolines.
Well, man 2 uretprobe can help ;)
> we
> shoudn't add any instruction (such as a syscall) that isn't *completely
> transparent* to the userspace program.
We can't make it *completely transparent*, but it is easy to hide this
syscall from seccomp (and/or ptrace).
And this will fix the problem. But I don't feel this is the right solution.
Oleg.
^ permalink raw reply
* Re: [PATCH v23 7/8] samples/check-exec: Add an enlighten "inc" interpreter and 28 tests
From: Mickaël Salaün @ 2025-01-15 15:24 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Al Viro, Christian Brauner, Kees Cook, Paul Moore, Serge Hallyn,
Adhemerval Zanella Netto, Alejandro Colomar, Aleksa Sarai,
Andrew Morton, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Christian Heimes, Dmitry Vyukov, Elliott Hughes, Eric Biggers,
Eric Chiang, Fan Wu, Florian Weimer, Geert Uytterhoeven,
James Morris, Jan Kara, Jann Horn, Jeff Xu, Jonathan Corbet,
Jordan R Abrahams, Lakshmi Ramasubramanian, Linus Torvalds,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Roberto Sassu, Scott Shell,
Shuah Khan, Shuah Khan, Stephen Rothwell, Steve Dower,
Steve Grubb, Theodore Ts'o, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, kernel-hardening, linux-api,
linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20250114205645.GA2825031@ax162>
On Tue, Jan 14, 2025 at 01:56:45PM -0700, Nathan Chancellor wrote:
> Hi Mickaël,
>
> On Thu, Dec 12, 2024 at 06:42:22PM +0100, Mickaël Salaün wrote:
> > Add a very simple script interpreter called "inc" that can evaluate two
> > different commands (one per line):
> > - "?" to initialize a counter from user's input;
> > - "+" to increment the counter (which is set to 0 by default).
> >
> > It is enlighten to only interpret executable files according to
> > AT_EXECVE_CHECK and the related securebits:
> >
> > # Executing a script with RESTRICT_FILE is only allowed if the script
> > # is executable:
> > ./set-exec -f -- ./inc script-exec.inc # Allowed
> > ./set-exec -f -- ./inc script-noexec.inc # Denied
> >
> > # Executing stdin with DENY_INTERACTIVE is only allowed if stdin is an
> > # executable regular file:
> > ./set-exec -i -- ./inc -i < script-exec.inc # Allowed
> > ./set-exec -i -- ./inc -i < script-noexec.inc # Denied
> >
> > # However, a pipe is not executable and it is then denied:
> > cat script-noexec.inc | ./set-exec -i -- ./inc -i # Denied
> >
> > # Executing raw data (e.g. command argument) with DENY_INTERACTIVE is
> > # always denied.
> > ./set-exec -i -- ./inc -c "+" # Denied
> > ./inc -c "$(<script-ask.inc)" # Allowed
> >
> > # To directly execute a script, we can update $PATH (used by `env`):
> > PATH="${PATH}:." ./script-exec.inc
> >
> > # To execute several commands passed as argument:
> >
> > Add a complete test suite to check the script interpreter against all
> > possible execution cases:
> >
> > make TARGETS=exec kselftest-install
> > ./tools/testing/selftests/kselftest_install/run_kselftest.sh
> >
> > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > Cc: Christian Brauner <brauner@kernel.org>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Paul Moore <paul@paul-moore.com>
> > Cc: Serge Hallyn <serge@hallyn.com>
> > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > Link: https://lore.kernel.org/r/20241212174223.389435-8-mic@digikod.net
> ...
> > diff --git a/samples/check-exec/inc.c b/samples/check-exec/inc.c
> > new file mode 100644
> > index 000000000000..94b87569d2a2
> > --- /dev/null
> > +++ b/samples/check-exec/inc.c
> ...
> > +/* Returns 1 on error, 0 otherwise. */
> > +static int interpret_stream(FILE *script, char *const script_name,
> > + char *const *const envp, const bool restrict_stream)
> > +{
> > + int err;
> > + char *const script_argv[] = { script_name, NULL };
> > + char buf[128] = {};
> > + size_t buf_size = sizeof(buf);
> > +
> > + /*
> > + * We pass a valid argv and envp to the kernel to emulate a native
> > + * script execution. We must use the script file descriptor instead of
> > + * the script path name to avoid race conditions.
> > + */
> > + err = execveat(fileno(script), "", script_argv, envp,
> > + AT_EMPTY_PATH | AT_EXECVE_CHECK);
> > + if (err && restrict_stream) {
> > + perror("ERROR: Script execution check");
> > + return 1;
> > + }
> > +
> > + /* Reads script. */
> > + buf_size = fread(buf, 1, buf_size - 1, script);
> > + return interpret_buffer(buf, buf_size);
> > +}
>
> The use of execveat() in this test case breaks the build when glibc is
> less than 2.34, as that is the earliest version that has the execveat()
> wrapper:
>
> https://sourceware.org/git/?p=glibc.git;a=commit;h=19d83270fcd993cc349570164e21b06d57036704
>
> $ ldd --version | head -1
> ldd (Debian GLIBC 2.31-13+deb11u11) 2.31
>
> $ make -skj"$(nproc)" ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- mrproper allmodconfig samples/
> ...
> samples/check-exec/inc.c:81:8: error: call to undeclared function 'execveat'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
> 81 | err = execveat(fileno(script), "", script_argv, envp,
> | ^
> samples/check-exec/inc.c:81:8: note: did you mean 'execve'?
> /usr/include/unistd.h:551:12: note: 'execve' declared here
> 551 | extern int execve (const char *__path, char *const __argv[],
> | ^
> 1 error generated.
> ...
>
> Should this just use the syscall directly?
Thanks for the report, I sent a fix:
https://lore.kernel.org/r/20250115144753.311152-1-mic@digikod.net
>
> Cheers,
> Nathan
^ permalink raw reply
* Re: Crash when attaching uretprobes to processes running in Docker
From: Oleg Nesterov @ 2025-01-15 15:06 UTC (permalink / raw)
To: Jiri Olsa
Cc: Eyal Birger, Aleksa Sarai, mhiramat, linux-kernel,
linux-trace-kernel, BPF-dev-list, Song Liu, Yonghong Song,
John Fastabend, peterz, tglx, bp, x86, linux-api, Andrii Nakryiko,
Daniel Borkmann, Alexei Starovoitov, Andrii Nakryiko,
rostedt@goodmis.org, rafi, Shmulik Ladkani
In-Reply-To: <Z4eBs0-kJ3iVZjXL@krava>
On 01/15, Jiri Olsa wrote:
>
> On Tue, Jan 14, 2025 at 06:25:20PM +0100, Oleg Nesterov wrote:
> >
> > Sorry, I don't understand... What exactly we can do? Aside from checking
> > IS_ENABLED(CONFIG_SECCOMP) in arch_uprobe_trampoline() ?
>
> I need to check more on seccomp, but I imagine we could do following:
> - when seccomp filter is installed we could check uprobe trampoline
> and if it's already installed we change it to int3 trampoline
> - when uprobe trampoline is getting installed we check if there's
> seccomp filter installed for task and we use int3 trampoline
I still don't understand... But whatever you meant, I doubt it can work.
> other than that I guess we will have to add sysctl to enable uretprobe
> trampoline..
Or we can change __secure_computing() to do nothing if
this_syscall == __NR_uretprobe. Or even change syscall_trace_enter/exit
to do this check.
But I don't really like this idea, I don't feel this is the right solution...
Oleg.
^ permalink raw reply
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