| Message ID | 20221206070857.2453919-1-v.v.mitrofanov@yadro.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | net: ipv6: Fix link-partner MAC address assignment | expand |
On 12/6/22 08:08, Viacheslav Mitrofanov wrote: > MAC address of a link-partner is not saved for future use because of > bad condition of if statement. Moreover it can potentially cause to > NULL-pointer dereference. > > Signed-off-by: Viacheslav Mitrofanov <v.v.mitrofanov@yadro.com> > --- > net/ndisc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Reviewed-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> > diff --git a/net/ndisc.c b/net/ndisc.c > index 3c0eeeaea3..56fc6390bc 100644 > --- a/net/ndisc.c > +++ b/net/ndisc.c > @@ -264,7 +264,7 @@ int ndisc_receive(struct ethernet_hdr *et, struct ip6_hdr *ip6, int len) > ndisc_extract_enetaddr(ndisc, neigh_eth_addr); > > /* save address for later use */ > - if (!net_nd_packet_mac) > + if (net_nd_packet_mac) > memcpy(net_nd_packet_mac, neigh_eth_addr, 7); > > /* modify header, and transmit it */
On Tue, 2022-12-06 at 13:22 +0100, Daniel Schwierzeck wrote: > «Внимание! Данное письмо от внешнего адресата!» > > On 12/6/22 08:08, Viacheslav Mitrofanov wrote: > > MAC address of a link-partner is not saved for future use because > > of > > bad condition of if statement. Moreover it can potentially cause to > > NULL-pointer dereference. > > > > Signed-off-by: Viacheslav Mitrofanov <v.v.mitrofanov@yadro.com> > > --- > > net/ndisc.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > Reviewed-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> > > > diff --git a/net/ndisc.c b/net/ndisc.c > > index 3c0eeeaea3..56fc6390bc 100644 > > --- a/net/ndisc.c > > +++ b/net/ndisc.c > > @@ -264,7 +264,7 @@ int ndisc_receive(struct ethernet_hdr *et, > > struct ip6_hdr *ip6, int len) > > ndisc_extract_enetaddr(ndisc, > > neigh_eth_addr); > > > > /* save address for later use */ > > - if (!net_nd_packet_mac) > > + if (net_nd_packet_mac) > > memcpy(net_nd_packet_mac, > > neigh_eth_addr, 7); > > > > /* modify header, and transmit it */ > > -- > - Daniel > This patch is not appropriate!net_nd_packet_mac is just a pointer, moreover there is no memory allocation. It has just keep a pointer to neigh_eth_addr. So the solution must be sth. like net_nd_packet_mac = neigh_eth_addr;
On 12/7/22 12:25, Vyacheslav Mitrofanov V wrote: > On Tue, 2022-12-06 at 13:22 +0100, Daniel Schwierzeck wrote: >> «Внимание! Данное письмо от внешнего адресата!» >> >> On 12/6/22 08:08, Viacheslav Mitrofanov wrote: >>> MAC address of a link-partner is not saved for future use because >>> of >>> bad condition of if statement. Moreover it can potentially cause to >>> NULL-pointer dereference. >>> >>> Signed-off-by: Viacheslav Mitrofanov <v.v.mitrofanov@yadro.com> >>> --- >>> net/ndisc.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >> >> Reviewed-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> >> >>> diff --git a/net/ndisc.c b/net/ndisc.c >>> index 3c0eeeaea3..56fc6390bc 100644 >>> --- a/net/ndisc.c >>> +++ b/net/ndisc.c >>> @@ -264,7 +264,7 @@ int ndisc_receive(struct ethernet_hdr *et, >>> struct ip6_hdr *ip6, int len) >>> ndisc_extract_enetaddr(ndisc, >>> neigh_eth_addr); >>> >>> /* save address for later use */ >>> - if (!net_nd_packet_mac) >>> + if (net_nd_packet_mac) >>> memcpy(net_nd_packet_mac, >>> neigh_eth_addr, 7); >>> >>> /* modify header, and transmit it */ >> >> -- >> - Daniel >> > This patch is not appropriate!net_nd_packet_mac is just a pointer, > moreover there is no memory allocation. It has just keep a pointer to > neigh_eth_addr. > So the solution must be sth. like net_nd_packet_mac = neigh_eth_addr; that wouldn't make much sense. You would assign a global pointer to an array defined in local function scope. After ndisc_receive() has finished, the pointer will have an invalid address. The same problem exists in ping6_send(). Also the pointer is assigned in net_send_udp_packet6() to an array so the memcpy in ndisc_receive() would work. BTW: the same approach is used with IPv4 and the arp_wait_packet_ethaddr pointer. The pointer is assigned in net_send_ip_packet() to an array and arp_receive() does a memcpy to that pointer if it is not NULL. I think this patch is correct but you should revisit the assignment of net_nd_packet_mac in ping6_send(). You copy net_null_ethaddr to the local mac array and assign net_nd_packet_mac to the mac array. Either the assignment is useless or it should be a memcpy too.
diff --git a/net/ndisc.c b/net/ndisc.c index 3c0eeeaea3..56fc6390bc 100644 --- a/net/ndisc.c +++ b/net/ndisc.c @@ -264,7 +264,7 @@ int ndisc_receive(struct ethernet_hdr *et, struct ip6_hdr *ip6, int len) ndisc_extract_enetaddr(ndisc, neigh_eth_addr); /* save address for later use */ - if (!net_nd_packet_mac) + if (net_nd_packet_mac) memcpy(net_nd_packet_mac, neigh_eth_addr, 7); /* modify header, and transmit it */
MAC address of a link-partner is not saved for future use because of bad condition of if statement. Moreover it can potentially cause to NULL-pointer dereference. Signed-off-by: Viacheslav Mitrofanov <v.v.mitrofanov@yadro.com> --- net/ndisc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)