New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolved packet size #6214

Merged
merged 4 commits into from Jun 28, 2017

Conversation

4 participants
@keszybz
Member

keszybz commented Jun 27, 2017

No description provided.

keszybz added some commits Jun 18, 2017

resolved: simplify alloc size calculation
The allocation size was calculated in a complicated way, and for values
close to the page size we would actually allocate less than requested.

Reported by Chris Coulson <chris.coulson@canonical.com>.

CVE-2017-9445
@poettering

Looks great otherwise

Show outdated Hide outdated src/resolve/resolved-dns-packet.c

@poettering poettering added the resolve label Jun 27, 2017

@keszybz

This comment has been minimized.

Show comment
Hide comment
@keszybz

keszybz Jun 27, 2017

Member

Updated, PTAL.

Member

keszybz commented Jun 27, 2017

Updated, PTAL.

@poettering

This comment has been minimized.

Show comment
Hide comment
@poettering

poettering Jun 27, 2017

Member

excellent! thanks a ton for fixing this!

Member

poettering commented Jun 27, 2017

excellent! thanks a ton for fixing this!

assert_se(dns_packet_new(&p, DNS_PROTOCOL_DNS, i) == 0);
log_debug("dns_packet_new: %zu%zu", i, p->allocated);
assert_se(p->allocated >= MIN(DNS_PACKET_SIZE_MAX, i));

This comment has been minimized.

@evverx

evverx Jun 27, 2017

Member

There is a warning:

src/resolve/test-resolved-packet.c: In function ‘test_dns_packet_new’:
  CC       src/resolve/test_resolved_packet-resolved-dns-question.o
./src/basic/macro.h:187:30: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
                 UNIQ_T(A,aq) < UNIQ_T(B,bq) ? UNIQ_T(A,aq) : UNIQ_T(B,bq); \
                              ^
./src/basic/macro.h:44:44: note: in definition of macro ‘_unlikely_’
 #define _unlikely_(x) (__builtin_expect(!!(x),0))
                                            ^
./src/basic/macro.h:235:25: note: in expansion of macro ‘assert_message_se’
 #define assert_se(expr) assert_message_se(expr, #expr)
                         ^
src/resolve/test-resolved-packet.c:32:17: note: in expansion of macro ‘assert_se’
                 assert_se(p->allocated >= MIN(DNS_PACKET_SIZE_MAX, i));
                 ^
./src/basic/macro.h:182:19: note: in expansion of macro ‘__MIN’
 #define MIN(a, b) __MIN(UNIQ, (a), UNIQ, (b))
                   ^
src/resolve/test-resolved-packet.c:32:43: note: in expansion of macro ‘MIN’
                 assert_se(p->allocated >= MIN(DNS_PACKET_SIZE_MAX, i));
                                           ^
./src/basic/macro.h:187:60: warning: signed and unsigned type in conditional expression [-Wsign-compare]
                 UNIQ_T(A,aq) < UNIQ_T(B,bq) ? UNIQ_T(A,aq) : UNIQ_T(B,bq); \
                                                            ^
./src/basic/macro.h:44:44: note: in definition of macro ‘_unlikely_’
 #define _unlikely_(x) (__builtin_expect(!!(x),0))
                                            ^
./src/basic/macro.h:235:25: note: in expansion of macro ‘assert_message_se’
 #define assert_se(expr) assert_message_se(expr, #expr)
                         ^
src/resolve/test-resolved-packet.c:32:17: note: in expansion of macro ‘assert_se’
                 assert_se(p->allocated >= MIN(DNS_PACKET_SIZE_MAX, i));
                 ^
./src/basic/macro.h:182:19: note: in expansion of macro ‘__MIN’
 #define MIN(a, b) __MIN(UNIQ, (a), UNIQ, (b))
                   ^
src/resolve/test-resolved-packet.c:32:43: note: in expansion of macro ‘MIN’
                 assert_se(p->allocated >= MIN(DNS_PACKET_SIZE_MAX, i));
                                           ^

On the other hand, I understand what is being fixed here, so the warning can be fixed later. Thank you.

@evverx

evverx Jun 27, 2017

Member

There is a warning:

src/resolve/test-resolved-packet.c: In function ‘test_dns_packet_new’:
  CC       src/resolve/test_resolved_packet-resolved-dns-question.o
