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

[Derek Foreman]

On 2018-08-17 05:39 AM, Pekka Paalanen wrote:
> 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>

It's also
Reviewed-by: Derek Foreman <derek.foreman.samsung at gmail.com>

I'd like to land this today, so I'll deal with the trivial style
complaints when landing.

Thanks,
Derek

>>  
>>  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;
> 
> 
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
> 


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