netdev - RE: [PATCH bpf-next v6 1/2] 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <BYAPR02MB523848C2591E467973B5592EAA539@BYAPR02MB5238.namprd02.prod.outlook.com>
Date:   Thu, 13 Jan 2022 05:12:34 +0000
From:   Tyler Wear <twear@...cinc.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        "Tyler Wear (QUIC)" <quic_twear@...cinc.com>
CC:     Network Development <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>,
        Maciej Żenczykowski <maze@...gle.com>,
        Yonghong Song <yhs@...com>, Martin KaFai Lau <kafai@...com>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Song Liu <song@...nel.org>
Subject: RE: [PATCH bpf-next v6 1/2] Add skb_store_bytes() for
 BPF_PROG_TYPE_CGROUP_SKB



> -----Original Message-----
> From: Alexei Starovoitov <alexei.starovoitov@...il.com>
> Sent: Wednesday, January 12, 2022 6:14 PM
> To: Tyler Wear (QUIC) <quic_twear@...cinc.com>
> Cc: Network Development <netdev@...r.kernel.org>; bpf
> <bpf@...r.kernel.org>; Maciej Żenczykowski <maze@...gle.com>;
> Yonghong Song <yhs@...com>; Martin KaFai Lau <kafai@...com>; Toke
> Høiland-Jørgensen <toke@...hat.com>; Daniel Borkmann
> <daniel@...earbox.net>; Song Liu <song@...nel.org>
> Subject: Re: [PATCH bpf-next v6 1/2] 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 Wed, Jan 12, 2022 at 5:15 PM Tyler Wear <quic_twear@...cinc.com>
> wrote:
> >
> > 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.
> >
> > V4 patch fixes warnings and errors from checkpatch.
> >
> > The existing check for bpf_try_make_writable() should mean that
> > skb_share_check() is not needed.
> >
> > Signed-off-by: Tyler Wear <quic_twear@...cinc.com>
> > ---
> >  net/core/filter.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/net/core/filter.c b/net/core/filter.c index
> > 6102f093d59a..f30d939cb4cf 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -7299,6 +7299,18 @@ 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;
> > +       case BPF_FUNC_csum_diff:
> > +               return &bpf_csum_diff_proto;
> 
> This is wrong.
> CGROUP_INET_EGRESS bpf prog cannot arbitrary change packet data.
> The networking stack populated the IP header at that point.
> If the prog changes it to something else it will be confusing other layers of
> stack. neigh(L2) will be wrong, etc.
> We can still change certain things in the packet, but not arbitrary bytes.
> 
> We cannot change the DS field directly in the packet either.
> It can only be changed by changing its value in the socket.

Why is the DS field unchangeable, but ecn is changeable?

> 
> TC layer is where packet modifications are allowed.