dhcpcd-discuss

Roy,

Thanks for the patch.   I'll run with it for a few weeks to see if it corrects the crash I seen.


I noticed something else that seems to be related to the BPF changes from 6.11 -> 7.x

If the interface is a wireless interface all the arp send attempts are failing to send.

It looks like this..


 dhcpcd[3887]: wlan0: probing address x.x.45.167/23
 dhcpcd[3887]: arp_probe1: Invalid argument

 dhcpcd[3887]: arp_announce1: Invalid argument

 dhcpcd[3887]: wlan0: leased x.x.45.167 for 5400 seconds


 I don't see this using a Ethernet interface.

 The invalid argument error is coming from the writev() inside bpf_send().


 That routine works fine for this interface when used with the dhcp bpf socket.

 It fails when used in the arp logic.   They use different filters but I was under the impression that

 should only effect inbound traffic.  Is that true ?


 This is a linux based device so I noticed it does have some special routines related to bpf.


 Thanks

 Ken


________________________________
From: Roy Marples <roy@xxxxxxxxxxxx>
Sent: Friday, April 26, 2019 6:23:25 PM
To: dhcpcd-discuss@xxxxxxxxxxxx; Kenny Napier
Subject: Re: [dhcpcd-discuss] buffer overflow crash 7.1.0 arp.c - arp_packet()

Hi Kenny

On 26/04/2019 21:06, Kenny Napier wrote:
>     I have seen a __memcpy_chk() detected buffer over flow crash occur
> twice using version 7.1.0
>
> The crash occurs in arp.c in the arp_packet() function during probing of
> a new address.
>
> The line is the last memcpy in that routine where it is attempting to
> copy the target ip address from the arp packet into a
>
> internal structure. I noticed this routine use to have some bounds
> checking at the top that has been commented out for a long
>
> time.   The comment says the BPF filters does this work now.
>
>
> I tried to verify the bpf filter is really doing this work and noticed a
> few things that look odd.
>
>
> bpf.c : bfp_arp_filter []
>
>
>     /* Make sure this is an ARP REQUEST. */
>          BPF_STMT(BPF_LD + BPF_H + BPF_IND, offsetof(struct arphdr, ar_op)),
>          BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, ARPOP_REQUEST, 2, 0),
>          /* or ARP REPLY. */
>          BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, ARPOP_REPLY, 1, 1),
>          BPF_STMT(BPF_RET + BPF_K, 0),
>    /* Make sure the protocol length matches. */
>          BPF_STMT(BPF_LD + BPF_B + BPF_IND, offsetof(struct arphdr,
> ar_pln)),
>          BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, sizeof(in_addr_t), 1, 0),
>          BPF_STMT(BPF_RET + BPF_K, 0),
>
> It looks like it intended to only let ARP_REQUEST and ARP_REPLY's
> through the filter but the  1,1 after the REPLY would
> let anything through this check.
>
> Also in the installer routine bfp_arp(...) the structure at the top used
> to hold the built up the filter does not take into account the
> bfp_arp_ether array that is copied in first.
>
> struct bpf_insn bpf[3+ bpf_arp_filter_len + bpf_arp_hw + bpf_arp_extra];
>
> It looks like we would always overflow this stack buffer. I don't know
> if the filter would get installed as intended if that is happening.

You're right!
I've attached a patch which should fix both issues.

> The crash is very rare.  I have no idea how to make it happen on demand.
> Looking for thoughts on the crash and my bpf code observations.

You would need several active ARP states I think - ensure the DHCP
server delays the reply so that IPv4LL ARP probes happen at the same
DHCP ARP probes happen.

I've tried to repliate this using valgind, but no memory errors are
reported :/
Let me know how the patch works for you!

Roy