On Sun, Oct 11, 2020 at 5:00 PM Willem de Bruijn <willemdebruijn.ker...@gmail.com> wrote: > > On Sun, Oct 11, 2020 at 3:11 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(). Similar for dev->needed_headroom. > > > > dev->hard_header_len is supposed to be the length of the header > > created by dev->header_ops->create(), so it should be used whenever > > header_ops is set, and dev->needed_headroom should be used when it > > is not set. > > > > Reported-and-tested-by: > > syzbot+4a2c52677a8a1aa28...@syzkaller.appspotmail.com > > Cc: Xie He <xie.he.0...@gmail.com> > > Cc: William Tu <u9012...@gmail.com> > > Cc: Willem de Bruijn <willemdebruijn.ker...@gmail.com> > > Signed-off-by: Cong Wang <xiyou.wangc...@gmail.com>
> I agree that then swapping dev->needed_headroom for > dev->hard_header_len at init is then a good approach. > > Underlying functions like ip_tunnel_xmit can modify needed_headroom at > runtime, not sure how that affects runtime changes in > ipgre_link_update: Never mind, the patch only selects between needed_headroom or hard_header_link at ipgre_tunnel_init. > > @@ -987,10 +991,14 @@ static int ipgre_tunnel_init(struct net_device *dev) > > return -EINVAL; > > dev->flags = IFF_BROADCAST; > > dev->header_ops = &ipgre_header_ops; > > + dev->hard_header_len = tunnel->hlen + sizeof(*iph); > > + dev->needed_headroom = 0; > > here and below, perhaps more descriptive of what is being done, something > like: > > /* If device has header_ops.create, then headers are part of hard_header_len > */ > swap(dev->needed_headroom, dev->hard_header_len) Arguably nitpicking. Otherwise, patch looks great to me, thanks, so Acked-by: Willem de Bruijn <will...@google.com>
