[net,2/2] packet: round up linear to header len
diff mbox

Message ID 20170207205721.23207-3-willemdebruijn.kernel@gmail.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Willem de Bruijn Feb. 7, 2017, 8:57 p.m. UTC
From: Willem de Bruijn <willemb@google.com>

Link layer protocols may unconditionally pull headers, as Ethernet
does in eth_type_trans. Ensure that the entire link layer header
always lies in the skb linear segment. tpacket_snd has such a check.
Extend this to packet_snd.

Variable length link layer headers complicate the computation
somewhat. Here skb->len may be smaller than dev->hard_header_len.

Round up the linear length to be at least as long as the smallest of
the two.

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 net/packet/af_packet.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Eric Dumazet Feb. 7, 2017, 9:23 p.m. UTC | #1
On Tue, 2017-02-07 at 15:57 -0500, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
> 
> Link layer protocols may unconditionally pull headers, as Ethernet
> does in eth_type_trans. Ensure that the entire link layer header
> always lies in the skb linear segment. tpacket_snd has such a check.
> Extend this to packet_snd.
> 
> Variable length link layer headers complicate the computation
> somewhat. Here skb->len may be smaller than dev->hard_header_len.
> 
> Round up the linear length to be at least as long as the smallest of
> the two.
> 
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> ---

Acked-by: Eric Dumazet <edumazet@google.com>
Sowmini Varadhan Feb. 8, 2017, 3:34 p.m. UTC | #2
On (02/07/17 15:57), Willem de Bruijn wrote:
> @@ -2816,8 +2816,9 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
>  	err = -ENOBUFS;
>  	hlen = LL_RESERVED_SPACE(dev);
>  	tlen = dev->needed_tailroom;
> -	skb = packet_alloc_skb(sk, hlen + tlen, hlen, len,
> -			       __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len),
> +	linear = __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len);
> +	linear = max(linear, min_t(int, len, dev->hard_header_len));
> +	skb = packet_alloc_skb(sk, hlen + tlen, hlen, len, linear,
>  			       msg->msg_flags & MSG_DONTWAIT, &err);

do we need a similar check in packet_sendsmg_spkt (even if it
is deprecated, would be better to get it to align with packet_snd?)

Also tpacket_fill_skb should ensure that copylen is set up like
the code above?

--Sowmini
Willem de Bruijn Feb. 8, 2017, 4:37 p.m. UTC | #3
On Wed, Feb 8, 2017 at 7:34 AM, Sowmini Varadhan
<sowmini.varadhan@oracle.com> wrote:
> On (02/07/17 15:57), Willem de Bruijn wrote:
>> @@ -2816,8 +2816,9 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
>>       err = -ENOBUFS;
>>       hlen = LL_RESERVED_SPACE(dev);
>>       tlen = dev->needed_tailroom;
>> -     skb = packet_alloc_skb(sk, hlen + tlen, hlen, len,
>> -                            __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len),
>> +     linear = __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len);
>> +     linear = max(linear, min_t(int, len, dev->hard_header_len));
>> +     skb = packet_alloc_skb(sk, hlen + tlen, hlen, len, linear,
>>                              msg->msg_flags & MSG_DONTWAIT, &err);
>
> do we need a similar check in packet_sendsmg_spkt (even if it
> is deprecated, would be better to get it to align with packet_snd?)

That path does not need this check, because it does not support
large packets and always allocates the entire packet as linear.

> Also tpacket_fill_skb should ensure that copylen is set up like
> the code above?

Do you mean the difference that it unconditionally pulls
hard_header_len, optionally with zero padding, whereas this new
path can check against new min_header_len and thus allows
packets shorter than hard_header_len?
Sowmini Varadhan Feb. 8, 2017, 4:58 p.m. UTC | #4
On (02/08/17 08:37), Willem de Bruijn wrote:
> Date: Wed, 8 Feb 2017 08:37:19 -0800
> From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> To: Sowmini Varadhan <sowmini.varadhan@oracle.com>
> Cc: Network Development <netdev@vger.kernel.org>, David Miller
>  <davem@davemloft.net>, Eric Dumazet <eric.dumazet@gmail.com>, Dmitry
>  Vyukov <dvyukov@google.com>, Willem de Bruijn <willemb@google.com>
> Subject: Re: [PATCH net 2/2] packet: round up linear to header len
> 
> On Wed, Feb 8, 2017 at 7:34 AM, Sowmini Varadhan
> <sowmini.varadhan@oracle.com> wrote:
> > On (02/07/17 15:57), Willem de Bruijn wrote:
> >> @@ -2816,8 +2816,9 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
> >>       err = -ENOBUFS;
> >>       hlen = LL_RESERVED_SPACE(dev);
> >>       tlen = dev->needed_tailroom;
> >> -     skb = packet_alloc_skb(sk, hlen + tlen, hlen, len,
> >> -                            __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len),
> >> +     linear = __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len);
> >> +     linear = max(linear, min_t(int, len, dev->hard_header_len));
> >> +     skb = packet_alloc_skb(sk, hlen + tlen, hlen, len, linear,
> >>                              msg->msg_flags & MSG_DONTWAIT, &err);
> >
  :
