[v2,bpf-next,2/4] bpf: handle bpf_user_pt_regs_t typedef explicitly for PTR_TO_CTX global arg

Commit Message

Andrii Nakryiko Feb. 12, 2024, 11:32 p.m. UTC

Expected canonical argument type for global function arguments
representing PTR_TO_CTX is `bpf_user_pt_regs_t *ctx`. This currently
works on s390x by accident because kernel resolves such typedef to
underlying struct (which is anonymous on s390x), and erroneously
accepting it as expected context type. We are fixing this problem next,
which would break s390x arch, so we need to handle `bpf_user_pt_regs_t`
case explicitly for KPROBE programs.

Fixes: 91cc1a99740e ("bpf: Annotate context types")
Signed-off-by: Andrii Nakryiko <[e-mail]>
---
 kernel/bpf/btf.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

Eduard Zingerman Feb. 13, 2024, 4:40 p.m. UTC | #1

On Mon, 2024-02-12 at 15:32 -0800, Andrii Nakryiko wrote:
> Expected canonical argument type for global function arguments
> representing PTR_TO_CTX is `bpf_user_pt_regs_t *ctx`. This currently
> works on s390x by accident because kernel resolves such typedef to
> underlying struct (which is anonymous on s390x), and erroneously
> accepting it as expected context type. We are fixing this problem next,
> which would break s390x arch, so we need to handle `bpf_user_pt_regs_t`
> case explicitly for KPROBE programs.
> 
> Fixes: 91cc1a99740e ("bpf: Annotate context types")
> Signed-off-by: Andrii Nakryiko <[e-mail]>
> ---

Nit: same could be achieved w/o special casing kprobes by looking
     if typedef's type is named before skipping, e.g. as below.
     But I do not insist, probably good as it is as well.

---

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index f0ce384aa73e..830635b37fa1 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -907,11 +907,9 @@ bool btf_member_is_reg_int(const struct btf *btf, const struct btf_type *s,
 }
 
 /* Similar to btf_type_skip_modifiers() but does not skip typedefs. */
