dns: check there is room in answer packet

[MattCatz]

I came across some crashes on some of my systems that looks like this:

#0  0x0000000000406fa0 in dns_packet_answer (name=name@entry=0x4206a4 <mdns_hostname_local> \"854-v6-XXXX.local\", type=type@entry=28, rdata=0x7fb4013858 \"\\376\\200\", rdlength=rdlength@entry=16, ttl=ttl@entry=4500) at dns.c:161
161     a->type = cpu_to_be16(type);
The target architecture is set to \"aarch64\".",
#0  0x0000000000406fa0 in dns_packet_answer (name=name@entry=0x4206a4 <mdns_hostname_local> \"854-v6-XXXX.local\", type=type@entry=28, rdata=0x7fb4013858 \"\\376\\200\", rdlength=rdlength@entry=16, ttl=ttl@entry=4500) at dns.c:161
#1  0x00000000004073e0 in dns_reply_a (iface=iface@entry=0x7fb3f25e40, to=to@entry=0x0, ttl=4500, hostname=0x4206a4 <mdns_hostname_local> \"854-v6-XXXX.local\", hostname@entry=0x0) at dns.c:290
#2  0x0000000000402a18 in announce_timer (timeout=0x7fb3f25ea8) at announce.c:68
#3  0x0000007fb3f6ed24 in uloop_process_timeouts () at uloop.c:633
#4  uloop_run_timeout (timeout=timeout@entry=-1) at uloop.c:667
#5  0x0000000000402730 in uloop_run () at /usr/include/libubox/uloop.h:149
#6  main (argc=1, argv=0x7fedda97f8) at main.c:142

I am no able to recreate the issue on demand. Looking through the code, I would guess the crash is caused by dns_packet_answer not checking if there is enough room in the packet (causing us to de-reference NULL).

This patch changes the code to do this patch and skip adding an answer if there is not enough room.

[blogic]

need to take a closer look but at first glance we have broken packet accumulation logic that causes unnecessary buffer exhaustion thus causing a failed NULL check

[MattCatz]

Makes sense. I can see this patch being a band-aid on an underlying issue. I have a handful of these crashes most of them are all the same code path as above ^^^.

I do have a small amounts of similar crashes that look like this:

[New LWP 20894]
Core was generated by `/usr/sbin/umdns'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x0000000000406fa0 in dns_packet_answer (name=name@entry=0x4206a4 <mdns_hostname_local> \"854-6-XXXX.local\ type=type@entry=28, rdata=0x7fbb6cad28 \"\\375\\v\\242\\323\ <incomplete sequence \\317>, rdlength=rdlength@entry=16, ttl=ttl@entry=4500) at dns.c:161
161     a->type = cpu_to_be16(type);
The target architecture is set to \"aarch64\".
#0  0x0000000000406fa0 in dns_packet_answer (name=name@entry=0x4206a4 <mdns_hostname_local> \"854-6-XXXX.local\ type=type@entry=28, rdata=0x7fbb6cad28 \"\\375\\v\\242\\323\ <incomplete sequence \\317>, rdlength=rdlength@entry=16, ttl=ttl@entry=4500) at dns.c:161
#1  0x00000000004073e0 in dns_reply_a (iface=0x7fbb79de70, to=0x7fecbc5cb8, ttl=4500, hostname=0x4206a4 <mdns_hostname_local> \"854-6-XXXX.local\ hostname@entry=0x0) at dns.c:290
#2  0x0000000000407a44 in parse_question (iface=<optimized out>, iface@entry=0x7fbb77ce40, from=from@entry=0x7fecbc5cb8, name=<optimized out>, q=<optimized out>) at dns.c:643
#3  0x0000000000407c54 in dns_handle_packet (iface=iface@entry=0x7fbb77ce40, from=from@entry=0x7fecbc5cb8, port=<optimized out>, buffer=buffer@entry=0x424ad8 <buffer> \"\ len=len@entry=60) at dns.c:688
#4  0x0000000000405c30 in read_socket4 (u=<optimized out>, events=<optimized out>) at interface.c:282
#5  0x0000007fbb7c5e68 in uloop_run_events (timeout=<optimized out>) at uloop.c:226
#6  uloop_run_timeout (timeout=timeout@entry=-1) at uloop.c:677
#7  0x0000000000402730 in uloop_run () at usr/include/libubox/uloop.h:149
#8  main (argc=1, argv=0x7fecbc5f78) at main.c:142
bt_done
threads: 1
Single-threaded program detected. No additional thread backtraces needed.

[MattCatz]

@blogic

I dug into this bug a little bit more. Using the coredump to dump the current packet, it looks like the packet accumulation logic is fine. It was just caused by appearing to exhaust the buffer.

The packet was ~900 bytes long. This is the effective max size since dns_packet_add_name always assumes the string is MAX_NAME_LEN long:

mdnsd/dns.c

Line 110 in 2f75344

data = dns_packet_tail(MAX_NAME_LEN);

Somehow the interface got assigned 30+ IPv6 addresses. That many addresses hit the effective max size on the buffer and caused dns_packet_add_name to fail the size check and dns_packet_record_add to return NULL.

We could lower the length assumption to something smaller to avoid the crash but adding a NULL check to dns_packet_answer is probably the only way to address the issue.