./src/basic/macro.h:187:30: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
                 UNIQ_T(A,aq) < UNIQ_T(B,bq) ? UNIQ_T(A,aq) : UNIQ_T(B,bq); \
                              ^
./src/basic/macro.h:44:44: note: in definition of macro ‘_unlikely_’
 #define _unlikely_(x) (__builtin_expect(!!(x),0))
                                            ^
./src/basic/macro.h:235:25: note: in expansion of macro ‘assert_message_se’
 #define assert_se(expr) assert_message_se(expr, #expr)
                         ^
src/resolve/test-resolved-packet.c:32:17: note: in expansion of macro ‘assert_se’
                 assert_se(p->allocated >= MIN(DNS_PACKET_SIZE_MAX, i));
                 ^
./src/basic/macro.h:182:19: note: in expansion of macro ‘__MIN’
 #define MIN(a, b) __MIN(UNIQ, (a), UNIQ, (b))
                   ^
src/resolve/test-resolved-packet.c:32:43: note: in expansion of macro ‘MIN’
                 assert_se(p->allocated >= MIN(DNS_PACKET_SIZE_MAX, i));
                                           ^
./src/basic/macro.h:187:60: warning: signed and unsigned type in conditional expression [-Wsign-compare]
                 UNIQ_T(A,aq) < UNIQ_T(B,bq) ? UNIQ_T(A,aq) : UNIQ_T(B,bq); \
                                                            ^
./src/basic/macro.h:44:44: note: in definition of macro ‘_unlikely_’
 #define _unlikely_(x) (__builtin_expect(!!(x),0))
                                            ^
./src/basic/macro.h:235:25: note: in expansion of macro ‘assert_message_se’
 #define assert_se(expr) assert_message_se(expr, #expr)
                         ^
src/resolve/test-resolved-packet.c:32:17: note: in expansion of macro ‘assert_se’
                 assert_se(p->allocated >= MIN(DNS_PACKET_SIZE_MAX, i));
                 ^
./src/basic/macro.h:182:19: note: in expansion of macro ‘__MIN’
 #define MIN(a, b) __MIN(UNIQ, (a), UNIQ, (b))
                   ^
src/resolve/test-resolved-packet.c:32:43: note: in expansion of macro ‘MIN’
                 assert_se(p->allocated >= MIN(DNS_PACKET_SIZE_MAX, i));
                                           ^

On the other hand, I understand what is being fixed here, so the warning can be fixed later. Thank you.

This comment has been minimized.

@keszybz

keszybz Jun 27, 2017

Member

What gcc version are you using? I don't get this warning here.

@keszybz

keszybz Jun 27, 2017

Member

What gcc version are you using? I don't get this warning here.

This comment has been minimized.

@evverx

evverx Jun 27, 2017

Member

I first noticed that warning there https://ci.centos.org/job/systemd-pr-build/3366/consoleFull. I also saw it while using

$ gcc --version
gcc (GCC) 6.3.1 20161221 (Red Hat 6.3.1-1)

and

$ gcc --version
gcc (Ubuntu 6.2.0-5ubuntu12) 6.2.0 20161005
@evverx

evverx Jun 27, 2017

Member

I first noticed that warning there https://ci.centos.org/job/systemd-pr-build/3366/consoleFull. I also saw it while using

$ gcc --version
gcc (GCC) 6.3.1 20161221 (Red Hat 6.3.1-1)

and

$ gcc --version
gcc (Ubuntu 6.2.0-5ubuntu12) 6.2.0 20161005

This comment has been minimized.

@keszybz

keszybz Jun 27, 2017

Member

I'm using gcc 7.1. That might explain the difference.

@keszybz

keszybz Jun 27, 2017

Member

I'm using gcc 7.1. That might explain the difference.

keszybz added some commits Jun 27, 2017

resolved: do not allocate packets with minimum size
dns_packet_new() is sometimes called with mtu == 0, and in that case we should
allocate more than the absolute minimum (which is the dns packet header size),
otherwise we have to resize immediately again after appending the first data to
the packet.

This partially reverts the previous commit.
resolved: define various packet sizes as unsigned
This seems like the right thing to do, and apparently at least some compilers
warn about signed/unsigned comparisons with DNS_PACKET_SIZE_MAX.
@keszybz

