netdev - Re: [PATCH net] net: ipv6: ip6_output: alloc skb with tailroom

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] [day] [month] [year] [list]

Date:   Thu, 7 Jun 2018 12:28:12 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Alexander Aring <aring@...atatu.com>
Cc:     David Miller <davem@...emloft.net>,
        Network Development <netdev@...r.kernel.org>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        david.palma@...u.no,
        rabi narayan sahoo <rabinarayans0828@...il.com>,
        Jamal Hadi Salim <jhs@...atatu.com>, stefan@....samsung.com,
        linux-wpan@...r.kernel.org, kernel@...atatu.com, sd@...asysnail.net
Subject: Re: [PATCH net] net: ipv6: ip6_output: alloc skb with tailroom

On Thu, Jun 7, 2018 at 12:22 PM, Alexander Aring <aring@...atatu.com> wrote:
> Hi,
>
> On Wed, Jun 06, 2018 at 04:26:19PM -0400, Willem de Bruijn wrote:
>> On Wed, Jun 6, 2018 at 2:11 PM, David Miller <davem@...emloft.net> wrote:
>> > From: Alexander Aring <aring@...atatu.com>
>> > Date: Wed, 6 Jun 2018 14:09:20 -0400
>> >
>> >> okay, then you want to have this patch for net-next? As an optimization?
>> >>
>> >> Of course, when it's open again.
>> >
>> > Like you, I have questions about where this adjustment is applied and
>> > why.  So I'm not sure yet.
>> >
>> > For example, only IPV6 really takes it into consideration and as you
>> > saw only really for the fragmentation path and not the normal output
>> > path.
>> >
>> > This needs more consideration and investigation.
>>
>> This is the unconditional skb_put in ieee802154_tx. In many cases
>> there is some tailroom due to SKB_DATA_ALIGN in __alloc_skb,
>> so it may take a specific case to not have even 2 bytes of tailroom
>> available.
>
> Yes it's in ieee802154_tx, but we need tailroom not just for checksum.
> The bugreport is related to the two bytes of tailroom, because virtual
> hardware doing checksum by software. The most real transceivers offload
> this feature, so zero tailroom is needed.
>
> I will of course add checks before adding L2 header for headroom and
> tailroom in related subsystem code.
>
> In IEEE 802.15.4 and secured enabled frames we need a MIC field at the
> end of the frame. In worst case this can be 16 bytes.
>
> I looked ethernet macsec feature and it seems they need to have a similar
> reseved tailroom which is 16 bytes by default (max 32 bytes).

Allocating with necessary tailroom to avoid a realloc in the path
sounds fine to me, too. Packet sockets also take needed_tailroom
into account, to give another example.

We just have to avoid papering over bugs. Fixing the locations that
unconditionally move the tail pointer beyond the allocated
region is more important.