Message ID | 20180926093018.6646-1-vdronov@redhat.com |
---|---|
State | Rejected, archived |
Delegated to: | David Miller |
Headers | show |
Series | net: arp, ipv6: handle special case of tap device | expand |
From: Vladis Dronov <vdronov@redhat.com> Date: Wed, 26 Sep 2018 11:30:18 +0200 > @@ -187,7 +187,14 @@ EXPORT_SYMBOL(arp_tbl); > > int arp_mc_map(__be32 addr, u8 *haddr, struct net_device *dev, int dir) > { > - switch (dev->type) { > + unsigned short type = dev->type; > + > +#if IS_ENABLED(CONFIG_TAP) > + if (dev->rtnl_link_ops && !strcmp(dev->rtnl_link_ops->kind, "tun")) > + type = ARPHRD_ETHER; > +#endif /* CONFIG_TAP */ This is insanely ugly. dev->type determines the link layer header layout and size. You can fix the kernel, but userspace AF_PACKET applications are still going to be broken by this behavior. And that is just the tip of the iceberg. I'm not applying this, sorry. I think tun/tap should be prevented from allowing the dev->type to be changed, unless it will make those changes adjust the link layer headers properly as well. Sorry.
Hello, David, all, > This is insanely ugly. > I'm not applying this, sorry. It is ugly, I agree. > dev->type determines the link layer header layout and size. Yes, unfortunately, TUNSETLINK ioctl handler does not adjust all the needed fields after the tun->dev->type: [drivers/net/tun.c] case TUNSETLINK: ... tun->dev->type = (int) arg; //<--- that's all Let me share what I see and let me hope to get an advise for a better fix. (also another related thread: https://marc.info/?t=153797194500007&r=1&w=2) - So setting just the tun->dev->type makes the dev struct inconsistent. - There are more field to adjust, at least dev->addr_len and dev->broadcast. - There is no get_addr_len_by_link_type() or a simple way to get link layer properties by dev->type. Such settings are scattered in *_setup and *_init functions, like ipgre_tunnel_init() { ... dev->addr_len = 4; ...} > I think tun/tap should be prevented from > allowing the dev->type to be changed, unless it will make those changes > adjust the link layer headers properly as well. Probably, this functionality is already used by some userspace, so I believe, I cannot just remove TUNSETLINK ioctl. Let me try to suggest a patch that sets dev->addr_len and dev->broadcast (and probably other link-level-related) fields according to the dev->type. Best regards, Vladis Dronov | Red Hat, Inc. | Product Security Engineer
From: Vladis Dronov <vdronov@redhat.com> Date: Mon, 1 Oct 2018 07:32:32 -0400 (EDT) > Probably, this functionality is already used by some userspace, so I > believe, I cannot just remove TUNSETLINK ioctl. Who uses it, how do they use it, and what do they sanely expect about the link layer header properties of a tap device which has had ->type changed?
> From: "David Miller" <davem@davemloft.net> > Sent: Wednesday, October 3, 2018 7:25:40 AM > > > Probably, this functionality is already used by some userspace, so I > > believe, I cannot just remove TUNSETLINK ioctl. > > Who uses it, how do they use it, and what do they sanely expect about > the link layer header properties of a tap device which has had ->type > changed? Honestly, I have no idea. I was working on a syzkaller bug report and found that the reason for that bug was TUNSETLINK ioctl. This ioctl was introduced 13 years ago with ff4cc3ac93e1d and it says: > For use with > wireless and other networking types it should be possible to set the > ARP type via an ioctl. , that's all. Let me try to prepare a patch that sets dev->addr_len and dev->broadcast according to dev->type. I would probably need an advise what other fields must be adjusted too. Otherwise, we can just to remove the TUNSETLINK ioctl. Best regards, Vladis Dronov | Red Hat, Inc. | Product Security Engineer
From: Vladis Dronov <vdronov@redhat.com> Date: Wed, 3 Oct 2018 09:25:12 -0400 (EDT) > Otherwise, we can just to remove the TUNSETLINK ioctl. I bet that is the best solution unless we can find a real user.
Vladis Dronov <vdronov@redhat.com> writes: >> From: "David Miller" <davem@davemloft.net> >> Sent: Wednesday, October 3, 2018 7:25:40 AM >> >> > Probably, this functionality is already used by some userspace, so I >> > believe, I cannot just remove TUNSETLINK ioctl. >> >> Who uses it, how do they use it, and what do they sanely expect about >> the link layer header properties of a tap device which has had ->type >> changed? > > Honestly, I have no idea. I was working on a syzkaller bug report and > found that the reason for that bug was TUNSETLINK ioctl. > > This ioctl was introduced 13 years ago with ff4cc3ac93e1d and it says: > >> For use with >> wireless and other networking types it should be possible to set the >> ARP type via an ioctl. > > , that's all. Let me try to prepare a patch that sets dev->addr_len and > dev->broadcast according to dev->type. I would probably need an advise > what other fields must be adjusted too. > > Otherwise, we can just to remove the TUNSETLINK ioctl. Let's add linux-wireless in case someone there knows about the ioctl.
diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c index e90c89ef8c08..9ce472cf98a3 100644 --- a/net/ipv4/arp.c +++ b/net/ipv4/arp.c @@ -187,7 +187,14 @@ EXPORT_SYMBOL(arp_tbl); int arp_mc_map(__be32 addr, u8 *haddr, struct net_device *dev, int dir) { - switch (dev->type) { + unsigned short type = dev->type; + +#if IS_ENABLED(CONFIG_TAP) + if (dev->rtnl_link_ops && !strcmp(dev->rtnl_link_ops->kind, "tun")) + type = ARPHRD_ETHER; +#endif /* CONFIG_TAP */ + + switch (type) { case ARPHRD_ETHER: case ARPHRD_FDDI: case ARPHRD_IEEE802: diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c index 0ec273997d1d..9371ff4454f7 100644 --- a/net/ipv6/ndisc.c +++ b/net/ipv6/ndisc.c @@ -283,7 +283,14 @@ struct ndisc_options *ndisc_parse_options(const struct net_device *dev, int ndisc_mc_map(const struct in6_addr *addr, char *buf, struct net_device *dev, int dir) { - switch (dev->type) { + unsigned short type = dev->type; + +#if IS_ENABLED(CONFIG_TAP) + if (dev->rtnl_link_ops && !strcmp(dev->rtnl_link_ops->kind, "tun")) + type = ARPHRD_ETHER; +#endif /* CONFIG_TAP */ + + switch (type) { case ARPHRD_ETHER: case ARPHRD_IEEE802: /* Not sure. Check it later. --ANK */ case ARPHRD_FDDI:
dev->type can be set to any value for tun/tap devices by TUNSETLINK ioctl. Nevertheless, all other dev's fields related to the link level like addr_len and broadcast remain the same as configured previously for ARPHRD_ETHER type, making dev inconsistent. This can lead to read from uninitialized memory. Fix this by checking for the case of tun/tap device and act according to the real link level type of dev while mapping a multicast IP onto multicast MAC Reported-by: syzbot+d3402c47f680ff24b29c@syzkaller.appspotmail.com Signed-off-by: Vladis Dronov <vdronov@redhat.com> --- net/ipv4/arp.c | 9 ++++++++- net/ipv6/ndisc.c | 9 ++++++++- 2 files changed, 16 insertions(+), 2 deletions(-)