This comment has been minimized.

Show comment
Hide comment
@keszybz

keszybz Jun 27, 2017

Member

Updated with all requested changes.

Member

keszybz commented Jun 27, 2017

Updated with all requested changes.

@poettering poettering merged commit 980cb55 into systemd:master Jun 28, 2017

4 of 5 checks passed

xenial-i386 autopkgtest running
Details
default Build finished.
Details
semaphoreci The build passed on Semaphore.
Details
xenial-amd64 autopkgtest finished (success)
Details
xenial-s390x autopkgtest finished (success)
Details

@keszybz keszybz deleted the keszybz:resolved-packet-size branch Jun 28, 2017

* absolute minimum (which is the dns packet header size), to avoid
* resizing immediately again after appending the first data to the packet.
*/
if (mtu < UDP_PACKET_HEADER_SIZE)

This comment has been minimized.

@benjarobin

benjarobin Jun 28, 2017

Contributor

@keszybz @poettering I was trying to understand what dns_packet_new() is really doing, and I think it may have still an error.
Why the size_t argument is named mtu ? This is not the source of the confusion ?
The only call to dns_packet_new() with a non null size is in manager_recv() of resolved-manager.c

It looks like the size pass is just the size of the data, without the UDP header, so why UDP_PACKET_HEADER_SIZE is used in the current computation ?

@benjarobin

benjarobin Jun 28, 2017

Contributor

@keszybz @poettering I was trying to understand what dns_packet_new() is really doing, and I think it may have still an error.
Why the size_t argument is named mtu ? This is not the source of the confusion ?
The only call to dns_packet_new() with a non null size is in manager_recv() of resolved-manager.c

It looks like the size pass is just the size of the data, without the UDP header, so why UDP_PACKET_HEADER_SIZE is used in the current computation ?

This comment has been minimized.

@benjarobin

benjarobin Jun 28, 2017

Contributor

Instead of doing these changes, we just should have partially revert keszybz@a016660 and also rename the parameter named mtu of dns_packet_new() to something else: dataSize ?

@benjarobin

benjarobin Jun 28, 2017

Contributor

Instead of doing these changes, we just should have partially revert keszybz@a016660 and also rename the parameter named mtu of dns_packet_new() to something else: dataSize ?

This comment has been minimized.

@poettering

poettering Jun 28, 2017

Member

@benjarobin the reason it's called MTU is that when reading a DNS UDP packet off the network, and know the MTU of the iface, then we know how large the packet can grow to at max, and then use that to size the allocation from the beginning, so that we don't have to reallocate later on.

@poettering

poettering Jun 28, 2017

Member

@benjarobin the reason it's called MTU is that when reading a DNS UDP packet off the network, and know the MTU of the iface, then we know how large the packet can grow to at max, and then use that to size the allocation from the beginning, so that we don't have to reallocate later on.

This comment has been minimized.

@benjarobin

benjarobin Jun 28, 2017

Contributor

@poettering I see, it make sense, but the interface is never used like that.
It always set to 0 or the size of the data. That why the bug was introduced in a016660 .
The mtu include the UDP header, but here we are allocating just the size for the data, not the size of the raw packet

@benjarobin

benjarobin Jun 28, 2017

Contributor

@poettering I see, it make sense, but the interface is never used like that.
It always set to 0 or the size of the data. That why the bug was introduced in a016660 .
The mtu include the UDP header, but here we are allocating just the size for the data, not the size of the raw packet

This comment has been minimized.

@benjarobin

benjarobin Jun 28, 2017

Contributor

@poettering If argument mtu pass to the dns_packet_new() is equal to 50 then the data that need to be allocated is only 22, since the UDP header size is equal to 28 (if I am not wrong, this is not really important, this is just for the example)

So when the parameter mtu is passed to dns_packet_new() with value 50, what is the expected allocated size ?

@benjarobin

benjarobin Jun 28, 2017

Contributor

@poettering If argument mtu pass to the dns_packet_new() is equal to 50 then the data that need to be allocated is only 22, since the UDP header size is equal to 28 (if I am not wrong, this is not really important, this is just for the example)

So when the parameter mtu is passed to dns_packet_new() with value 50, what is the expected allocated size ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment