linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] x86: Include asm/ptrace.h in syscall_wrapper header
@ 2022-10-18 12:27 Jiri Olsa
  2022-10-18 18:23 ` sdf
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Jiri Olsa @ 2022-10-18 12:27 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Peter Zijlstra, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, Andy Lutomirski
  Cc: Akihiro HARAI, bpf, linux-kernel, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
	Hao Luo

From: Jiri Olsa <olsajiri@gmail.com>

With just the forward declaration of the 'struct pt_regs' in
syscall_wrapper.h, the syscall stub functions:

  __[x64|ia32]_sys_*(struct pt_regs *regs)

will have different definition of 'regs' argument in BTF data
based on which object file they are defined in.

If the syscall's object includes 'struct pt_regs' definition,
the BTF argument data will point to 'struct pt_regs' record,
like:

  [226] STRUCT 'pt_regs' size=168 vlen=21
         'r15' type_id=1 bits_offset=0
         'r14' type_id=1 bits_offset=64
         'r13' type_id=1 bits_offset=128
  ...

If not, it will point to fwd declaration record:

  [15439] FWD 'pt_regs' fwd_kind=struct

and make bpf tracing program hooking on those functions unable
to access fields from 'struct pt_regs'.

Including asm/ptrace.h directly in syscall_wrapper.h to make
sure all syscalls see 'struct pt_regs' definition and resulted
BTF for '__*_sys_*(struct pt_regs *regs)' functions point to
actual struct, not just forward declaration.

Reported-by: Akihiro HARAI <jharai0815@gmail.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 arch/x86/include/asm/syscall_wrapper.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/syscall_wrapper.h b/arch/x86/include/asm/syscall_wrapper.h
index 59358d1bf880..fd2669b1cb2d 100644
--- a/arch/x86/include/asm/syscall_wrapper.h
+++ b/arch/x86/include/asm/syscall_wrapper.h
@@ -6,7 +6,7 @@
 #ifndef _ASM_X86_SYSCALL_WRAPPER_H
 #define _ASM_X86_SYSCALL_WRAPPER_H
 
-struct pt_regs;
+#include <asm/ptrace.h>
 
 extern long __x64_sys_ni_syscall(const struct pt_regs *regs);
 extern long __ia32_sys_ni_syscall(const struct pt_regs *regs);
-- 
2.37.3


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] x86: Include asm/ptrace.h in syscall_wrapper header
  2022-10-18 12:27 [PATCH] x86: Include asm/ptrace.h in syscall_wrapper header Jiri Olsa
@ 2022-10-18 18:23 ` sdf
  2022-10-19  9:30   ` Jiri Olsa
  2022-10-24 15:26 ` Lorenz Bauer
  2022-10-24 17:10 ` [tip: x86/urgent] x86/syscall: " tip-bot2 for Jiri Olsa
  2 siblings, 1 reply; 6+ messages in thread
From: sdf @ 2022-10-18 18:23 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Peter Zijlstra, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, Andy Lutomirski, Akihiro HARAI, bpf, linux-kernel,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Hao Luo

On 10/18, Jiri Olsa wrote:
> From: Jiri Olsa <olsajiri@gmail.com>

> With just the forward declaration of the 'struct pt_regs' in
> syscall_wrapper.h, the syscall stub functions:

>    __[x64|ia32]_sys_*(struct pt_regs *regs)

> will have different definition of 'regs' argument in BTF data
> based on which object file they are defined in.

> If the syscall's object includes 'struct pt_regs' definition,
> the BTF argument data will point to 'struct pt_regs' record,
> like:

>    [226] STRUCT 'pt_regs' size=168 vlen=21
>           'r15' type_id=1 bits_offset=0
>           'r14' type_id=1 bits_offset=64
>           'r13' type_id=1 bits_offset=128
>    ...

> If not, it will point to fwd declaration record:

>    [15439] FWD 'pt_regs' fwd_kind=struct

> and make bpf tracing program hooking on those functions unable
> to access fields from 'struct pt_regs'.

Is the core issue here is that we can't / don't resolve FWD declarations
at load time (or dedup time)? I'm assuming 'struct pt_regs' is still
exposed somewhere in BTF via some other obj file, it's just those
syscall definitions that have FWD decl?

Applying this patch seems fine for now, but also seems fragile. Should
we instead/also teach verifier/dedup/whatever to resolve those FWD
declarations?

> Including asm/ptrace.h directly in syscall_wrapper.h to make
> sure all syscalls see 'struct pt_regs' definition and resulted
> BTF for '__*_sys_*(struct pt_regs *regs)' functions point to
> actual struct, not just forward declaration.

> Reported-by: Akihiro HARAI <jharai0815@gmail.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>   arch/x86/include/asm/syscall_wrapper.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

> diff --git a/arch/x86/include/asm/syscall_wrapper.h  
> b/arch/x86/include/asm/syscall_wrapper.h
> index 59358d1bf880..fd2669b1cb2d 100644
> --- a/arch/x86/include/asm/syscall_wrapper.h
> +++ b/arch/x86/include/asm/syscall_wrapper.h
> @@ -6,7 +6,7 @@
>   #ifndef _ASM_X86_SYSCALL_WRAPPER_H
>   #define _ASM_X86_SYSCALL_WRAPPER_H

> -struct pt_regs;
> +#include <asm/ptrace.h>

>   extern long __x64_sys_ni_syscall(const struct pt_regs *regs);
>   extern long __ia32_sys_ni_syscall(const struct pt_regs *regs);
> --
> 2.37.3


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] x86: Include asm/ptrace.h in syscall_wrapper header
  2022-10-18 18:23 ` sdf
