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]

Date:   Mon, 19 Dec 2022 17:21:27 -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 4:47 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> Anand hit a BUG() when pulling off headers on egress to a SW tunnel.
> We get to skb_checksum_help() with an invalid checksum offset
> (commit d7ea0d9df2a6 ("net: remove two BUG() from skb_checksum_help()")
> converted those BUGs to WARN_ONs()).
> He points out oddness in how skb_postpull_rcsum() gets used.
> Indeed looks like we should pull before "postpull", otherwise
> the CHECKSUM_PARTIAL fixup from skb_postpull_rcsum() will not
> be able to do its job:
>
>         if (skb->ip_summed == CHECKSUM_PARTIAL &&
>             skb_checksum_start_offset(skb) < 0)
>                 skb->ip_summed = CHECKSUM_NONE;
>
> Reported-by: Anand Parthasarathy <anpartha@...a.com>
> Fixes: 6578171a7ff0 ("bpf: add bpf_skb_change_proto helper")
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
> CC: daniel@...earbox.net
> CC: martin.lau@...ux.dev
> CC: song@...nel.org
> CC: john.fastabend@...il.com
> CC: sdf@...gle.com
> CC: bpf@...r.kernel.org
> ---
>  net/core/filter.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 929358677183..43cc1fe58a2c 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -3180,15 +3180,18 @@ static int bpf_skb_generic_push(struct sk_buff *skb, u32 off, u32 len)
>
>  static int bpf_skb_generic_pop(struct sk_buff *skb, u32 off, u32 len)
>  {
> +       void *old_data;
> +
>         /* skb_ensure_writable() is not needed here, as we're
>          * already working on an uncloned skb.
>          */
>         if (unlikely(!pskb_may_pull(skb, off + len)))
>                 return -ENOMEM;
>
> -       skb_postpull_rcsum(skb, skb->data + off, len);
> -       memmove(skb->data + len, skb->data, off);
> +       old_data = skb->data;
>         __skb_pull(skb, len);

[..]

> +       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?
I'm assuming we need to negate the old parts that we've pulled?

Maybe safer/more correct to do the following?

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


> +       memmove(skb->data, old_data, off);
>
>         return 0;
>  }
> --
> 2.38.1
>