Message ID | 20170906120208.10561-2-kleber.souza@canonical.com |
---|---|
State | New |
Headers | show |
Series | Fix for CVE-2016-8632 | expand |
On 06.09.2017 14:02, Kleber Sacilotto de Souza wrote: > From: Michal Kubeček <mkubecek@suse.cz> > > Qian Zhang (张谦) reported a potential socket buffer overflow in > tipc_msg_build() which is also known as CVE-2016-8632: due to > insufficient checks, a buffer overflow can occur if MTU is too short for > even tipc headers. As anyone can set device MTU in a user/net namespace, > this issue can be abused by a regular user. > > As agreed in the discussion on Ben Hutchings' original patch, we should > check the MTU at the moment a bearer is attached rather than for each > processed packet. We also need to repeat the check when bearer MTU is > adjusted to new device MTU. UDP case also needs a check to avoid > overflow when calculating bearer MTU. > > Fixes: b97bf3fd8f6a ("[TIPC] Initial merge") > Signed-off-by: Michal Kubecek <mkubecek@suse.cz> > Reported-by: Qian Zhang (张谦) <zhangqian-c@360.cn> > Acked-by: Ying Xue <ying.xue@windriver.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > > CVE-2016-8632 > (backported from commit 3de81b758853f0b29c61e246679d20b513c4cfec) > [kleber: > - Adjust context > - Duplicate macro definitions in bearer.h to avoid mutual inclusion > - Duplicate bearer changes for eth and ib media > - Drop changes in udp_media.c] > Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- Tested and looks sensible with explanation > net/tipc/bearer.h | 16 ++++++++++++++++ > net/tipc/eth_media.c | 11 +++++++++-- > net/tipc/ib_media.c | 11 +++++++++-- > 3 files changed, 34 insertions(+), 4 deletions(-) > > diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h > index e5e04be6fffa..cc8bc3b7cb6f 100644 > --- a/net/tipc/bearer.h > +++ b/net/tipc/bearer.h > @@ -58,6 +58,13 @@ > #define TIPC_MEDIA_TYPE_ETH 1 > #define TIPC_MEDIA_TYPE_IB 2 > > +/* Message header sizes from msg.h - duplicated to avoid mutual inclusion */ > +#define INT_H_SIZE 40 > +#define MAX_H_SIZE 60 > + > +/* minimum bearer MTU */ > +#define TIPC_MIN_BEARER_MTU (MAX_H_SIZE + INT_H_SIZE) > + > /** > * struct tipc_media_addr - destination address used by TIPC bearers > * @value: address info (format defined by media) > @@ -210,4 +217,13 @@ static inline void tipc_bearer_send(struct tipc_bearer *b, struct sk_buff *buf, > b->media->send_msg(buf, b, dest); > } > > +/* check if device MTU is too low for tipc headers */ > +static inline bool tipc_mtu_bad(struct net_device *dev, unsigned int reserve) > +{ > + if (dev->mtu >= TIPC_MIN_BEARER_MTU + reserve) > + return false; > + netdev_warn(dev, "MTU too low for tipc bearer\n"); > + return true; > +} > + > #endif /* _TIPC_BEARER_H */ > diff --git a/net/tipc/eth_media.c b/net/tipc/eth_media.c > index f80d59f5a161..3fe074af15ec 100644 > --- a/net/tipc/eth_media.c > +++ b/net/tipc/eth_media.c > @@ -180,6 +180,10 @@ static int enable_media(struct tipc_bearer *tb_ptr) > dev = dev_get_by_name(&init_net, driver_name); > if (!dev) > return -ENODEV; > + if (tipc_mtu_bad(dev, 0)) { > + dev_put(dev); > + return -EINVAL; > + } > > /* Create Ethernet bearer for device */ > eb_ptr->dev = dev; > @@ -258,8 +262,6 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt, > if (!eb_ptr->bearer) > return NOTIFY_DONE; /* bearer had been disabled */ > > - eb_ptr->bearer->mtu = dev->mtu; > - > switch (evt) { > case NETDEV_CHANGE: > if (netif_carrier_ok(dev)) > @@ -274,6 +276,11 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt, > tipc_block_bearer(eb_ptr->bearer); > break; > case NETDEV_CHANGEMTU: > + if (tipc_mtu_bad(dev, 0)) { > + tipc_disable_bearer(eb_ptr->bearer->name); > + break; > + } > + eb_ptr->bearer->mtu = dev->mtu; > case NETDEV_CHANGEADDR: > tipc_block_bearer(eb_ptr->bearer); > tipc_continue(eb_ptr->bearer); > diff --git a/net/tipc/ib_media.c b/net/tipc/ib_media.c > index c13989297464..c34380b9357c 100644 > --- a/net/tipc/ib_media.c > +++ b/net/tipc/ib_media.c > @@ -173,6 +173,10 @@ static int enable_media(struct tipc_bearer *tb_ptr) > dev = dev_get_by_name(&init_net, driver_name); > if (!dev) > return -ENODEV; > + if (tipc_mtu_bad(dev, 0)) { > + dev_put(dev); > + return -EINVAL; > + } > > /* Create InfiniBand bearer for device */ > ib_ptr->dev = dev; > @@ -251,8 +255,6 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt, > if (!ib_ptr->bearer) > return NOTIFY_DONE; /* bearer had been disabled */ > > - ib_ptr->bearer->mtu = dev->mtu; > - > switch (evt) { > case NETDEV_CHANGE: > if (netif_carrier_ok(dev)) > @@ -267,6 +269,11 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt, > tipc_block_bearer(ib_ptr->bearer); > break; > case NETDEV_CHANGEMTU: > + if (tipc_mtu_bad(dev, 0)) { > + tipc_disable_bearer(ib_ptr->bearer->name); > + break; > + } > + ib_ptr->bearer->mtu = dev->mtu; > case NETDEV_CHANGEADDR: > tipc_block_bearer(ib_ptr->bearer); > tipc_continue(ib_ptr->bearer); >
On 06/09/17 13:02, Kleber Sacilotto de Souza wrote: > From: Michal Kubeček <mkubecek@suse.cz> > > Qian Zhang (张谦) reported a potential socket buffer overflow in > tipc_msg_build() which is also known as CVE-2016-8632: due to > insufficient checks, a buffer overflow can occur if MTU is too short for > even tipc headers. As anyone can set device MTU in a user/net namespace, > this issue can be abused by a regular user. > > As agreed in the discussion on Ben Hutchings' original patch, we should > check the MTU at the moment a bearer is attached rather than for each > processed packet. We also need to repeat the check when bearer MTU is > adjusted to new device MTU. UDP case also needs a check to avoid > overflow when calculating bearer MTU. > > Fixes: b97bf3fd8f6a ("[TIPC] Initial merge") > Signed-off-by: Michal Kubecek <mkubecek@suse.cz> > Reported-by: Qian Zhang (张谦) <zhangqian-c@360.cn> > Acked-by: Ying Xue <ying.xue@windriver.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > > CVE-2016-8632 > (backported from commit 3de81b758853f0b29c61e246679d20b513c4cfec) > [kleber: > - Adjust context > - Duplicate macro definitions in bearer.h to avoid mutual inclusion > - Duplicate bearer changes for eth and ib media > - Drop changes in udp_media.c] > Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> > --- > net/tipc/bearer.h | 16 ++++++++++++++++ > net/tipc/eth_media.c | 11 +++++++++-- > net/tipc/ib_media.c | 11 +++++++++-- > 3 files changed, 34 insertions(+), 4 deletions(-) > > diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h > index e5e04be6fffa..cc8bc3b7cb6f 100644 > --- a/net/tipc/bearer.h > +++ b/net/tipc/bearer.h > @@ -58,6 +58,13 @@ > #define TIPC_MEDIA_TYPE_ETH 1 > #define TIPC_MEDIA_TYPE_IB 2 > > +/* Message header sizes from msg.h - duplicated to avoid mutual inclusion */ > +#define INT_H_SIZE 40 > +#define MAX_H_SIZE 60 > + > +/* minimum bearer MTU */ > +#define TIPC_MIN_BEARER_MTU (MAX_H_SIZE + INT_H_SIZE) > + > /** > * struct tipc_media_addr - destination address used by TIPC bearers > * @value: address info (format defined by media) > @@ -210,4 +217,13 @@ static inline void tipc_bearer_send(struct tipc_bearer *b, struct sk_buff *buf, > b->media->send_msg(buf, b, dest); > } > > +/* check if device MTU is too low for tipc headers */ > +static inline bool tipc_mtu_bad(struct net_device *dev, unsigned int reserve) > +{ > + if (dev->mtu >= TIPC_MIN_BEARER_MTU + reserve) > + return false; > + netdev_warn(dev, "MTU too low for tipc bearer\n"); > + return true; > +} > + > #endif /* _TIPC_BEARER_H */ > diff --git a/net/tipc/eth_media.c b/net/tipc/eth_media.c > index f80d59f5a161..3fe074af15ec 100644 > --- a/net/tipc/eth_media.c > +++ b/net/tipc/eth_media.c > @@ -180,6 +180,10 @@ static int enable_media(struct tipc_bearer *tb_ptr) > dev = dev_get_by_name(&init_net, driver_name); > if (!dev) > return -ENODEV; > + if (tipc_mtu_bad(dev, 0)) { > + dev_put(dev); > + return -EINVAL; > + } > > /* Create Ethernet bearer for device */ > eb_ptr->dev = dev; > @@ -258,8 +262,6 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt, > if (!eb_ptr->bearer) > return NOTIFY_DONE; /* bearer had been disabled */ > > - eb_ptr->bearer->mtu = dev->mtu; > - > switch (evt) { > case NETDEV_CHANGE: > if (netif_carrier_ok(dev)) > @@ -274,6 +276,11 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt, > tipc_block_bearer(eb_ptr->bearer); > break; > case NETDEV_CHANGEMTU: > + if (tipc_mtu_bad(dev, 0)) { > + tipc_disable_bearer(eb_ptr->bearer->name); > + break; > + } > + eb_ptr->bearer->mtu = dev->mtu; > case NETDEV_CHANGEADDR: > tipc_block_bearer(eb_ptr->bearer); > tipc_continue(eb_ptr->bearer); > diff --git a/net/tipc/ib_media.c b/net/tipc/ib_media.c > index c13989297464..c34380b9357c 100644 > --- a/net/tipc/ib_media.c > +++ b/net/tipc/ib_media.c > @@ -173,6 +173,10 @@ static int enable_media(struct tipc_bearer *tb_ptr) > dev = dev_get_by_name(&init_net, driver_name); > if (!dev) > return -ENODEV; > + if (tipc_mtu_bad(dev, 0)) { > + dev_put(dev); > + return -EINVAL; > + } > > /* Create InfiniBand bearer for device */ > ib_ptr->dev = dev; > @@ -251,8 +255,6 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt, > if (!ib_ptr->bearer) > return NOTIFY_DONE; /* bearer had been disabled */ > > - ib_ptr->bearer->mtu = dev->mtu; > - > switch (evt) { > case NETDEV_CHANGE: > if (netif_carrier_ok(dev)) > @@ -267,6 +269,11 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt, > tipc_block_bearer(ib_ptr->bearer); > break; > case NETDEV_CHANGEMTU: > + if (tipc_mtu_bad(dev, 0)) { > + tipc_disable_bearer(ib_ptr->bearer->name); > + break; > + } > + ib_ptr->bearer->mtu = dev->mtu; > case NETDEV_CHANGEADDR: > tipc_block_bearer(ib_ptr->bearer); > tipc_continue(ib_ptr->bearer); > Testing looks good to me. Acked-by: Colin Ian King <colin.king@canonical.com>
Applied to trusty master-next branch. Thanks. Cascardo.
diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h index e5e04be6fffa..cc8bc3b7cb6f 100644 --- a/net/tipc/bearer.h +++ b/net/tipc/bearer.h @@ -58,6 +58,13 @@ #define TIPC_MEDIA_TYPE_ETH 1 #define TIPC_MEDIA_TYPE_IB 2 +/* Message header sizes from msg.h - duplicated to avoid mutual inclusion */ +#define INT_H_SIZE 40 +#define MAX_H_SIZE 60 + +/* minimum bearer MTU */ +#define TIPC_MIN_BEARER_MTU (MAX_H_SIZE + INT_H_SIZE) + /** * struct tipc_media_addr - destination address used by TIPC bearers * @value: address info (format defined by media) @@ -210,4 +217,13 @@ static inline void tipc_bearer_send(struct tipc_bearer *b, struct sk_buff *buf, b->media->send_msg(buf, b, dest); } +/* check if device MTU is too low for tipc headers */ +static inline bool tipc_mtu_bad(struct net_device *dev, unsigned int reserve) +{ + if (dev->mtu >= TIPC_MIN_BEARER_MTU + reserve) + return false; + netdev_warn(dev, "MTU too low for tipc bearer\n"); + return true; +} + #endif /* _TIPC_BEARER_H */ diff --git a/net/tipc/eth_media.c b/net/tipc/eth_media.c index f80d59f5a161..3fe074af15ec 100644 --- a/net/tipc/eth_media.c +++ b/net/tipc/eth_media.c @@ -180,6 +180,10 @@ static int enable_media(struct tipc_bearer *tb_ptr) dev = dev_get_by_name(&init_net, driver_name); if (!dev) return -ENODEV; + if (tipc_mtu_bad(dev, 0)) { + dev_put(dev); + return -EINVAL; + } /* Create Ethernet bearer for device */ eb_ptr->dev = dev; @@ -258,8 +262,6 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt, if (!eb_ptr->bearer) return NOTIFY_DONE; /* bearer had been disabled */ - eb_ptr->bearer->mtu = dev->mtu; - switch (evt) { case NETDEV_CHANGE: if (netif_carrier_ok(dev)) @@ -274,6 +276,11 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt, tipc_block_bearer(eb_ptr->bearer); break; case NETDEV_CHANGEMTU: + if (tipc_mtu_bad(dev, 0)) { + tipc_disable_bearer(eb_ptr->bearer->name); + break; + } + eb_ptr->bearer->mtu = dev->mtu; case NETDEV_CHANGEADDR: tipc_block_bearer(eb_ptr->bearer); tipc_continue(eb_ptr->bearer); diff --git a/net/tipc/ib_media.c b/net/tipc/ib_media.c index c13989297464..c34380b9357c 100644 --- a/net/tipc/ib_media.c +++ b/net/tipc/ib_media.c @@ -173,6 +173,10 @@ static int enable_media(struct tipc_bearer *tb_ptr) dev = dev_get_by_name(&init_net, driver_name); if (!dev) return -ENODEV; + if (tipc_mtu_bad(dev, 0)) { + dev_put(dev); + return -EINVAL; + } /* Create InfiniBand bearer for device */ ib_ptr->dev = dev; @@ -251,8 +255,6 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt, if (!ib_ptr->bearer) return NOTIFY_DONE; /* bearer had been disabled */ - ib_ptr->bearer->mtu = dev->mtu; - switch (evt) { case NETDEV_CHANGE: if (netif_carrier_ok(dev)) @@ -267,6 +269,11 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt, tipc_block_bearer(ib_ptr->bearer); break; case NETDEV_CHANGEMTU: + if (tipc_mtu_bad(dev, 0)) { + tipc_disable_bearer(ib_ptr->bearer->name); + break; + } + ib_ptr->bearer->mtu = dev->mtu; case NETDEV_CHANGEADDR: tipc_block_bearer(ib_ptr->bearer); tipc_continue(ib_ptr->bearer);