@ 2022-10-19  9:30   ` Jiri Olsa
  2022-10-21 21:49     ` Andrii Nakryiko
  0 siblings, 1 reply; 6+ messages in thread
From: Jiri Olsa @ 2022-10-19  9:30 UTC (permalink / raw)
  To: sdf
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Peter Zijlstra, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, Andy Lutomirski, Akihiro HARAI, bpf, linux-kernel,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Hao Luo

On Tue, Oct 18, 2022 at 11:23:21AM -0700, sdf@google.com wrote:
> On 10/18, Jiri Olsa wrote:
> > From: Jiri Olsa <olsajiri@gmail.com>
> 
> > With just the forward declaration of the 'struct pt_regs' in
> > syscall_wrapper.h, the syscall stub functions:
> 
> >    __[x64|ia32]_sys_*(struct pt_regs *regs)
> 
> > will have different definition of 'regs' argument in BTF data
> > based on which object file they are defined in.
> 
> > If the syscall's object includes 'struct pt_regs' definition,
> > the BTF argument data will point to 'struct pt_regs' record,
> > like:
> 
> >    [226] STRUCT 'pt_regs' size=168 vlen=21
> >           'r15' type_id=1 bits_offset=0
> >           'r14' type_id=1 bits_offset=64
> >           'r13' type_id=1 bits_offset=128
> >    ...
> 
> > If not, it will point to fwd declaration record:
> 
> >    [15439] FWD 'pt_regs' fwd_kind=struct
> 
> > and make bpf tracing program hooking on those functions unable
> > to access fields from 'struct pt_regs'.
> 
> Is the core issue here is that we can't / don't resolve FWD declarations
> at load time (or dedup time)? I'm assuming 'struct pt_regs' is still
> exposed somewhere in BTF via some other obj file, it's just those
> syscall definitions that have FWD decl?

yes, BTF is generated from dwarf debug info, so it's object based,
and in some objects the regs argument will point to full struct
definition and in some just to forward declaration

> 
> Applying this patch seems fine for now, but also seems fragile. Should
> we instead/also teach verifier/dedup/whatever to resolve those FWD
> declarations?

I'm not sure how hard it'd be to connect forward declarations
to definitions.. it'd need to be probably in dedup or verifier
as you suggest

and I think it'd need to be done just based on struct name search,
so I expect all sort of unexpected problems ;-) other than to be
sure you connect to proper struct

Andrii will have probably better idea if and where this is possible

jirka

> 
> > Including asm/ptrace.h directly in syscall_wrapper.h to make
> > sure all syscalls see 'struct pt_regs' definition and resulted
> > BTF for '__*_sys_*(struct pt_regs *regs)' functions point to
> > actual struct, not just forward declaration.
> 
> > Reported-by: Akihiro HARAI <jharai0815@gmail.com>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >   arch/x86/include/asm/syscall_wrapper.h | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> > diff --git a/arch/x86/include/asm/syscall_wrapper.h
> > b/arch/x86/include/asm/syscall_wrapper.h
> > index 59358d1bf880..fd2669b1cb2d 100644
> > --- a/arch/x86/include/asm/syscall_wrapper.h
> > +++ b/arch/x86/include/asm/syscall_wrapper.h
> > @@ -6,7 +6,7 @@
> >   #ifndef _ASM_X86_SYSCALL_WRAPPER_H
> >   #define _ASM_X86_SYSCALL_WRAPPER_H
> 
> > -struct pt_regs;
> > +#include <asm/ptrace.h>
> 
> >   extern long __x64_sys_ni_syscall(const struct pt_regs *regs);
> >   extern long __ia32_sys_ni_syscall(const struct pt_regs *regs);
> > --
> > 2.37.3
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] x86: Include asm/ptrace.h in syscall_wrapper header
  2022-10-19  9:30   ` Jiri Olsa
@ 2022-10-21 21:49     ` Andrii Nakryiko
  0 siblings, 0 replies; 6+ messages in thread
