[bpf-next,3/3] bpf: Fix an error in verifying a field in a union

Commit Message

Yafang Shao July 9, 2023, 2:59 a.m. UTC

We are utilizing BPF LSM to monitor BPF operations within our container
environment. When we add support for raw_tracepoint, it hits below
error.

; (const void *)attr->raw_tracepoint.name);
27: (79) r3 = *(u64 *)(r2 +0)
access beyond the end of member map_type (mend:4) in struct (anon) with off 0 size 8

It can be reproduced with below BPF prog.

SEC("lsm/bpf")
int BPF_PROG(bpf_audit, int cmd, union bpf_attr *attr, unsigned int size)
{
	switch (cmd) {
	case BPF_RAW_TRACEPOINT_OPEN:
		bpf_printk("raw_tracepoint is %s", attr->raw_tracepoint.name);
		break;
	default:
		break;
	}
	return 0;
}

The reason is that when accessing a field in a union, such as bpf_attr,
if the field is located within a nested struct that is not the first
member of the union, it can result in incorrect field verification.

  union bpf_attr {
      struct {
          __u32 map_type; <<<< Actually it will find that field.
          __u32 key_size;
          __u32 value_size;
         ...
      };
      ...
      struct {
          __u64 name;    <<<< We want to verify this field.
          __u32 prog_fd;
      } raw_tracepoint;
  };

Considering the potential deep nesting levels, finding a perfect
solution to address this issue has proven challenging. Therefore, I
propose a solution where we simply skip the verification process if the
field in question is located within a union.

Fixes: 7e3617a72df3 ("bpf: Add array support to btf_struct_access")
Signed-off-by: Yafang Shao <[e-mail]>
---
 kernel/bpf/btf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Alexei Starovoitov July 11, 2023, 2:56 a.m. UTC | #1

On Sat, Jul 8, 2023 at 7:59 PM Yafang Shao <[e-mail]> wrote:
>
> We are utilizing BPF LSM to monitor BPF operations within our container
> environment. When we add support for raw_tracepoint, it hits below
> error.
>
> ; (const void *)attr->raw_tracepoint.name);
> 27: (79) r3 = *(u64 *)(r2 +0)
> access beyond the end of member map_type (mend:4) in struct (anon) with off 0 size 8
>
> It can be reproduced with below BPF prog.
>
> SEC("lsm/bpf")
> int BPF_PROG(bpf_audit, int cmd, union bpf_attr *attr, unsigned int size)
> {
>         switch (cmd) {
>         case BPF_RAW_TRACEPOINT_OPEN:
>                 bpf_printk("raw_tracepoint is %s", attr->raw_tracepoint.name);
>                 break;
>         default:
>                 break;
>         }
>         return 0;
> }
>
> The reason is that when accessing a field in a union, such as bpf_attr,
> if the field is located within a nested struct that is not the first
> member of the union, it can result in incorrect field verification.
>
>   union bpf_attr {
>       struct {
>           __u32 map_type; <<<< Actually it will find that field.
>           __u32 key_size;
>           __u32 value_size;
>          ...
>       };
>       ...
>       struct {
>           __u64 name;    <<<< We want to verify this field.
>           __u32 prog_fd;
>       } raw_tracepoint;
>   };
>
> Considering the potential deep nesting levels, finding a perfect
> solution to address this issue has proven challenging. Therefore, I
> propose a solution where we simply skip the verification process if the
> field in question is located within a union.
>
> Fixes: 7e3617a72df3 ("bpf: Add array support to btf_struct_access")
> Signed-off-by: Yafang Shao <[e-mail]>
> ---
>  kernel/bpf/btf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index fae6fc24a845..a542760c807a 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6368,7 +6368,7 @@ static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf,
>                  * that also allows using an array of int as a scratch
>                  * space. e.g. skb->cb[].
>                  */
> -               if (off + size > mtrue_end) {
> +               if (off + size > mtrue_end && !(*flag & PTR_UNTRUSTED)) {

The selftest for this condition is missing.

Yafang Shao July 11, 2023, 2:22 p.m. UTC | #2

On Tue, Jul 11, 2023 at 10:56 AM Alexei Starovoitov
<[e-mail]> wrote:
>
> On Sat, Jul 8, 2023 at 7:59 PM Yafang Shao <[e-mail]> wrote:
> >
> > We are utilizing BPF LSM to monitor BPF operations within our container
> > environment. When we add support for raw_tracepoint, it hits below
> > error.
> >
> > ; (const void *)attr->raw_tracepoint.name);
> > 27: (79) r3 = *(u64 *)(r2 +0)
> > access beyond the end of member map_type (mend:4) in struct (anon) with off 0 size 8
> >
> > It can be reproduced with below BPF prog.
> >
> > SEC("lsm/bpf")
> > int BPF_PROG(bpf_audit, int cmd, union bpf_attr *attr, unsigned int size)
> > {
> >         switch (cmd) {
> >         case BPF_RAW_TRACEPOINT_OPEN:
> >                 bpf_printk("raw_tracepoint is %s", attr->raw_tracepoint.name);
> >                 break;
> >         default:
> >                 break;
> >         }
> >         return 0;
> > }
> >
> > The reason is that when accessing a field in a union, such as bpf_attr,
> > if the field is located within a nested struct that is not the first
> > member of the union, it can result in incorrect field verification.
> >
> >   union bpf_attr {
> >       struct {
> >           __u32 map_type; <<<< Actually it will find that field.
> >           __u32 key_size;
> >           __u32 value_size;
> >          ...
> >       };
> >       ...
> >       struct {
> >           __u64 name;    <<<< We want to verify this field.
> >           __u32 prog_fd;
> >       } raw_tracepoint;
> >   };
> >
> > Considering the potential deep nesting levels, finding a perfect
> > solution to address this issue has proven challenging. Therefore, I
> > propose a solution where we simply skip the verification process if the
> > field in question is located within a union.
> >
> > Fixes: 7e3617a72df3 ("bpf: Add array support to btf_struct_access")
> > Signed-off-by: Yafang Shao <[e-mail]>
> > ---
> >  kernel/bpf/btf.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index fae6fc24a845..a542760c807a 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -6368,7 +6368,7 @@ static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf,
> >                  * that also allows using an array of int as a scratch
> >                  * space. e.g. skb->cb[].
> >                  */
> > -               if (off + size > mtrue_end) {
> > +               if (off + size > mtrue_end && !(*flag & PTR_UNTRUSTED)) {
>
> The selftest for this condition is missing.

Will add it.

Patch

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index fae6fc24a845..a542760c807a 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6368,7 +6368,7 @@  static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf,
 		 * that also allows using an array of int as a scratch
 		 * space. e.g. skb->cb[].
 		 */
-		if (off + size > mtrue_end) {
+		if (off + size > mtrue_end && !(*flag & PTR_UNTRUSTED)) {
 			bpf_log(log,
 				"access beyond the end of member %s (mend:%u) in struct %s with off %u size %u\n",
 				mname, mtrue_end, tname, off, size);