On Tue, 14 Aug 2018 13:07:53 +0200 Michal Srb <m...@suse.com> wrote: > If the remote side sends sufficiently large `length` field, it will > overflow the `p` pointer. Technically it is undefined behavior, in > practice it makes `p < end`, so the length check passes. Attempts to > access the data later causes crashes. > > This issue manifests only on 32bit systems, but the behavior is > undefined everywhere. > --- > src/connection.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-)
Hi,
this looks correct to me and should address Jann's concerns too. I also
checked that (end - p) cannot become negative.
Reviewed-by: Pekka Paalanen <pekka.paala...@collabora.co.uk>
I'm still playing with the tests patch a bit.
Thanks,
pq
>
> diff --git a/src/connection.c b/src/connection.c
> index 31396bc..a0f10ae 100644
> --- a/src/connection.c
> +++ b/src/connection.c
> @@ -683,7 +683,7 @@ wl_connection_demarshal(struct wl_connection *connection,
> struct wl_map *objects,
> const struct wl_message *message)
> {
> - uint32_t *p, *next, *end, length, id;
> + uint32_t *p, *next, *end, length, length_in_u32, id;
> int fd;
> char *s;
> int i, count, num_arrays;
> @@ -739,8 +739,8 @@ wl_connection_demarshal(struct wl_connection *connection,
> break;
> }
>
> - next = p + div_roundup(length, sizeof *p);
> - if (next > end) {
> + length_in_u32 = div_roundup(length, sizeof *p);
> + if ((uint32_t) (end - p) < length_in_u32) {
> wl_log("message too short, "
> "object (%d), message %s(%s)\n",
> closure->sender_id, message->name,
> @@ -748,6 +748,7 @@ wl_connection_demarshal(struct wl_connection *connection,
> errno = EINVAL;
> goto err;
> }
> + next = p + length_in_u32;
>
> s = (char *) p;
>
> @@ -798,8 +799,8 @@ wl_connection_demarshal(struct wl_connection *connection,
> case 'a':
> length = *p++;
>
> - next = p + div_roundup(length, sizeof *p);
> - if (next > end) {
> + length_in_u32 = div_roundup(length, sizeof *p);
> + if ((uint32_t) (end - p) < length_in_u32) {
> wl_log("message too short, "
> "object (%d), message %s(%s)\n",
> closure->sender_id, message->name,
> @@ -807,6 +808,7 @@ wl_connection_demarshal(struct wl_connection *connection,
> errno = EINVAL;
> goto err;
> }
> + next = p + length_in_u32;
>
> array_extra->size = length;
> array_extra->alloc = 0;
pgp06SqJnEuXx.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