From: Andrii Nakryiko @ 2022-10-21 21:49 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: sdf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Peter Zijlstra, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, Andy Lutomirski, Akihiro HARAI, bpf, linux-kernel,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Hao Luo

On Wed, Oct 19, 2022 at 2:30 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Tue, Oct 18, 2022 at 11:23:21AM -0700, sdf@google.com wrote:
> > On 10/18, Jiri Olsa wrote:
> > > From: Jiri Olsa <olsajiri@gmail.com>
> >
> > > With just the forward declaration of the 'struct pt_regs' in
> > > syscall_wrapper.h, the syscall stub functions:
> >
> > >    __[x64|ia32]_sys_*(struct pt_regs *regs)
> >
> > > will have different definition of 'regs' argument in BTF data
> > > based on which object file they are defined in.
> >
> > > If the syscall's object includes 'struct pt_regs' definition,
> > > the BTF argument data will point to 'struct pt_regs' record,
> > > like:
> >
> > >    [226] STRUCT 'pt_regs' size=168 vlen=21
> > >           'r15' type_id=1 bits_offset=0
> > >           'r14' type_id=1 bits_offset=64
> > >           'r13' type_id=1 bits_offset=128
> > >    ...
> >
> > > If not, it will point to fwd declaration record:
> >
> > >    [15439] FWD 'pt_regs' fwd_kind=struct
> >
> > > and make bpf tracing program hooking on those functions unable
> > > to access fields from 'struct pt_regs'.
> >
> > Is the core issue here is that we can't / don't resolve FWD declarations
> > at load time (or dedup time)? I'm assuming 'struct pt_regs' is still
> > exposed somewhere in BTF via some other obj file, it's just those
> > syscall definitions that have FWD decl?
>
> yes, BTF is generated from dwarf debug info, so it's object based,
> and in some objects the regs argument will point to full struct
> definition and in some just to forward declaration
>
> >
> > Applying this patch seems fine for now, but also seems fragile. Should
> > we instead/also teach verifier/dedup/whatever to resolve those FWD
> > declarations?
>
> I'm not sure how hard it'd be to connect forward declarations
> to definitions.. it'd need to be probably in dedup or verifier
> as you suggest
>
> and I think it'd need to be done just based on struct name search,
> so I expect all sort of unexpected problems ;-) other than to be
> sure you connect to proper struct

exactly, which is why BTF dedup algorithm resolves FWD to STRUCT/UNION
only when we have to outer structs/unions that we are deduplicating,
and one of their field points to FWD on one side and STRUCT on another
side. Looking up by name is ambiguous and could be incorrect, so BTF
dedup needs sort of a "structural proof".

As far as the patch goes, it's a good thing to have more complete
types wherever possible, so:

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>
> Andrii will have probably better idea if and where this is possible
>
> jirka
>
> >
> > > Including asm/ptrace.h directly in syscall_wrapper.h to make
> > > sure all syscalls see 'struct pt_regs' definition and resulted
> > > BTF for '__*_sys_*(struct pt_regs *regs)' functions point to
> > > actual struct, not just forward declaration.
> >
> > > Reported-by: Akihiro HARAI <jharai0815@gmail.com>
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >   arch/x86/include/asm/syscall_wrapper.h | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > > diff --git a/arch/x86/include/asm/syscall_wrapper.h
> > > b/arch/x86/include/asm/syscall_wrapper.h
> > > index 59358d1bf880..fd2669b1cb2d 100644
> > > --- a/arch/x86/include/asm/syscall_wrapper.h
> > > +++ b/arch/x86/include/asm/syscall_wrapper.h
> > > @@ -6,7 +6,7 @@
> > >   #ifndef _ASM_X86_SYSCALL_WRAPPER_H
> > >   #define _ASM_X86_SYSCALL_WRAPPER_H
> >
> > > -struct pt_regs;
> > > +#include <asm/ptrace.h>
> >
> > >   extern long __x64_sys_ni_syscall(const struct pt_regs *regs);
> > >   extern long __ia32_sys_ni_syscall(const struct pt_regs *regs);
> > > --
> > > 2.37.3
> >

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] x86: Include asm/ptrace.h in syscall_wrapper header
  2022-10-18 12:27 [PATCH] x86: Include asm/ptrace.h in syscall_wrapper header Jiri Olsa
  2022-10-18 18:23 ` sdf
@ 2022-10-24 15:26 ` Lorenz Bauer
  2022-10-24 17:10 ` [tip: x86/urgent] x86/syscall: " tip-bot2 for Jiri Olsa
  2 siblings, 0 replies; 6+ messages in thread
From: Lorenz Bauer @ 2022-10-24 15:26 UTC (permalink / raw)
  To: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Peter Zijlstra, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, Andy Lutomirski
  Cc: Akihiro HARAI, bpf, linux-kernel, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
	Hao Luo

On Tue, 18 Oct 2022, at 13:27, Jiri Olsa wrote:
> From: Jiri Olsa <olsajiri@gmail.com>
>
> With just the forward declaration of the 'struct pt_regs' in
> syscall_wrapper.h, the syscall stub functions:
>
>   __[x64|ia32]_sys_*(struct pt_regs *regs)

I think arm64 has a similar problem: https://elixir.bootlin.com/linux/v6.1-rc2/source/arch/arm64/include/asm/syscall_wrapper.h#L11

Best
Lorenz

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [tip: x86/urgent] x86/syscall: Include asm/ptrace.h in syscall_wrapper header
  2022-10-18 12:27 [PATCH] x86: Include asm/ptrace.h in syscall_wrapper header Jiri Olsa
  2022-10-18 18:23 ` sdf
  2022-10-24 15:26 ` Lorenz Bauer
@ 2022-10-24 17:10 ` tip-bot2 for Jiri Olsa
  2 siblings, 0 replies; 6+ messages in thread
