The length field can be any uint32 value. Two kinds of overflows may happen on 32 bit systems:
1) If the value is in range [UINT32_MAX-3, UINT32_MAX], the DIV_ROUNDUP will turn it into 0. Then `next` equals `p` and so the big `length` is not detected. But the wl_array will contain the original big value. Probably leading to crashes it is used later. 2) The pointer may overflow if the `length` is sufficiently big. In that case `next` will point to some memory before the beginning of the buffer and the the check `next > end` is not triggered. Using `next` later can crash. Signed-off-by: Michal Srb <[email protected]> --- Note that the problem happens only on 32bit systems. src/connection.c | 19 +++++++++++++++++-- tests/connection-test.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/src/connection.c b/src/connection.c index 294c521..b48f3a4 100644 --- a/src/connection.c +++ b/src/connection.c @@ -46,6 +46,21 @@ #define DIV_ROUNDUP(n, a) ( ((n) + ((a) - 1)) / (a) ) +inline uint32_t * +ptr_add_saturating(uint32_t *p, uint32_t length) { + // Make sure that rounding length up will not overflow. + if (length > UINT32_MAX - sizeof(uint32_t)) + return (uint32_t*) INTPTR_MAX; + + length = DIV_ROUNDUP(length, sizeof(uint32_t)); + + // Make sure that adding length to p will not overflow. + if (length * sizeof(uint32_t) > UINTPTR_MAX - (uintptr_t) p) + return (uint32_t*) INTPTR_MAX; + + return p + length; +} + struct wl_buffer { char data[4096]; uint32_t head, tail; @@ -734,7 +749,7 @@ wl_connection_demarshal(struct wl_connection *connection, break; } - next = p + DIV_ROUNDUP(length, sizeof *p); + next = ptr_add_saturating(p, length); if (next > end) { wl_log("message too short, " "object (%d), message %s(%s)\n", @@ -793,7 +808,7 @@ wl_connection_demarshal(struct wl_connection *connection, case 'a': length = *p++; - next = p + DIV_ROUNDUP(length, sizeof *p); + next = ptr_add_saturating(p, length); if (next > end) { wl_log("message too short, " "object (%d), message %s(%s)\n", diff --git a/tests/connection-test.c b/tests/connection-test.c index 157e1bc..ff064cf 100644 --- a/tests/connection-test.c +++ b/tests/connection-test.c @@ -456,6 +456,52 @@ TEST(connection_demarshal) release_marshal_data(&data); } +static void +expected_fail_demarshal(struct marshal_data *data, const char *format, uint32_t *msg, int expected_error) +{ + struct wl_message message = { "test", format, NULL }; + struct wl_closure *closure; + struct wl_map objects; + int size = msg[1]; + + assert(write(data->s[1], msg, size) == size); + assert(wl_connection_read(data->read_connection) == size); + + wl_map_init(&objects, WL_MAP_SERVER_SIDE); + closure = wl_connection_demarshal(data->read_connection, + size, &objects, &message); + + assert(closure == NULL); + assert(errno == expected_error); +} + +TEST(connection_demarshal_failures) +{ + struct marshal_data data; + uint32_t msg[10]; + + setup_marshal_data(&data); + + // These need careful handling on 32bit systems. + uint32_t overflowing_values[] = { + 0xffffffff, 0xfffffffe, 0xfffffffd, 0xfffffffc, + 0xfffff000, 0xffffd000, 0xffffc000, 0xffffb000 + }; + for (unsigned int i = 0; i < ARRAY_LENGTH(overflowing_values); i++) { + msg[0] = 400200; + msg[1] = 24; + msg[2] = overflowing_values[i]; + expected_fail_demarshal(&data, "s", msg, EINVAL); + + msg[0] = 400200; + msg[1] = 24; + msg[2] = overflowing_values[i]; + expected_fail_demarshal(&data, "a", msg, EINVAL); + } + + release_marshal_data(&data); +} + static void marshal_demarshal(struct marshal_data *data, void (*func)(void), int size, const char *format, ...) -- 2.16.4 _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
