| Message ID | 1460023795-27136-2-git-send-email-thuth@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
Thomas Huth <thuth@redhat.com> writes: > The code in send_ipv6() currently basically looks like this: > > uint8_t *mac_addr, mac[6]; > mac_addr = mac; > ... > n = find_neighbor (&ip_dst); > if (ip6_is_multicast (&ip_dst)) { > mac_addr = ip6_to_multicast_mac (&ip_dst, mac); > } > else { > // Check if the MAC address is already cached > if (n) { > if (memcmp(n->mac, null_mac, ETH_ALEN) != 0) > memcpy (mac_addr, &(n->mac), ETH_ALEN); > /* XXX */ > } > ... > } > ... > fill_ethhdr (n->eth_frame, htons(ETHERTYPE_IPv6), get_mac_address(), > mac_addr); > > That means mac_addr initially points to the uninitialized mac[6] > array on the stack. Now if there was already an entry in the neighbor > cache, but the MAC address has not been determined yet, that > uninitialized array could be used as final MAC address for fill_ethhdr() > (since there is no "else" path at the spot marked with XXX above), > resulting in random data in the MAC address field of the ethernet packet. > > Let's fix this issue by letting mac_addr point to the null_mac by > default instead, so that it never points to invalid data. Also > rename mac[6] to mc_mac[6] to make it clear that this array is > only used for storing the multicast mac address. > > Signed-off-by: Thomas Huth <thuth@redhat.com> Reviewed-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> > --- > clients/net-snk/app/netlib/ipv6.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/clients/net-snk/app/netlib/ipv6.c b/clients/net-snk/app/netlib/ipv6.c > index 348e79d..5a307ca 100644 > --- a/clients/net-snk/app/netlib/ipv6.c > +++ b/clients/net-snk/app/netlib/ipv6.c > @@ -491,9 +491,9 @@ int send_ipv6(int fd, void* buffer, int len) > struct udphdr *udph; > struct icmp6hdr *icmp6h; > ip6_addr_t ip_dst; > - uint8_t *mac_addr, mac[6]; > + uint8_t *mac_addr, mc_mac[6]; > > - mac_addr = mac; > + mac_addr = null_mac; > > ip6h = (struct ip6hdr *) buffer; > udph = (struct udphdr *) ((uint8_t *) ip6h + sizeof (struct ip6hdr)); > @@ -526,13 +526,13 @@ int send_ipv6(int fd, void* buffer, int len) > > // If address is a multicast address, create a proper mac address > if (ip6_is_multicast (&ip_dst)) { > - mac_addr = ip6_to_multicast_mac (&ip_dst, mac); > + mac_addr = ip6_to_multicast_mac (&ip_dst, mc_mac); > } > else { > // Check if the MAC address is already cached > if (n) { > if (memcmp(n->mac, null_mac, ETH_ALEN) != 0) > - memcpy (mac_addr, &(n->mac), ETH_ALEN); /* found it */ > + mac_addr = n->mac; /* found it */ > } else if (!is_ip6addr_in_my_net(&ip_dst)) { > struct router *gw; > gw = ipv6_get_default_router(&ip6h->src); > -- > 1.8.3.1
On 04/07/2016 08:09 PM, Thomas Huth wrote: > The code in send_ipv6() currently basically looks like this: > > uint8_t *mac_addr, mac[6]; > mac_addr = mac; > ... > n = find_neighbor (&ip_dst); > if (ip6_is_multicast (&ip_dst)) { > mac_addr = ip6_to_multicast_mac (&ip_dst, mac); > } > else { > // Check if the MAC address is already cached > if (n) { > if (memcmp(n->mac, null_mac, ETH_ALEN) != 0) > memcpy (mac_addr, &(n->mac), ETH_ALEN); > /* XXX */ > } > ... > } > ... > fill_ethhdr (n->eth_frame, htons(ETHERTYPE_IPv6), get_mac_address(), > mac_addr); > > That means mac_addr initially points to the uninitialized mac[6] > array on the stack. Now if there was already an entry in the neighbor > cache, but the MAC address has not been determined yet, that > uninitialized array could be used as final MAC address for fill_ethhdr() > (since there is no "else" path at the spot marked with XXX above), > resulting in random data in the MAC address field of the ethernet packet. > > Let's fix this issue by letting mac_addr point to the null_mac by > default instead, so that it never points to invalid data. Also > rename mac[6] to mc_mac[6] to make it clear that this array is > only used for storing the multicast mac address. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > clients/net-snk/app/netlib/ipv6.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/clients/net-snk/app/netlib/ipv6.c b/clients/net-snk/app/netlib/ipv6.c > index 348e79d..5a307ca 100644 > --- a/clients/net-snk/app/netlib/ipv6.c > +++ b/clients/net-snk/app/netlib/ipv6.c > @@ -491,9 +491,9 @@ int send_ipv6(int fd, void* buffer, int len) > struct udphdr *udph; > struct icmp6hdr *icmp6h; > ip6_addr_t ip_dst; > - uint8_t *mac_addr, mac[6]; > + uint8_t *mac_addr, mc_mac[6]; > > - mac_addr = mac; > + mac_addr = null_mac; > > ip6h = (struct ip6hdr *) buffer; > udph = (struct udphdr *) ((uint8_t *) ip6h + sizeof (struct ip6hdr)); > @@ -526,13 +526,13 @@ int send_ipv6(int fd, void* buffer, int len) > > // If address is a multicast address, create a proper mac address > if (ip6_is_multicast (&ip_dst)) { > - mac_addr = ip6_to_multicast_mac (&ip_dst, mac); > + mac_addr = ip6_to_multicast_mac (&ip_dst, mc_mac); > } > else { > // Check if the MAC address is already cached > if (n) { > if (memcmp(n->mac, null_mac, ETH_ALEN) != 0) > - memcpy (mac_addr, &(n->mac), ETH_ALEN); /* found it */ > + mac_addr = n->mac; /* found it */ btw there is another thing - sometime the check if mac is NULL is done by "if (mac_addr == null_mac)", sometime by memcmp, can we unify this? > } else if (!is_ip6addr_in_my_net(&ip_dst)) { > struct router *gw; > gw = ipv6_get_default_router(&ip6h->src); >
On 12.04.2016 05:43, Alexey Kardashevskiy wrote: > On 04/07/2016 08:09 PM, Thomas Huth wrote: >> The code in send_ipv6() currently basically looks like this: >> >> uint8_t *mac_addr, mac[6]; >> mac_addr = mac; >> ... >> n = find_neighbor (&ip_dst); >> if (ip6_is_multicast (&ip_dst)) { >> mac_addr = ip6_to_multicast_mac (&ip_dst, mac); >> } >> else { >> // Check if the MAC address is already cached >> if (n) { >> if (memcmp(n->mac, null_mac, ETH_ALEN) != 0) >> memcpy (mac_addr, &(n->mac), ETH_ALEN); >> /* XXX */ >> } >> ... >> } >> ... >> fill_ethhdr (n->eth_frame, htons(ETHERTYPE_IPv6), get_mac_address(), >> mac_addr); >> >> That means mac_addr initially points to the uninitialized mac[6] >> array on the stack. Now if there was already an entry in the neighbor >> cache, but the MAC address has not been determined yet, that >> uninitialized array could be used as final MAC address for fill_ethhdr() >> (since there is no "else" path at the spot marked with XXX above), >> resulting in random data in the MAC address field of the ethernet packet. >> >> Let's fix this issue by letting mac_addr point to the null_mac by >> default instead, so that it never points to invalid data. Also >> rename mac[6] to mc_mac[6] to make it clear that this array is >> only used for storing the multicast mac address. >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> clients/net-snk/app/netlib/ipv6.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/clients/net-snk/app/netlib/ipv6.c >> b/clients/net-snk/app/netlib/ipv6.c >> index 348e79d..5a307ca 100644 >> --- a/clients/net-snk/app/netlib/ipv6.c >> +++ b/clients/net-snk/app/netlib/ipv6.c >> @@ -491,9 +491,9 @@ int send_ipv6(int fd, void* buffer, int len) >> struct udphdr *udph; >> struct icmp6hdr *icmp6h; >> ip6_addr_t ip_dst; >> - uint8_t *mac_addr, mac[6]; >> + uint8_t *mac_addr, mc_mac[6]; >> >> - mac_addr = mac; >> + mac_addr = null_mac; >> >> ip6h = (struct ip6hdr *) buffer; >> udph = (struct udphdr *) ((uint8_t *) ip6h + sizeof (struct >> ip6hdr)); >> @@ -526,13 +526,13 @@ int send_ipv6(int fd, void* buffer, int len) >> >> // If address is a multicast address, create a proper mac address >> if (ip6_is_multicast (&ip_dst)) { >> - mac_addr = ip6_to_multicast_mac (&ip_dst, mac); >> + mac_addr = ip6_to_multicast_mac (&ip_dst, mc_mac); >> } >> else { >> // Check if the MAC address is already cached >> if (n) { >> if (memcmp(n->mac, null_mac, ETH_ALEN) != 0) >> - memcpy (mac_addr, &(n->mac), ETH_ALEN); /* found it */ >> + mac_addr = n->mac; /* found it */ > > > btw there is another thing - sometime the check if mac is NULL is done > by "if (mac_addr == null_mac)", sometime by memcmp, can we unify this? The mac_addr == null_mac checks are of course only useful for the case where mac_addr has been set to null_mac in this function before. The memcmp is needed in the above chunk since it checks n->mac instead. So if you want to unify all checks, I think you have to convert them to memcmp(), but it won't work the other way round. Thomas
On 04/12/2016 05:05 PM, Thomas Huth wrote: > On 12.04.2016 05:43, Alexey Kardashevskiy wrote: >> On 04/07/2016 08:09 PM, Thomas Huth wrote: >>> The code in send_ipv6() currently basically looks like this: >>> >>> uint8_t *mac_addr, mac[6]; >>> mac_addr = mac; >>> ... >>> n = find_neighbor (&ip_dst); >>> if (ip6_is_multicast (&ip_dst)) { >>> mac_addr = ip6_to_multicast_mac (&ip_dst, mac); >>> } >>> else { >>> // Check if the MAC address is already cached >>> if (n) { >>> if (memcmp(n->mac, null_mac, ETH_ALEN) != 0) >>> memcpy (mac_addr, &(n->mac), ETH_ALEN); >>> /* XXX */ >>> } >>> ... >>> } >>> ... >>> fill_ethhdr (n->eth_frame, htons(ETHERTYPE_IPv6), get_mac_address(), >>> mac_addr); >>> >>> That means mac_addr initially points to the uninitialized mac[6] >>> array on the stack. Now if there was already an entry in the neighbor >>> cache, but the MAC address has not been determined yet, that >>> uninitialized array could be used as final MAC address for fill_ethhdr() >>> (since there is no "else" path at the spot marked with XXX above), >>> resulting in random data in the MAC address field of the ethernet packet. >>> >>> Let's fix this issue by letting mac_addr point to the null_mac by >>> default instead, so that it never points to invalid data. Also >>> rename mac[6] to mc_mac[6] to make it clear that this array is >>> only used for storing the multicast mac address. >>> >>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> --- >>> clients/net-snk/app/netlib/ipv6.c | 8 ++++---- >>> 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/clients/net-snk/app/netlib/ipv6.c >>> b/clients/net-snk/app/netlib/ipv6.c >>> index 348e79d..5a307ca 100644 >>> --- a/clients/net-snk/app/netlib/ipv6.c >>> +++ b/clients/net-snk/app/netlib/ipv6.c >>> @@ -491,9 +491,9 @@ int send_ipv6(int fd, void* buffer, int len) >>> struct udphdr *udph; >>> struct icmp6hdr *icmp6h; >>> ip6_addr_t ip_dst; >>> - uint8_t *mac_addr, mac[6]; >>> + uint8_t *mac_addr, mc_mac[6]; >>> >>> - mac_addr = mac; >>> + mac_addr = null_mac; >>> >>> ip6h = (struct ip6hdr *) buffer; >>> udph = (struct udphdr *) ((uint8_t *) ip6h + sizeof (struct >>> ip6hdr)); >>> @@ -526,13 +526,13 @@ int send_ipv6(int fd, void* buffer, int len) >>> >>> // If address is a multicast address, create a proper mac address >>> if (ip6_is_multicast (&ip_dst)) { >>> - mac_addr = ip6_to_multicast_mac (&ip_dst, mac); >>> + mac_addr = ip6_to_multicast_mac (&ip_dst, mc_mac); >>> } >>> else { >>> // Check if the MAC address is already cached >>> if (n) { >>> if (memcmp(n->mac, null_mac, ETH_ALEN) != 0) >>> - memcpy (mac_addr, &(n->mac), ETH_ALEN); /* found it */ >>> + mac_addr = n->mac; /* found it */ >> >> >> btw there is another thing - sometime the check if mac is NULL is done >> by "if (mac_addr == null_mac)", sometime by memcmp, can we unify this? > > The mac_addr == null_mac checks are of course only useful for the case > where mac_addr has been set to null_mac in this function before. I am just reading the resulting code and afaict you could return from send_ipv6() right where you assign null_mac to mac_addr; otherwise I get impression that n->mac can possibly point to null_mac or ip6_to_multicast_mac() can return null_mac (but neither can). > The > memcmp is needed in the above chunk since it checks n->mac instead. So > if you want to unify all checks, I think you have to convert them to > memcmp(), but it won't work the other way round. Sure, memcmp() is the way to go.
diff --git a/clients/net-snk/app/netlib/ipv6.c b/clients/net-snk/app/netlib/ipv6.c index 348e79d..5a307ca 100644 --- a/clients/net-snk/app/netlib/ipv6.c +++ b/clients/net-snk/app/netlib/ipv6.c @@ -491,9 +491,9 @@ int send_ipv6(int fd, void* buffer, int len) struct udphdr *udph; struct icmp6hdr *icmp6h; ip6_addr_t ip_dst; - uint8_t *mac_addr, mac[6]; + uint8_t *mac_addr, mc_mac[6]; - mac_addr = mac; + mac_addr = null_mac; ip6h = (struct ip6hdr *) buffer; udph = (struct udphdr *) ((uint8_t *) ip6h + sizeof (struct ip6hdr)); @@ -526,13 +526,13 @@ int send_ipv6(int fd, void* buffer, int len) // If address is a multicast address, create a proper mac address if (ip6_is_multicast (&ip_dst)) { - mac_addr = ip6_to_multicast_mac (&ip_dst, mac); + mac_addr = ip6_to_multicast_mac (&ip_dst, mc_mac); } else { // Check if the MAC address is already cached if (n) { if (memcmp(n->mac, null_mac, ETH_ALEN) != 0) - memcpy (mac_addr, &(n->mac), ETH_ALEN); /* found it */ + mac_addr = n->mac; /* found it */ } else if (!is_ip6addr_in_my_net(&ip_dst)) { struct router *gw; gw = ipv6_get_default_router(&ip6h->src);
The code in send_ipv6() currently basically looks like this: uint8_t *mac_addr, mac[6]; mac_addr = mac; ... n = find_neighbor (&ip_dst); if (ip6_is_multicast (&ip_dst)) { mac_addr = ip6_to_multicast_mac (&ip_dst, mac); } else { // Check if the MAC address is already cached if (n) { if (memcmp(n->mac, null_mac, ETH_ALEN) != 0) memcpy (mac_addr, &(n->mac), ETH_ALEN); /* XXX */ } ... } ... fill_ethhdr (n->eth_frame, htons(ETHERTYPE_IPv6), get_mac_address(), mac_addr); That means mac_addr initially points to the uninitialized mac[6] array on the stack. Now if there was already an entry in the neighbor cache, but the MAC address has not been determined yet, that uninitialized array could be used as final MAC address for fill_ethhdr() (since there is no "else" path at the spot marked with XXX above), resulting in random data in the MAC address field of the ethernet packet. Let's fix this issue by letting mac_addr point to the null_mac by default instead, so that it never points to invalid data. Also rename mac[6] to mc_mac[6] to make it clear that this array is only used for storing the multicast mac address. Signed-off-by: Thomas Huth <thuth@redhat.com> --- clients/net-snk/app/netlib/ipv6.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)