On 2018-08-17 05:39 AM, Pekka Paalanen wrote:
> 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>
It's also
Reviewed-by: Derek Foreman <derek.foreman.sams...@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
signature.asc
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
