netdev - Re: [PATCH bpf 1/2] bpf: pull before calling skb_postpull

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: <CAKH8qBtW_iyP4a_jWbYERC2cZBKTbrW8QMO3S4Hv1xj+CTrv0A@mail.gmail.com>
Date:   Mon, 19 Dec 2022 17:55:31 -0800
From:   Stanislav Fomichev <sdf@...gle.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     daniel@...earbox.net, bpf@...r.kernel.org, netdev@...r.kernel.org,
        Anand Parthasarathy <anpartha@...a.com>, martin.lau@...ux.dev,
        song@...nel.org, john.fastabend@...il.com
Subject: Re: [PATCH bpf 1/2] bpf: pull before calling skb_postpull_rcsum()

On Mon, Dec 19, 2022 at 5:45 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Mon, 19 Dec 2022 17:21:27 -0800 Stanislav Fomichev wrote:
> > > -       skb_postpull_rcsum(skb, skb->data + off, len);
> > > -       memmove(skb->data + len, skb->data, off);
> > > +       old_data = skb->data;
> > >         __skb_pull(skb, len);
> >
> > [..]
>
> Very counter-productively trimmed ;)
>
> > > +       skb_postpull_rcsum(skb, old_data + off, len);
> >
> > Are you sure about the 'old_data + off' part here (for
> > CHECKSUM_COMPLETE)? Shouldn't it be old_data?
>
> AFAIU before:
>
>       old_data (aka skb->data before)
>      /
>     / off  len
>    V-----><--->
> [ .=======xxxxx... buffer ...... ]
>           ^
>            \
>             the xxx part is what we delete
>
> after:
>           skb->data (after)
>          /
>         v
> [ .yyyyy=======... buffer ...... ]
>    <---><----->
>     len   off
>    ^
>     \
>      the yyy part is technically the old values of === but now "outside"
>      of the valid packet data
>
> > I'm assuming we need to negate the old parts that we've pulled?
>
> Yes.
>
> > Maybe safer/more correct to do the following?
> >
> > skb_pull_rcsum(skb, off);
>
> This just pulls from the front, we need to skip over various L2/L3
> headers thanks to off. Hopefully the diagrams help, LMK if they are
> wrong.

Ah, yeah, you're totally correct, thanks for the pics :-) Not sure how
I mixed up off/len in skb_pull_rcsum..

Acked-by: Stanislav Fomichev <sdf@...gle.com>

> > memmove(skb->data, skb->data-off, off);