-static const struct btf_type *btf_type_skip_qualifiers(const struct btf *btf,
-						       u32 id)
+static const struct btf_type *__btf_type_skip_qualifiers(const struct btf *btf,
+							 const struct btf_type *t)
 {
-	const struct btf_type *t = btf_type_by_id(btf, id);
-
 	while (btf_type_is_modifier(t) &&
 	       BTF_INFO_KIND(t->info) != BTF_KIND_TYPEDEF) {
 		t = btf_type_by_id(btf, t->type);
@@ -920,6 +918,12 @@ static const struct btf_type *btf_type_skip_qualifiers(const struct btf *btf,
 	return t;
 }
 
+static const struct btf_type *btf_type_skip_qualifiers(const struct btf *btf,
+						       u32 id)
+{
+	return __btf_type_skip_qualifiers(btf, btf_type_by_id(btf, id));
+}
+
 #define BTF_SHOW_MAX_ITER	10
 
 #define BTF_KIND_BIT(kind)	(1ULL << kind)
@@ -5695,9 +5699,25 @@ bool btf_is_prog_ctx_type(struct bpf_verifier_log *log, const struct btf *btf,
 	const char *tname, *ctx_tname;
 
 	t = btf_type_by_id(btf, t->type);
-	while (btf_type_is_modifier(t))
-		t = btf_type_by_id(btf, t->type);
-	if (!btf_type_is_struct(t)) {
+
+	/* Skip modifiers, but stop if skipping of typedef would
+	 * lead an anonymous type, e.g. like for s390:
+	 *
+	 *   typedef struct { ... } user_pt_regs;
+	 *   typedef user_pt_regs bpf_user_pt_regs_t;
+	 */
+	t = __btf_type_skip_qualifiers(btf, t);
+	while (btf_type_is_typedef(t)) {
+		const struct btf_type *t1;
+
+		t1 = btf_type_by_id(btf, t->type);
+		t1 = __btf_type_skip_qualifiers(btf, t1);
+		tname = btf_name_by_offset(btf, t1->name_off);
+		if (!tname || tname[0] == '\0')
+			break;
+		t = t1;
+	}
+	if (!btf_type_is_struct(t) && !btf_type_is_typedef(t)) {
 		/* Only pointer to struct is supported for now.
 		 * That means that BPF_PROG_TYPE_TRACEPOINT with BTF
 		 * is not supported yet.

Andrii Nakryiko Feb. 13, 2024, 5:02 p.m. UTC | #2

On Tue, Feb 13, 2024 at 8:40 AM Eduard Zingerman <[e-mail]> wrote:
>
> On Mon, 2024-02-12 at 15:32 -0800, Andrii Nakryiko wrote:
> > Expected canonical argument type for global function arguments
> > representing PTR_TO_CTX is `bpf_user_pt_regs_t *ctx`. This currently
> > works on s390x by accident because kernel resolves such typedef to
> > underlying struct (which is anonymous on s390x), and erroneously
> > accepting it as expected context type. We are fixing this problem next,
> > which would break s390x arch, so we need to handle `bpf_user_pt_regs_t`
> > case explicitly for KPROBE programs.
> >
> > Fixes: 91cc1a99740e ("bpf: Annotate context types")
> > Signed-off-by: Andrii Nakryiko <[e-mail]>
> > ---
>
> Nit: same could be achieved w/o special casing kprobes by looking
>      if typedef's type is named before skipping, e.g. as below.
>      But I do not insist, probably good as it is as well.
>
> ---
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index f0ce384aa73e..830635b37fa1 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -907,11 +907,9 @@ bool btf_member_is_reg_int(const struct btf *btf, const struct btf_type *s,
>  }
>
>  /* Similar to btf_type_skip_modifiers() but does not skip typedefs. */
> -static const struct btf_type *btf_type_skip_qualifiers(const struct btf *btf,
> -                                                      u32 id)
> +static const struct btf_type *__btf_type_skip_qualifiers(const struct btf *btf,
> +                                                        const struct btf_type *t)
>  {
> -       const struct btf_type *t = btf_type_by_id(btf, id);
> -
>         while (btf_type_is_modifier(t) &&
>                BTF_INFO_KIND(t->info) != BTF_KIND_TYPEDEF) {
>                 t = btf_type_by_id(btf, t->type);
> @@ -920,6 +918,12 @@ static const struct btf_type *btf_type_skip_qualifiers(const struct btf *btf,
>         return t;
>  }
>
> +static const struct btf_type *btf_type_skip_qualifiers(const struct btf *btf,
> +                                                      u32 id)
> +{
> +       return __btf_type_skip_qualifiers(btf, btf_type_by_id(btf, id));
> +}
> +
>  #define BTF_SHOW_MAX_ITER      10
>
>  #define BTF_KIND_BIT(kind)     (1ULL << kind)
> @@ -5695,9 +5699,25 @@ bool btf_is_prog_ctx_type(struct bpf_verifier_log *log, const struct btf *btf,
>         const char *tname, *ctx_tname;
>
>         t = btf_type_by_id(btf, t->type);
> -       while (btf_type_is_modifier(t))
> -               t = btf_type_by_id(btf, t->type);
> -       if (!btf_type_is_struct(t)) {
> +
> +       /* Skip modifiers, but stop if skipping of typedef would
> +        * lead an anonymous type, e.g. like for s390:
> +        *
> +        *   typedef struct { ... } user_pt_regs;
> +        *   typedef user_pt_regs bpf_user_pt_regs_t;
> +        */
> +       t = __btf_type_skip_qualifiers(btf, t);
> +       while (btf_type_is_typedef(t)) {
> +               const struct btf_type *t1;
> +
> +               t1 = btf_type_by_id(btf, t->type);
> +               t1 = __btf_type_skip_qualifiers(btf, t1);
> +               tname = btf_name_by_offset(btf, t1->name_off);
> +               if (!tname || tname[0] == '\0')
> +                       break;
> +               t = t1;
> +       }
> +       if (!btf_type_is_struct(t) && !btf_type_is_typedef(t)) {

and now we potentially are intermixing structs and typedefs and don't
really distinguish them later (but struct abc is not the same thing as
typedef abc), which is probably not what we want.

Also, we resolve typedef to its underlying type *or* typedef, right?
So if I have typedef A -> typedef B -> struct C, we won't get to
struct C, even if struct C is the expected correct context type for a
given program type (and it should work).

So in general, yes, I think this code could be changed to not ignore
typedefs and do the right thing, but we'd need to be very careful to
not allow unexpected things for all program types. Given only kprobes
define context as typedef, it's simpler and more reliable to special
case kprobe, IMO.

>                 /* Only pointer to struct is supported for now.
>                  * That means that BPF_PROG_TYPE_TRACEPOINT with BTF
>                  * is not supported yet.

Eduard Zingerman Feb. 13, 2024, 5:08 p.m. UTC | #3

On Tue, 2024-02-13 at 09:02 -0800, Andrii Nakryiko wrote:
[...]

> >         t = btf_type_by_id(btf, t->type);
> > -       while (btf_type_is_modifier(t))
> > -               t = btf_type_by_id(btf, t->type);
> > -       if (!btf_type_is_struct(t)) {
> > +
> > +       /* Skip modifiers, but stop if skipping of typedef would
> > +        * lead an anonymous type, e.g. like for s390:
> > +        *
> > +        *   typedef struct { ... } user_pt_regs;
> > +        *   typedef user_pt_regs bpf_user_pt_regs_t;
> > +        */
> > +       t = __btf_type_skip_qualifiers(btf, t);
> > +       while (btf_type_is_typedef(t)) {
> > +               const struct btf_type *t1;
> > +
> > +               t1 = btf_type_by_id(btf, t->type);
> > +               t1 = __btf_type_skip_qualifiers(btf, t1);
> > +               tname = btf_name_by_offset(btf, t1->name_off);
> > +               if (!tname || tname[0] == '\0')
> > +                       break;
> > +               t = t1;
> > +       }
> > +       if (!btf_type_is_struct(t) && !btf_type_is_typedef(t)) {
> 
> and now we potentially are intermixing structs and typedefs and don't
> really distinguish them later (but struct abc is not the same thing as
> typedef abc), which is probably not what we want.

Yes, need a condition in the end to check that 'ctx_type' and 't' have
same kind.

> Also, we resolve typedef to its underlying type *or* typedef, right?
> So if I have typedef A -> typedef B -> struct C, we won't get to
> struct C, even if struct C is the expected correct context type for a
> given program type (and it should work).

For code above we would get to 'struct C'.

> So in general, yes, I think this code could be changed to not ignore
> typedefs and do the right thing, but we'd need to be very careful to
> not allow unexpected things for all program types. Given only kprobes
> define context as typedef, it's simpler and more reliable to special
> case kprobe, IMO.

Maybe, I do not insist.

Andrii Nakryiko Feb. 13, 2024, 6:12 p.m. UTC | #4

On Tue, Feb 13, 2024 at 9:08 AM Eduard Zingerman <[e-mail]> wrote:
>
> On Tue, 2024-02-13 at 09:02 -0800, Andrii Nakryiko wrote:
> [...]
>
> > >         t = btf_type_by_id(btf, t->type);
> > > -       while (btf_type_is_modifier(t))
> > > -               t = btf_type_by_id(btf, t->type);
> > > -       if (!btf_type_is_struct(t)) {
> > > +
> > > +       /* Skip modifiers, but stop if skipping of typedef would
> > > +        * lead an anonymous type, e.g. like for s390:
> > > +        *
> > > +        *   typedef struct { ... } user_pt_regs;
> > > +        *   typedef user_pt_regs bpf_user_pt_regs_t;
> > > +        */
> > > +       t = __btf_type_skip_qualifiers(btf, t);
> > > +       while (btf_type_is_typedef(t)) {
> > > +               const struct btf_type *t1;
> > > +
> > > +               t1 = btf_type_by_id(btf, t->type);
> > > +               t1 = __btf_type_skip_qualifiers(btf, t1);
> > > +               tname = btf_name_by_offset(btf, t1->name_off);
> > > +               if (!tname || tname[0] == '\0')
> > > +                       break;
> > > +               t = t1;
> > > +       }
> > > +       if (!btf_type_is_struct(t) && !btf_type_is_typedef(t)) {
> >
> > and now we potentially are intermixing structs and typedefs and don't
> > really distinguish them later (but struct abc is not the same thing as
> > typedef abc), which is probably not what we want.
>
> Yes, need a condition in the end to check that 'ctx_type' and 't' have
> same kind.

Yeah, and then special case, for KPROBE that `struct
bpf_user_pt_regs_t` (not a typedef!) is also acceptable. Hopefully you
see why I'm saying that the early special case of `bpf_user_pt_regs_t`
typedef is much easier to reason about.

>
> > Also, we resolve typedef to its underlying type *or* typedef, right?
> > So if I have typedef A -> typedef B -> struct C, we won't get to
> > struct C, even if struct C is the expected correct context type for a
> > given program type (and it should work).
>
> For code above we would get to 'struct C'.

ha, right, it's "break if empty name", not the other way.

So basically in `typedef A -> typedef B -> struct C` we get `typedef
B` if C is anon, otherwise it's `struct C`. It will be a slightly
different behavior for bpf_user_pt_regs_t if the user typedef'ed it
(for whatever reason).

>
> > So in general, yes, I think this code could be changed to not ignore
> > typedefs and do the right thing, but we'd need to be very careful to
> > not allow unexpected things for all program types. Given only kprobes
> > define context as typedef, it's simpler and more reliable to special
> > case kprobe, IMO.
>
> Maybe, I do not insist.

Great.

Eduard Zingerman Feb. 13, 2024, 6:48 p.m. UTC | #5

On Tue, 2024-02-13 at 10:12 -0800, Andrii Nakryiko wrote:
[...]

> Yeah, and then special case, for KPROBE that `struct
> bpf_user_pt_regs_t` (not a typedef!) is also acceptable.

Hm, I missed the point that for kporbes there is a need to accept both
simultaneously (for the same running kernel):

    typedef __whatever__ bpf_user_pt_regs_t;
    struct bpf_user_pt_regs_t {};

If this is the only such case, then I agree that special case is
simplest to implement.

Andrii Nakryiko Feb. 13, 2024, 6:59 p.m. UTC | #6

On Tue, Feb 13, 2024 at 10:48 AM Eduard Zingerman <[e-mail]> wrote:
>
> On Tue, 2024-02-13 at 10:12 -0800, Andrii Nakryiko wrote:
> [...]
>
> > Yeah, and then special case, for KPROBE that `struct
> > bpf_user_pt_regs_t` (not a typedef!) is also acceptable.
>
> Hm, I missed the point that for kporbes there is a need to accept both
> simultaneously (for the same running kernel):
>
>     typedef __whatever__ bpf_user_pt_regs_t;
>     struct bpf_user_pt_regs_t {};
>
> If this is the only such case, then I agree that special case is
> simplest to implement.

Yeah, it's backwards compatibility quirk which is important to preserve

Patch

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index f0ce384aa73e..da958092c969 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -5695,6 +5695,21 @@  bool btf_is_prog_ctx_type(struct bpf_verifier_log *log, const struct btf *btf,
 	const char *tname, *ctx_tname;
 
 	t = btf_type_by_id(btf, t->type);
+
+	/* KPROBE programs allow bpf_user_pt_regs_t typedef, which we need to
+	 * check before we skip all the typedef below.
+	 */
+	if (prog_type == BPF_PROG_TYPE_KPROBE) {
+		while (btf_type_is_modifier(t) && !btf_type_is_typedef(t))
+			t = btf_type_by_id(btf, t->type);
+
+		if (btf_type_is_typedef(t)) {
+			tname = btf_name_by_offset(btf, t->name_off);
+			if (tname && strcmp(tname, "bpf_user_pt_regs_t") == 0)
+				return true;
+		}
+	}
+
 	while (btf_type_is_modifier(t))
 		t = btf_type_by_id(btf, t->type);
 	if (!btf_type_is_struct(t)) {