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  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, 17 Aug 2020 09:48:10 +0200
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     Jesper Dangaard Brouer <brouer@...hat.com>
Cc:     David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
        bpf@...r.kernel.org
Subject: Re: [PATCH net] net: xdp: pull ethernet header off packet after
 computing skb->protocol

On 8/17/20, Jesper Dangaard Brouer <brouer@...hat.com> wrote:
> On Sun, 16 Aug 2020 15:29:37 -0700 (PDT)
> David Miller <davem@...emloft.net> wrote:
>
>> From: "Jason A. Donenfeld" <Jason@...c4.com>
>> Date: Sat, 15 Aug 2020 09:29:30 +0200
>>
>> > When an XDP program changes the ethernet header protocol field,
>> > eth_type_trans is used to recalculate skb->protocol. In order for
>> > eth_type_trans to work correctly, the ethernet header must actually be
>> > part of the skb data segment, so the code first pushes that onto the
>> > head of the skb. However, it subsequently forgets to pull it back off,
>> > making the behavior of the passed-on packet inconsistent between the
>> > protocol modifying case and the static protocol case. This patch fixes
>> > the issue by simply pulling the ethernet header back off of the skb
>> > head.
>> >
>> > Fixes: 297249569932 ("net: fix generic XDP to handle if eth header was
>> > mangled")
>> > Cc: Jesper Dangaard Brouer <brouer@...hat.com>
>> > Cc: David S. Miller <davem@...emloft.net>
>> > Signed-off-by: Jason A. Donenfeld <Jason@...c4.com>
>>
>> Applied and queued up for -stable, thanks.
>>
>> Jesper, I wonder how your original patch was tested because it pushes a
>> packet
>> with skb->data pointing at the ethernet header into the stack.  That
>> should be
>> popped at this point as per this fix here.
>
> I think this patch is wrong, because eth_type_trans() also does a
> skb_pull_inline(skb, ETH_HLEN).

Huh, wow. That's one unusual and confusing function. But indeed it
seems like I'm the one who needs to reevaluate testing methodology
here. I'm very sorry for the noise and hassle.

Jason

Powered by blists - more mailing lists