On Tue, 14 Aug 2018 13:07:52 +0200
Michal Srb <m...@suse.com> wrote:
> The DIV_ROUNDUP macro would overflow when trying to round values higher
> than MAX_UINT32 - (a - 1). The result is 0 after the division. This is
> potential security issue when demarshalling an array because the length
> check is performed with the overflowed value, but then the original huge
> value is stored for later use.
>
> The issue was present only on 32bit platforms. The use of size_t in the
> DIV_ROUNDUP macro already promoted everything to 64 bit size on 64 bit
> systems.
> ---
> src/connection.c | 23 ++++++++++++++---------
> 1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/src/connection.c b/src/connection.c
> index 294c521..31396bc 100644
> --- a/src/connection.c
> +++ b/src/connection.c
> @@ -44,7 +44,12 @@
> #include "wayland-private.h"
> #include "wayland-os.h"
>
> -#define DIV_ROUNDUP(n, a) ( ((n) + ((a) - 1)) / (a) )
> +static inline uint32_t div_roundup(uint32_t n, size_t a) {
> + // The cast to uint64_t is necessary to prevent overflow when rounding
> + // values close to UINT32_MAX. After the division it is again safe to
> + // cast back to uint32_t.
> + return (uint32_t) (((uint64_t) n + (a - 1)) / a);
> +}
The above has a few style issues:
- div_roundup should start on a new line as it is a function now
- use /* */ comment style
- use tabs for indent
- missing Signed-off-by
But aside from those, this patch is:
Reviewed-by: Pekka Paalanen <pekka.paala...@collabora.co.uk>
>
> struct wl_buffer {
> char data[4096];
> @@ -734,7 +739,7 @@ wl_connection_demarshal(struct wl_connection *connection,
> break;
> }
>
> - next = p + DIV_ROUNDUP(length, sizeof *p);
> + next = p + div_roundup(length, sizeof *p);
> if (next > end) {
> wl_log("message too short, "
> "object (%d), message %s(%s)\n",
> @@ -793,7 +798,7 @@ wl_connection_demarshal(struct wl_connection *connection,
> case 'a':
> length = *p++;
>
> - next = p + DIV_ROUNDUP(length, sizeof *p);
> + next = p + div_roundup(length, sizeof *p);
> if (next > end) {
> wl_log("message too short, "
> "object (%d), message %s(%s)\n",
> @@ -1068,7 +1073,7 @@ buffer_size_for_closure(struct wl_closure *closure)
> }
>
> size = strlen(closure->args[i].s) + 1;
> - buffer_size += 1 + DIV_ROUNDUP(size, sizeof(uint32_t));
> + buffer_size += 1 + div_roundup(size, sizeof(uint32_t));
> break;
> case 'a':
> if (closure->args[i].a == NULL) {
> @@ -1077,7 +1082,7 @@ buffer_size_for_closure(struct wl_closure *closure)
> }
>
> size = closure->args[i].a->size;
> - buffer_size += (1 + DIV_ROUNDUP(size,
> sizeof(uint32_t)));
> + buffer_size += (1 + div_roundup(size,
> sizeof(uint32_t)));
> break;
> default:
> break;
> @@ -1139,11 +1144,11 @@ serialize_closure(struct wl_closure *closure,
> uint32_t *buffer,
> size = strlen(closure->args[i].s) + 1;
> *p++ = size;
>
> - if (p + DIV_ROUNDUP(size, sizeof *p) > end)
> + if (p + div_roundup(size, sizeof *p) > end)
> goto overflow;
>
> memcpy(p, closure->args[i].s, size);
> - p += DIV_ROUNDUP(size, sizeof *p);
> + p += div_roundup(size, sizeof *p);
> break;
> case 'a':
> if (closure->args[i].a == NULL) {
> @@ -1154,11 +1159,11 @@ serialize_closure(struct wl_closure *closure,
> uint32_t *buffer,
> size = closure->args[i].a->size;
> *p++ = size;
>
> - if (p + DIV_ROUNDUP(size, sizeof *p) > end)
> + if (p + div_roundup(size, sizeof *p) > end)
> goto overflow;
>
> memcpy(p, closure->args[i].a->data, size);
> - p += DIV_ROUNDUP(size, sizeof *p);
> + p += div_roundup(size, sizeof *p);
> break;
> default:
> break;
pgpBAT4VP0VbZ.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
