| Message ID | 1493183003-884-4-git-send-email-xiyou.wangcong@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Wed, Apr 26, 2017 at 07:03:23AM CEST, xiyou.wangcong@gmail.com wrote: >IPv6 tunnels use sizeof(struct in6_addr) as dev->addr_len, >but in many places especially bonding, we use struct sockaddr >to copy and set mac addr, this could lead to stack out-of-bounds >access. > >Fix it by using a larger address storage. > >Reported-by: Andrey Konovalov <andreyknvl@google.com> >Cc: Jiri Pirko <jiri@resnulli.us> >Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> >--- > drivers/net/team/team.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > >diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c >index 85c0124..88878f1 100644 >--- a/drivers/net/team/team.c >+++ b/drivers/net/team/team.c >@@ -60,10 +60,13 @@ static struct team_port *team_port_get_rtnl(const struct net_device *dev) > static int __set_port_dev_addr(struct net_device *port_dev, > const unsigned char *dev_addr) > { >- struct sockaddr addr; >+ struct { >+ unsigned short type; >+ unsigned char addr[MAX_ADDR_LEN]; >+ } addr; Wouldn't it make sense to define this struct somewhere in the core headers? > >- memcpy(addr.sa_data, dev_addr, port_dev->addr_len); >- addr.sa_family = port_dev->type; >+ memcpy(addr.addr, dev_addr, port_dev->addr_len); >+ addr.type = port_dev->type; > return dev_set_mac_address(port_dev, &addr); > } > >-- >2.5.5 >
On 2017-04-26 1:40 AM, Jiri Pirko wrote: > Wed, Apr 26, 2017 at 07:03:23AM CEST, xiyou.wangcong@gmail.com wrote: >> IPv6 tunnels use sizeof(struct in6_addr) as dev->addr_len, >> but in many places especially bonding, we use struct sockaddr >> to copy and set mac addr, this could lead to stack out-of-bounds >> access. >> >> Fix it by using a larger address storage. >> >> Reported-by: Andrey Konovalov <andreyknvl@google.com> >> Cc: Jiri Pirko <jiri@resnulli.us> >> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> >> --- >> drivers/net/team/team.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c >> index 85c0124..88878f1 100644 >> --- a/drivers/net/team/team.c >> +++ b/drivers/net/team/team.c >> @@ -60,10 +60,13 @@ static struct team_port *team_port_get_rtnl(const struct net_device *dev) >> static int __set_port_dev_addr(struct net_device *port_dev, >> const unsigned char *dev_addr) >> { >> - struct sockaddr addr; >> + struct { >> + unsigned short type; >> + unsigned char addr[MAX_ADDR_LEN]; >> + } addr; > > Wouldn't it make sense to define this struct somewhere in the core > headers? We already have struct sockaddr_storage that could be used throughout this set as well. We just converted a few pieces of the bonding driver over to using it for better support of ipoib bonds, via commit faeeb317a5615076dff1ff44b51e862e6064dbd0. Might be better to just use that in both bonding and team, rather than having different per-driver structs, or Yet Another Address Storage implementation.
On Tue, Apr 25, 2017 at 10:40 PM, Jiri Pirko <jiri@resnulli.us> wrote: > Wed, Apr 26, 2017 at 07:03:23AM CEST, xiyou.wangcong@gmail.com wrote: >>IPv6 tunnels use sizeof(struct in6_addr) as dev->addr_len, >>but in many places especially bonding, we use struct sockaddr >>to copy and set mac addr, this could lead to stack out-of-bounds >>access. >> >>Fix it by using a larger address storage. >> >>Reported-by: Andrey Konovalov <andreyknvl@google.com> >>Cc: Jiri Pirko <jiri@resnulli.us> >>Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> >>--- >> drivers/net/team/team.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >>diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c >>index 85c0124..88878f1 100644 >>--- a/drivers/net/team/team.c >>+++ b/drivers/net/team/team.c >>@@ -60,10 +60,13 @@ static struct team_port *team_port_get_rtnl(const struct net_device *dev) >> static int __set_port_dev_addr(struct net_device *port_dev, >> const unsigned char *dev_addr) >> { >>- struct sockaddr addr; >>+ struct { >>+ unsigned short type; >>+ unsigned char addr[MAX_ADDR_LEN]; >>+ } addr; > > Wouldn't it make sense to define this struct somewhere in the core > headers? I _did_ use a struct mac_addr until I found there are multiple places in the tree already defining it... We are in a similar situation to the union of struct in_addr and struct in6_addr, unfortunately. We can always clean up these for net-next.
On Wed, Apr 26, 2017 at 8:55 AM, Jarod Wilson <jarod@redhat.com> wrote: > > We already have struct sockaddr_storage that could be used throughout this > set as well. We just converted a few pieces of the bonding driver over to > using it for better support of ipoib bonds, via commit > faeeb317a5615076dff1ff44b51e862e6064dbd0. Might be better to just use that > in both bonding and team, rather than having different per-driver structs, > or Yet Another Address Storage implementation. Technically, struct sockaddr_storage is not enough either, given the max is MAX_ADDR_LEN. This is why I gave up on sockaddr_storage.
On 2017-04-26 12:11 PM, Cong Wang wrote: > On Wed, Apr 26, 2017 at 8:55 AM, Jarod Wilson <jarod@redhat.com> wrote: >> >> We already have struct sockaddr_storage that could be used throughout this >> set as well. We just converted a few pieces of the bonding driver over to >> using it for better support of ipoib bonds, via commit >> faeeb317a5615076dff1ff44b51e862e6064dbd0. Might be better to just use that >> in both bonding and team, rather than having different per-driver structs, >> or Yet Another Address Storage implementation. > > Technically, struct sockaddr_storage is not enough either, given the > max is MAX_ADDR_LEN. This is why I gave up on sockaddr_storage. Wait, what? Am I missing something? MAX_ADDR_LEN is 32, and sockaddr_storage is a #define for __kernel_sockaddr_storage, which has it's __data member defined as being of size 128 - sizeof(unsigned short).
On Wed, Apr 26, 2017 at 9:46 AM, Jarod Wilson <jarod@redhat.com> wrote: > On 2017-04-26 12:11 PM, Cong Wang wrote: >> >> On Wed, Apr 26, 2017 at 8:55 AM, Jarod Wilson <jarod@redhat.com> wrote: >>> >>> >>> We already have struct sockaddr_storage that could be used throughout >>> this >>> set as well. We just converted a few pieces of the bonding driver over to >>> using it for better support of ipoib bonds, via commit >>> faeeb317a5615076dff1ff44b51e862e6064dbd0. Might be better to just use >>> that >>> in both bonding and team, rather than having different per-driver >>> structs, >>> or Yet Another Address Storage implementation. >> >> >> Technically, struct sockaddr_storage is not enough either, given the >> max is MAX_ADDR_LEN. This is why I gave up on sockaddr_storage. > > > Wait, what? Am I missing something? MAX_ADDR_LEN is 32, and sockaddr_storage > is a #define for __kernel_sockaddr_storage, which has it's __data member > defined as being of size 128 - sizeof(unsigned short). My bad, I thought it is same with sizeof(in6addr) without looking into it. The question is, why do we waste 126 - 32 = 94 bytes on stack to just use struct sockaddr_storage? I totally understand we want a unified struct, but we already redefine it in multiple places in tree...
On 2017-04-26 1:28 PM, Cong Wang wrote: > On Wed, Apr 26, 2017 at 9:46 AM, Jarod Wilson <jarod@redhat.com> wrote: >> On 2017-04-26 12:11 PM, Cong Wang wrote: >>> >>> On Wed, Apr 26, 2017 at 8:55 AM, Jarod Wilson <jarod@redhat.com> wrote: >>>> >>>> >>>> We already have struct sockaddr_storage that could be used throughout >>>> this >>>> set as well. We just converted a few pieces of the bonding driver over to >>>> using it for better support of ipoib bonds, via commit >>>> faeeb317a5615076dff1ff44b51e862e6064dbd0. Might be better to just use >>>> that >>>> in both bonding and team, rather than having different per-driver >>>> structs, >>>> or Yet Another Address Storage implementation. >>> >>> >>> Technically, struct sockaddr_storage is not enough either, given the >>> max is MAX_ADDR_LEN. This is why I gave up on sockaddr_storage. >> >> >> Wait, what? Am I missing something? MAX_ADDR_LEN is 32, and sockaddr_storage >> is a #define for __kernel_sockaddr_storage, which has it's __data member >> defined as being of size 128 - sizeof(unsigned short). > > My bad, I thought it is same with sizeof(in6addr) without looking into it. > The question is, why do we waste 126 - 32 = 94 bytes on stack to just > use struct sockaddr_storage? That's a fair point. > I totally understand we want a unified struct, but we already redefine > it in multiple places in tree... Something unified and centralized with a data storage of MAX_ADDR_LEN does seem reasonable to get both consistency and minimized waste, and I could certainly do a follow-up patch for the bonding driver to switch the bits now using sockaddr_storage over to whatever new struct gets added.
diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c index 85c0124..88878f1 100644 --- a/drivers/net/team/team.c +++ b/drivers/net/team/team.c @@ -60,10 +60,13 @@ static struct team_port *team_port_get_rtnl(const struct net_device *dev) static int __set_port_dev_addr(struct net_device *port_dev, const unsigned char *dev_addr) { - struct sockaddr addr; + struct { + unsigned short type; + unsigned char addr[MAX_ADDR_LEN]; + } addr; - memcpy(addr.sa_data, dev_addr, port_dev->addr_len); - addr.sa_family = port_dev->type; + memcpy(addr.addr, dev_addr, port_dev->addr_len); + addr.type = port_dev->type; return dev_set_mac_address(port_dev, &addr); }
IPv6 tunnels use sizeof(struct in6_addr) as dev->addr_len, but in many places especially bonding, we use struct sockaddr to copy and set mac addr, this could lead to stack out-of-bounds access. Fix it by using a larger address storage. Reported-by: Andrey Konovalov <andreyknvl@google.com> Cc: Jiri Pirko <jiri@resnulli.us> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> --- drivers/net/team/team.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)