On 12/03/15(Thu) 11:12, Mike Belopuhov wrote: > On 12 March 2015 at 10:39, Martin Pieuchot <[email protected]> wrote: > > On 12/03/15(Thu) 09:53, Henk Jan Agteresch wrote: > >> On Tue, 10 Mar 2015, Martin Pieuchot wrote: > >> > >> > > >> > Here's a first diff that should prevent the stack smashing. Could you > >> > run with it and tell me if the ARP entry gets overwritten as in 5.5? > >> > > >> > >> Patch works for me. Arp entry gets overwritten. No more panics during > >> network configuring. > >> > >> # dmesg |grep arp > >> arpresolve: 213.154.229.23: incorrect arp information > >> arpresolve: 213.154.229.23: incorrect arp information > >> arpresolve: 213.154.229.23: incorrect arp information > >> arpresolve: 213.154.229.23: incorrect arp information > >> arpresolve: 213.154.229.23: incorrect arp information > >> arp info overwritten for 213.154.229.23 by fe:54:00:b2:8c:98 on pcn0 > > > > Thanks for testing, I'll try to cook another diff to not encode the name > > of the interface in the arp information. > > > > Here's the same diff with the nit pointed by krw@ fixed, any ok? > > > > So how can this happen? What kind of incorrect ARP information is it? > Where does it come from? And why didn't we see that before?
The incorrect ARP information is the name of the interface. This is due to the way information are encoded in sockaddr_dl, see the LLADDR() macro. When route(8) is called with the -link argument it fills the gateway field with link_addr(3). This field is then copied into the kernel and passed directly to rtrequest1(RTM_ADD,). To understand that, look at the "so_gate" field when you run: $ route -nvd get default -link lo0 Why we didn't see that before I don't know. If you find why, I'm interested. That's what the second diff should fix. My guess is that sdl_nlen is incorrectly set which would explain why the name is now encoded and the size longer than 6. Another hypothesis is that the sdl_data field is cleared after being set. Or we always smashed the stack in this case... It seems the regression has been introduced between 5.5 and 5.6 but I couldn't find it as a first glance. Anyway I still believe this diff is worth it as it will prevent future bugs to smash your stack. Ok?
