On Thu, Oct 8, 2020 at 1:34 PM Cong Wang <xiyou.wangc...@gmail.com> wrote: > > On Thu, Oct 8, 2020 at 4:49 AM Willem de Bruijn > <willemdebruijn.ker...@gmail.com> wrote: > > > > On Wed, Oct 7, 2020 at 9:22 PM Cong Wang <xiyou.wangc...@gmail.com> wrote: > > > > > > GRE tunnel has its own header_ops, ipgre_header_ops, and sets it > > > conditionally. When it is set, it assumes the outer IP header is > > > already created before ipgre_xmit(). > > > > > > This is not true when we send packets through a raw packet socket, > > > where L2 headers are supposed to be constructed by user. Packet > > > socket calls dev_validate_header() to validate the header. But > > > GRE tunnel does not set dev->hard_header_len, so that check can > > > be simply bypassed, therefore uninit memory could be passed down > > > to ipgre_xmit(). > > > > If dev->hard_header_len is zero, the packet socket will not reserve > > room for the link layer header, so skb->data points to network_header. > > But I don't see any uninitialized packet data? > > The uninit data is allocated by packet_alloc_skb(), if dev->hard_header_len > is 0 and 'len' is anything between [0, tunnel->hlen + sizeof(struct iphdr)), > dev_validate_header() still returns true obviously but only 'len' > bytes are copied > from user-space by skb_copy_datagram_from_iter(). Therefore, those bytes > within range (len, tunnel->hlen + sizeof(struct iphdr)] are uninitialized.
With dev->hard_header_len of zero, packet_alloc_skb() only allocates len bytes. With SOCK_RAW, the writer is expected to write the ip and gre header and include these in the send len argument. The only difference I see is that with hard_header_len the data starts reserve bytes before skb_network_header, and an additional tail has been allocated that is not used. But this also fixes a potentially more serious bug. With SOCK_DGRAM, dev_hard_header/ipgre_header just assumes that there is enough room in the packet to skb_push(skb, t->hlen + sizeof(*iph)). Which may be false if this header length had not been reserved. Though I've mainly looked at packet_snd. Perhaps you are referring to tpacket_snd?
