Re: [B.A.T.M.A.N.] [PATCH v7] batman-adv: Snoop DHCPACKs for DAT

[Sven Eckelmann <sven@narfation.org>]

[-- Attachment #1: Type: text/plain, Size: 3647 bytes --]

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]