From: tip-bot2 for Jiri Olsa @ 2022-10-24 17:10 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Akihiro HARAI, Jiri Olsa, Borislav Petkov, Andrii Nakryiko,
	stable, x86, linux-kernel

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     9440c42941606af4c379afa3cf8624f0dc43a629
Gitweb:        https://git.kernel.org/tip/9440c42941606af4c379afa3cf8624f0dc43a629
Author:        Jiri Olsa <olsajiri@gmail.com>
AuthorDate:    Tue, 18 Oct 2022 14:27:08 +02:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Mon, 24 Oct 2022 17:57:28 +02:00

x86/syscall: Include asm/ptrace.h in syscall_wrapper header

With just the forward declaration of the 'struct pt_regs' in
syscall_wrapper.h, the syscall stub functions:

  __[x64|ia32]_sys_*(struct pt_regs *regs)

will have different definition of 'regs' argument in BTF data
based on which object file they are defined in.

If the syscall's object includes 'struct pt_regs' definition,
the BTF argument data will point to a 'struct pt_regs' record,
like:

  [226] STRUCT 'pt_regs' size=168 vlen=21
         'r15' type_id=1 bits_offset=0
         'r14' type_id=1 bits_offset=64
         'r13' type_id=1 bits_offset=128
  ...

If not, it will point to a fwd declaration record:

  [15439] FWD 'pt_regs' fwd_kind=struct

and make bpf tracing program hooking on those functions unable
to access fields from 'struct pt_regs'.

Include asm/ptrace.h directly in syscall_wrapper.h to make sure all
syscalls see 'struct pt_regs' definition. This then results in BTF for
'__*_sys_*(struct pt_regs *regs)' functions to point to the actual
struct, not just the forward declaration.

  [ bp: No Fixes tag as this is not really a bug fix but "adjustment" so
    that BTF is happy. ]

Reported-by: Akihiro HARAI <jharai0815@gmail.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Borislav Petkov <bp@suse.de>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Cc: <stable@vger.kernel.org> # this is needed only for BTF so kernels >= 5.15
Link: https://lore.kernel.org/r/20221018122708.823792-1-jolsa@kernel.org
---
 arch/x86/include/asm/syscall_wrapper.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/syscall_wrapper.h b/arch/x86/include/asm/syscall_wrapper.h
index 59358d1..fd2669b 100644
--- a/arch/x86/include/asm/syscall_wrapper.h
+++ b/arch/x86/include/asm/syscall_wrapper.h
@@ -6,7 +6,7 @@
 #ifndef _ASM_X86_SYSCALL_WRAPPER_H
 #define _ASM_X86_SYSCALL_WRAPPER_H
 
-struct pt_regs;
+#include <asm/ptrace.h>
 
 extern long __x64_sys_ni_syscall(const struct pt_regs *regs);
 extern long __ia32_sys_ni_syscall(const struct pt_regs *regs);

^ permalink raw reply related	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-10-24 18:39 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-18 12:27 [PATCH] x86: Include asm/ptrace.h in syscall_wrapper header Jiri Olsa
2022-10-18 18:23 ` sdf
2022-10-19  9:30   ` Jiri Olsa
2022-10-21 21:49     ` Andrii Nakryiko
2022-10-24 15:26 ` Lorenz Bauer
2022-10-24 17:10 ` [tip: x86/urgent] x86/syscall: " tip-bot2 for Jiri Olsa

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).