> Do you mean the difference that it unconditionally pulls
> hard_header_len, optionally with zero padding, whereas this new
> path can check against new min_header_len and thus allows
> packets shorter than hard_header_len?

yes, maybe it doesnt matter, becaues hard_header_len >= min_header_len
at all times

--Sowmini
Willem de Bruijn Feb. 8, 2017, 5:27 p.m. UTC | #5
On Wed, Feb 8, 2017 at 8:58 AM, Sowmini Varadhan
<sowmini.varadhan@oracle.com> wrote:
> On (02/08/17 08:37), Willem de Bruijn wrote:
>> Date: Wed, 8 Feb 2017 08:37:19 -0800
>> From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
>> To: Sowmini Varadhan <sowmini.varadhan@oracle.com>
>> Cc: Network Development <netdev@vger.kernel.org>, David Miller
>>  <davem@davemloft.net>, Eric Dumazet <eric.dumazet@gmail.com>, Dmitry
>>  Vyukov <dvyukov@google.com>, Willem de Bruijn <willemb@google.com>
>> Subject: Re: [PATCH net 2/2] packet: round up linear to header len
>>
>> On Wed, Feb 8, 2017 at 7:34 AM, Sowmini Varadhan
>> <sowmini.varadhan@oracle.com> wrote:
>> > On (02/07/17 15:57), Willem de Bruijn wrote:
>> >> @@ -2816,8 +2816,9 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
>> >>       err = -ENOBUFS;
>> >>       hlen = LL_RESERVED_SPACE(dev);
>> >>       tlen = dev->needed_tailroom;
>> >> -     skb = packet_alloc_skb(sk, hlen + tlen, hlen, len,
>> >> -                            __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len),
>> >> +     linear = __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len);
>> >> +     linear = max(linear, min_t(int, len, dev->hard_header_len));
>> >> +     skb = packet_alloc_skb(sk, hlen + tlen, hlen, len, linear,
>> >>                              msg->msg_flags & MSG_DONTWAIT, &err);
>> >
>   :
>> Do you mean the difference that it unconditionally pulls
>> hard_header_len, optionally with zero padding, whereas this new
>> path can check against new min_header_len and thus allows
>> packets shorter than hard_header_len?
>
> yes, maybe it doesnt matter, becaues hard_header_len >= min_header_len
> at all times

The code is not subject to this bug, so I'd rather not touch it in
this fix for stable.

But you raise a good point. This logic is subtle and fragile. It will be good to
deduplicate across packet_snd and tpacket_snd in a follow-up to net-next.

Patch
diff mbox

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 3d555c79a7b5..d56ee46b11fc 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2755,7 +2755,7 @@  static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
 	struct virtio_net_hdr vnet_hdr = { 0 };
 	int offset = 0;
 	struct packet_sock *po = pkt_sk(sk);
-	int hlen, tlen;
+	int hlen, tlen, linear;
 	int extra_len = 0;
 
 	/*
@@ -2816,8 +2816,9 @@  static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
 	err = -ENOBUFS;
 	hlen = LL_RESERVED_SPACE(dev);
 	tlen = dev->needed_tailroom;
-	skb = packet_alloc_skb(sk, hlen + tlen, hlen, len,
-			       __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len),
+	linear = __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len);
+	linear = max(linear, min_t(int, len, dev->hard_header_len));
+	skb = packet_alloc_skb(sk, hlen + tlen, hlen, len, linear,
 			       msg->msg_flags & MSG_DONTWAIT, &err);
 	if (skb == NULL)
 		goto out_unlock;