[PATCH 2/3] connection: Prevent integer overflow in DIV_ROUNDUP.

[Pekka Paalanen]

On Tue, 14 Aug 2018 13:07:52 +0200
Michal Srb <msrb at 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.paalanen at 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;

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180817/ce264b9b/attachment.sig>