linux-kernel - Re: [PATCH] drivers/net/wan/lapbether: Use needed_headroom instead of hard_header

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: <20200728195246.GA482576@google.com>
Date:   Tue, 28 Jul 2020 12:52:46 -0700
From:   Brian Norris <briannorris@...omium.org>
To:     Xie He <xie.he.0141@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-x25@...r.kernel.org,
        Cong Wang <xiyou.wangcong@...il.com>
Subject: Re: [PATCH] drivers/net/wan/lapbether: Use needed_headroom instead
 of hard_header_len

(Reviewing as requested; I'm not familiar with this driver either, or
really any WAN driver. It also seems that hard_header_len vs.
needed_headroom aren't very well documented, and even I can't guarantee
I understand them completely. So take my thoughts with a grain of salt.)

Hi,

On Sun, Jul 26, 2020 at 04:05:24AM -0700, Xie He wrote:
> In net/packet/af_packet.c, the function packet_snd first reserves a
> headroom of length (dev->hard_header_len + dev->needed_headroom).
> Then if the socket is a SOCK_DGRAM socket, it calls dev_hard_header,
> which calls dev->header_ops->create, to create the link layer header.
> If the socket is a SOCK_RAW socket, it "un-reserves" a headroom of
> length (dev->hard_header_len), and assumes the user to provide the
> appropriate link layer header.
> 
> So according to the logic of af_packet.c, dev->hard_header_len should
> be the length of the header that would be created by
> dev->header_ops->create.

I believe I'm with you up to here, but:

> However, this driver doesn't provide dev->header_ops, so logically
> dev->hard_header_len should be 0.

I'm not clear on this part.

What's to say you shouldn't be implementing header_ops instead? Note
that with WiFi drivers, they're exposing themselves as ARPHRD_ETHER, and
only the Ethernet headers are part of the upper "protocol" headers. So
my patch deferred to the eth headers.

What is the intention with this X25 protocol? I guess the headers added
in lapbeth_data_transmit() are supposed to be "invisible", as with this
note in af_packet.c?

   - if device has no dev->hard_header routine, it adds and removes ll header
     inside itself. In this case ll header is invisible outside of device,
     but higher levels still should reserve dev->hard_header_len.

If that's the case, then yes, I believe this patch should be correct.

Brian

> So we should use dev->needed_headroom instead of dev->hard_header_len
> to request necessary headroom to be allocated.
> 
> Signed-off-by: Xie He <xie.he.0141@...il.com>