On Saturday, 29 December 2018 04.10.40 CET Linus Lüssing wrote:
+#define BATADV_DHCP_YIADDR_LEN sizeof(((struct batadv_dhcp_packet *)0)->chaddr) +#define BATADV_DHCP_CHADDR_LEN sizeof(((struct batadv_dhcp_packet *)0)->chaddr)
Why do you use chaddr to calculate the size of yiaddr? yiaddr is only 4 byte and chaddr is 16 byte. Looks to me like you have a potential stack overflow in batadv_dat_dhcp_get_yiaddr because of that.
And you have a lot of code which calls skb_header_pointer again and again + a lot of offset calculations. So it has to potentially traverse all the fragments again and again. I have seen:
* op * htype * hlen * magic * TLV options parts * yiaddr * chaddr
Do you see any potential in combining some of them in batadv_dat_check_dhcp (e.g. op, htype, hlen)?
+/**
- batadv_dat_check_dhcp_ack() - examine packet for valid DHCP message
- @skb: the packet to check
- @proto: ethernet protocol hint (behind a potential vlan)
- @ip_src: a buffer to store the IPv4 source address in
- @chaddr: a buffer to store the DHCP Client Hardware Address in
- @yiaddr: a buffer to store the DHCP Your IP Address in
- Checks whether the given skb is a valid DHCPACK. And if so, stores the
- IPv4 server source address (ip_src), client MAC address (chaddr) and client
- IPv4 address (yiaddr) in the provided buffers.
- Caller needs to ensure that the skb network header is set correctly.
- Return: True if the skb is a valid DHCPACK. False otherwise.
- */
There seems to be a missing "*" in the line before the line "Caller needs to ensure that the skb network header"
Missing header in distributed-arp-table.c:
* asm/unaligned.h
--- a/net/batman-adv/distributed-arp-table.c +++ b/net/batman-adv/distributed-arp-table.c
[...]
@@ -42,9 +43,11 @@ #include <linux/spinlock.h> #include <linux/stddef.h> #include <linux/string.h> +#include <linux/udp.h> #include <linux/workqueue.h> #include <net/arp.h> #include <net/genetlink.h> +#include <net/ip.h> #include <net/netlink.h> #include <net/sock.h> #include <uapi/linux/batman_adv.h>
And "net/ip.h" also doesn't seem to be used in distributed-arp-table.c
But the biggest problem is that it doesn't build:
/usr/bin/make -C /home/build_test/build_env/linux-build/linux-4.9.148 M=/home/build_test/build_env/tmp.9CjwRXIj4f PWD=/home/build_test/build_env/tmp.9CjwRXIj4f REVISION= CONFIG_BATMAN_ADV=m CONFIG_BATMAN_ADV_DEBUG=n CONFIG_BATMAN_ADV_DEBUGFS=y CONFIG_BATMAN_ADV_BLA=n CONFIG_BATMAN_ADV_DAT=n CONFIG_BATMAN_ADV_NC=y CONFIG_BATMAN_ADV_MCAST=y CONFIG_BATMAN_ADV_TRACING=y CONFIG_BATMAN_ADV_BATMAN_V=y INSTALL_MOD_DIR=updates/ modules [...] /home/build_test/build_env/tmp.9CjwRXIj4f/net/batman-adv/routing.c:1046:17: error: undefined identifier 'batadv_dat_snoop_incoming_dhcp_ack' /home/build_test/build_env/tmp.9CjwRXIj4f/net/batman-adv/routing.c:1283:9: error: undefined identifier 'batadv_dat_snoop_incoming_dhcp_ack' /home/build_test/build_env/tmp.9CjwRXIj4f/net/batman-adv/routing.c: In function ‘batadv_recv_unicast_packet’: /home/build_test/build_env/tmp.9CjwRXIj4f/net/batman-adv/routing.c:1046:3: error: implicit declaration of function ‘batadv_dat_snoop_incoming_dhcp_ack’ [-Werror=implicit-function-declaration] batadv_dat_snoop_incoming_dhcp_ack(bat_priv, skb, hdr_size);
Looks like distributed-arp-table.h is missing a stub function for batadv_dat_snoop_incoming_dhcp_ack when CONFIG_BATMAN_ADV_DAT is not set.
Kind regards, Sven