From mboxrd@z Thu Jan  1 00:00:00 1970
Subject: Re: [PATCH 03/13] overflow.h: Add allocation size calculation helpers
References: <20180509004229.36341-1-keescook@chromium.org>
 <20180509004229.36341-4-keescook@chromium.org>
From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Message-ID: <b481bbb2-758f-dcd3-3e1a-8b3137081614@rasmusvillemoes.dk>
Date: Wed, 9 May 2018 20:27:22 +0200
MIME-Version: 1.0
In-Reply-To: <20180509004229.36341-4-keescook@chromium.org>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
To: Kees Cook <keescook@chromium.org>, Matthew Wilcox <mawilcox@microsoft.com>
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, kernel-hardening@lists.openwall.com
List-ID: <kernel-hardening.lists.openwall.com>

On 2018-05-09 02:42, Kees Cook wrote:
> In preparation for replacing unchecked overflows for memory allocations,
> this creates helpers for the 3 most common calculations:
> 
> array_size(a, b): 2-dimensional array
> array3_size(a, b, c): 2-dimensional array

yeah... complete confusion...

> +/**
> + * array_size() - Calculate size of 2-dimensional array.
> + *
> + * @a: dimension one
> + * @b: dimension two
> + *
> + * Calculates size of 2-dimensional array: @a * @b.
> + *
> + * Returns: number of bytes needed to represent the array or SIZE_MAX on
> + * overflow.
> + */
> +static inline __must_check size_t array_size(size_t a, size_t b)
> +{
> +	size_t bytes;
> +
> +	if (check_mul_overflow(a, b, &bytes))
> +		return SIZE_MAX;
> +
> +	return bytes;
> +}
> +
> +/**
> + * array3_size() - Calculate size of 3-dimensional array.
> + *

...IDGI. array_size is/will most often be used to calculate the size of
a one-dimensional array, count*elemsize, accessed as foo[i]. Won't a
three-factor product usually be dim1*dim2*elemsize, i.e. 2-dimensional,
accessed (because C is lame) as foo[i*dim2 + j]?

> +/**
> + * struct_size() - Calculate size of structure with trailing array.
> + * @p: Pointer to the structure.
> + * @member: Name of the array member.
> + * @n: Number of elements in the array.
> + *
> + * Calculates size of memory needed for structure @p followed by an
> + * array of @n @member elements.
> + *
> + * Return: number of bytes needed or SIZE_MAX on overflow.
> + */
> +#define struct_size(p, member, n)					\
> +	__ab_c_size(n,							\
> +		    sizeof(*(p)->member) + __must_be_array((p)->member),\
> +		    offsetof(typeof(*(p)), member))
> +
> +

struct s { int a; char b; char c[]; } has sizeof > offsetof(c), so for
small enough n, we end up allocating less than sizeof(struct s). And the
caller might reasonably do memset(s, 0, sizeof(*s)) to initialize all
members before c. In practice our allocators round up to a multiple of
8, so I don't think it's a big problem, but some sanitizer might
complain. But I do think you should make that addend sizeof() instead of
offsetof().

Rasmus