netdev - RE: [PATCH bpf-next v3] Add skb_store_bytes() for BPF_PROG_TYPE_CGROUP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <BN7PR02MB52366E745C1EC32B57F2C06AAA4D9@BN7PR02MB5236.namprd02.prod.outlook.com>
Date:   Fri, 7 Jan 2022 16:57:21 +0000
From:   "Tyler Wear (QUIC)" <quic_twear@...cinc.com>
To:     Daniel Borkmann <daniel@...earbox.net>,
        "Tyler Wear (QUIC)" <quic_twear@...cinc.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>
CC:     "maze@...gle.com" <maze@...gle.com>, "yhs@...com" <yhs@...com>,
        "kafai@...com" <kafai@...com>, "toke@...hat.com" <toke@...hat.com>,
        Tyler Wear <quic_twear@...cinc.org>
Subject: RE: [PATCH bpf-next v3] Add skb_store_bytes() for
 BPF_PROG_TYPE_CGROUP_SKB



> -----Original Message-----
> From: Daniel Borkmann <daniel@...earbox.net>
> Sent: Thursday, January 6, 2022 3:35 PM
> To: Tyler Wear (QUIC) <quic_twear@...cinc.com>; netdev@...r.kernel.org; bpf@...r.kernel.org
> Cc: maze@...gle.com; yhs@...com; kafai@...com; toke@...hat.com; Tyler Wear <quic_twear@...cinc.org>
> Subject: Re: [PATCH bpf-next v3] Add skb_store_bytes() for BPF_PROG_TYPE_CGROUP_SKB
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.
> 
> On 1/6/22 1:43 AM, Tyler Wear wrote:
> > From: Tyler Wear <quic_twear@...cinc.org>
> >
> > Need to modify the ds field to support upcoming Wifi QoS Alliance spec.
> > Instead of adding generic function for just modifying the ds field,
> > add skb_store_bytes for BPF_PROG_TYPE_CGROUP_SKB.
> > This allows other fields in the network and transport header to be
> > modified in the future.
> >
> > Checksum API's also need to be added for completeness.
> >
> > It is not possible to use CGROUP_(SET|GET)SOCKOPT since the policy may
> > change during runtime and would result in a large number of entries
> > with wildcards.
> >
> > Signed-off-by: Tyler Wear <quic_twear@...cinc.com>
> > ---
> >   net/core/filter.c                             | 10 ++
> >   .../bpf/prog_tests/cgroup_store_bytes.c       | 97 +++++++++++++++++++
> >   .../selftests/bpf/progs/cgroup_store_bytes.c  | 64 ++++++++++++
> >   3 files changed, 171 insertions(+)
> >   create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_store_bytes.c
> >   create mode 100644
> > tools/testing/selftests/bpf/progs/cgroup_store_bytes.c
> >
> > diff --git a/net/core/filter.c b/net/core/filter.c index
> > 6102f093d59a..ce01a8036361 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -7299,6 +7299,16 @@ cg_skb_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> >               return &bpf_sk_storage_delete_proto;
> >       case BPF_FUNC_perf_event_output:
> >               return &bpf_skb_event_output_proto;
> > +     case BPF_FUNC_skb_store_bytes:
> > +             return &bpf_skb_store_bytes_proto;
> > +     case BPF_FUNC_csum_update:
> > +             return &bpf_csum_update_proto;
> > +     case BPF_FUNC_csum_level:
> > +             return &bpf_csum_level_proto;
> > +     case BPF_FUNC_l3_csum_replace:
> > +             return &bpf_l3_csum_replace_proto;
> > +     case BPF_FUNC_l4_csum_replace:
> > +             return &bpf_l4_csum_replace_proto;
> >   #ifdef CONFIG_SOCK_CGROUP_DATA
> >       case BPF_FUNC_skb_cgroup_id:
> >               return &bpf_skb_cgroup_id_proto;
> 
> Do we need skb_share_check in the write helpers at these hook points when this goes beyond just reading?
> 
> Thanks,
> Daniel

Is there a difference here between the cg_skb and other function proto's that would require skb_share_check? Since these function proto's already exist for other attach types it should be fine right?