All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] exofs: Avoid VLA in structures
@ 2018-03-27 20:39 Kees Cook
  2018-03-27 22:10 ` Andrew Morton
  2018-03-29 21:32 ` Nick Desaulniers
  0 siblings, 2 replies; 4+ messages in thread
From: Kees Cook @ 2018-03-27 20:39 UTC (permalink / raw)
  To: Andrew Morton, Boaz Harrosh
  Cc: linux-kernel, Nick Desaulniers, kernel-hardening

On the quest to remove all VLAs from the kernel[1] this adjusts several
cases where allocation is made after an array of structures that points
back into the allocation. The allocations are changed to perform explicit
calculations instead of using a Variable Length Array in a structure.
Additionally, this lets Clang compile this code now, since Clang does not
support VLAIS[2].

[1] https://lkml.org/lkml/2018/3/7/621
[2] https://lkml.org/lkml/2013/9/23/500

Signed-off-by: Kees Cook <keescook@chromium.org>
---
I not sure the best way to test this. Kconfig implies I need special hardware?
---
 fs/exofs/ore.c      | 84 +++++++++++++++++++++++++++++++----------------------
 fs/exofs/ore_raid.c | 73 ++++++++++++++++++++++++++++++++++------------
 fs/exofs/super.c    | 23 +++++++--------
 3 files changed, 114 insertions(+), 66 deletions(-)

diff --git a/fs/exofs/ore.c b/fs/exofs/ore.c
index 3c6a9c156b7a..cfa862ea19d2 100644
--- a/fs/exofs/ore.c
+++ b/fs/exofs/ore.c
@@ -146,68 +146,82 @@ int  _ore_get_io_state(struct ore_layout *layout,
 			struct ore_io_state **pios)
 {
 	struct ore_io_state *ios;
-	struct page **pages;
-	struct osd_sg_entry *sgilist;
+	size_t size_ios, size_extra, size_total;
+	void *ios_extra;
+
+	/*
+	 * The desired layout looks like this, with the extra_allocation
+	 * items pointed at from fields within ios or per_dev:
+
 	struct __alloc_all_io_state {
 		struct ore_io_state ios;
 		struct ore_per_dev_state per_dev[numdevs];
 		union {
 			struct osd_sg_entry sglist[sgs_per_dev * numdevs];
 			struct page *pages[num_par_pages];
-		};
-	} *_aios;
-
-	if (likely(sizeof(*_aios) <= PAGE_SIZE)) {
-		_aios = kzalloc(sizeof(*_aios), GFP_KERNEL);
-		if (unlikely(!_aios)) {
-			ORE_DBGMSG("Failed kzalloc bytes=%zd\n",
-				   sizeof(*_aios));
+		} extra_allocation;
+	} whole_allocation;
+
+	*/
+
+	/* This should never happen, so abort early if it ever does. */
+	if (sgs_per_dev && num_par_pages) {
+		ORE_DBGMSG("Tried to use both pages and sglist\n");
+		*pios = NULL;
+		return -EINVAL;
+	}
+
+	if (numdevs > (INT_MAX - sizeof(*ios)) /
+		       sizeof(struct ore_per_dev_state))
+		return -ENOMEM;
+	size_ios = sizeof(*ios) + sizeof(struct ore_per_dev_state) * numdevs;
+
+	if (sgs_per_dev * numdevs > INT_MAX / sizeof(struct osd_sg_entry))
+		return -ENOMEM;
+	if (num_par_pages > INT_MAX / sizeof(struct page *))
+		return -ENOMEM;
+	size_extra = max(sizeof(struct osd_sg_entry) * (sgs_per_dev * numdevs),
+			 sizeof(struct page *) * num_par_pages);
+
+	size_total = size_ios + size_extra;
+
+	if (likely(size_total <= PAGE_SIZE)) {
+		ios = kzalloc(size_total, GFP_KERNEL);
+		if (unlikely(!ios)) {
+			ORE_DBGMSG("Failed kzalloc bytes=%zd\n", size_total);
 			*pios = NULL;
 			return -ENOMEM;
 		}
-		pages = num_par_pages ? _aios->pages : NULL;
-		sgilist = sgs_per_dev ? _aios->sglist : NULL;
-		ios = &_aios->ios;
+		ios_extra = (char *)ios + size_ios;
 	} else {
-		struct __alloc_small_io_state {
-			struct ore_io_state ios;
-			struct ore_per_dev_state per_dev[numdevs];
-		} *_aio_small;
-		union __extra_part {
-			struct osd_sg_entry sglist[sgs_per_dev * numdevs];
-			struct page *pages[num_par_pages];
-		} *extra_part;
-
-		_aio_small = kzalloc(sizeof(*_aio_small), GFP_KERNEL);
-		if (unlikely(!_aio_small)) {
+		ios = kzalloc(size_ios, GFP_KERNEL);
+		if (unlikely(!ios)) {
 			ORE_DBGMSG("Failed alloc first part bytes=%zd\n",
-				   sizeof(*_aio_small));
+				   size_ios);
 			*pios = NULL;
 			return -ENOMEM;
 		}
-		extra_part = kzalloc(sizeof(*extra_part), GFP_KERNEL);
-		if (unlikely(!extra_part)) {
+		ios_extra = kzalloc(size_extra, GFP_KERNEL);
+		if (unlikely(!ios_extra)) {
 			ORE_DBGMSG("Failed alloc second part bytes=%zd\n",
-				   sizeof(*extra_part));
-			kfree(_aio_small);
+				   size_extra);
+			kfree(ios);
 			*pios = NULL;
 			return -ENOMEM;
 		}
 
-		pages = num_par_pages ? extra_part->pages : NULL;
-		sgilist = sgs_per_dev ? extra_part->sglist : NULL;
 		/* In this case the per_dev[0].sgilist holds the pointer to
 		 * be freed
 		 */
-		ios = &_aio_small->ios;
 		ios->extra_part_alloc = true;
 	}
 
-	if (pages) {
-		ios->parity_pages = pages;
+	if (num_par_pages) {
+		ios->parity_pages = ios_extra;
 		ios->max_par_pages = num_par_pages;
 	}
-	if (sgilist) {
+	if (sgs_per_dev) {
+		struct osd_sg_entry *sgilist = ios_extra;
 		unsigned d;
 
 		for (d = 0; d < numdevs; ++d) {
diff --git a/fs/exofs/ore_raid.c b/fs/exofs/ore_raid.c
index 27cbdb697649..659129d5e9f7 100644
--- a/fs/exofs/ore_raid.c
+++ b/fs/exofs/ore_raid.c
@@ -71,6 +71,11 @@ static int _sp2d_alloc(unsigned pages_in_unit, unsigned group_width,
 {
 	struct __stripe_pages_2d *sp2d;
 	unsigned data_devs = group_width - parity;
+
+	/*
+	 * Desired allocation layout is, though when larger than PAGE_SIZE,
+	 * each struct __alloc_1p_arrays is separately allocated:
+
 	struct _alloc_all_bytes {
 		struct __alloc_stripe_pages_2d {
 			struct __stripe_pages_2d sp2d;
@@ -82,55 +87,85 @@ static int _sp2d_alloc(unsigned pages_in_unit, unsigned group_width,
 			char page_is_read[data_devs];
 		} __a1pa[pages_in_unit];
 	} *_aab;
+
 	struct __alloc_1p_arrays *__a1pa;
 	struct __alloc_1p_arrays *__a1pa_end;
-	const unsigned sizeof__a1pa = sizeof(_aab->__a1pa[0]);
+
+	*/
+
+	char *__a1pa;
+	char *__a1pa_end;
+
+	const size_t sizeof_stripe_pages_2d =
+		sizeof(struct __stripe_pages_2d) +
+		sizeof(struct __1_page_stripe) * pages_in_unit;
+	const size_t sizeof__a1pa =
+		ALIGN(sizeof(struct page *) * (2 * group_width) + data_devs,
+		      sizeof(void *));
+	const size_t sizeof__a1pa_arrays = sizeof__a1pa * pages_in_unit;
+	const size_t alloc_total = sizeof_stripe_pages_2d +
+				   sizeof__a1pa_arrays;
+
 	unsigned num_a1pa, alloc_size, i;
 
 	/* FIXME: check these numbers in ore_verify_layout */
-	BUG_ON(sizeof(_aab->__asp2d) > PAGE_SIZE);
+	BUG_ON(sizeof_stripe_pages_2d > PAGE_SIZE);
 	BUG_ON(sizeof__a1pa > PAGE_SIZE);
 
-	if (sizeof(*_aab) > PAGE_SIZE) {
-		num_a1pa = (PAGE_SIZE - sizeof(_aab->__asp2d)) / sizeof__a1pa;
-		alloc_size = sizeof(_aab->__asp2d) + sizeof__a1pa * num_a1pa;
+	/*
+	 * If alloc_total would be larger than PAGE_SIZE, only allocate
+	 * as many a1pa items as would fill the rest of the page, instead
+	 * of the full pages_in_unit count.
+	 */
+	if (alloc_total > PAGE_SIZE) {
+		num_a1pa = (PAGE_SIZE - sizeof_stripe_pages_2d) / sizeof__a1pa;
+		alloc_size = sizeof_stripe_pages_2d + sizeof__a1pa * num_a1pa;
 	} else {
 		num_a1pa = pages_in_unit;
-		alloc_size = sizeof(*_aab);
+		alloc_size = alloc_total;
 	}
 
-	_aab = kzalloc(alloc_size, GFP_KERNEL);
-	if (unlikely(!_aab)) {
+	*psp2d = sp2d = kzalloc(alloc_size, GFP_KERNEL);
+	if (unlikely(!sp2d)) {
 		ORE_DBGMSG("!! Failed to alloc sp2d size=%d\n", alloc_size);
 		return -ENOMEM;
 	}
+	/* From here Just call _sp2d_free */
 
-	sp2d = &_aab->__asp2d.sp2d;
-	*psp2d = sp2d; /* From here Just call _sp2d_free */
-
-	__a1pa = _aab->__a1pa;
-	__a1pa_end = __a1pa + num_a1pa;
+	/* Find start of a1pa area. */
+	__a1pa = (char *)sp2d + sizeof_stripe_pages_2d;
+	/* Find end of the _allocated_ a1pa area. */
+	__a1pa_end = __a1pa + alloc_size;
 
+	/* Allocate additionally needed a1pa items in PAGE_SIZE chunks. */
 	for (i = 0; i < pages_in_unit; ++i) {
 		if (unlikely(__a1pa >= __a1pa_end)) {
 			num_a1pa = min_t(unsigned, PAGE_SIZE / sizeof__a1pa,
 							pages_in_unit - i);
+			alloc_size = sizeof__a1pa * num_a1pa;
 
-			__a1pa = kcalloc(num_a1pa, sizeof__a1pa, GFP_KERNEL);
+			__a1pa = kzalloc(alloc_size, GFP_KERNEL);
 			if (unlikely(!__a1pa)) {
 				ORE_DBGMSG("!! Failed to _alloc_1p_arrays=%d\n",
 					   num_a1pa);
 				return -ENOMEM;
 			}
-			__a1pa_end = __a1pa + num_a1pa;
+			__a1pa_end = __a1pa + alloc_size;
 			/* First *pages is marked for kfree of the buffer */
 			sp2d->_1p_stripes[i].alloc = true;
 		}
 
-		sp2d->_1p_stripes[i].pages = __a1pa->pages;
-		sp2d->_1p_stripes[i].scribble = __a1pa->scribble ;
-		sp2d->_1p_stripes[i].page_is_read = __a1pa->page_is_read;
-		++__a1pa;
+		/*
+		 * Attach all _lp_stripes pointers to the allocation for
+		 * it which was either part of the original PAGE_SIZE
+		 * allocation or the subsequent allocation in this loop.
+		 */
+		sp2d->_1p_stripes[i].pages = (void *)__a1pa;
+		sp2d->_1p_stripes[i].scribble =
+			sp2d->_1p_stripes[i].pages + group_width;
+		sp2d->_1p_stripes[i].page_is_read =
+			(char *)(sp2d->_1p_stripes[i].scribble + group_width);
+		__a1pa += sizeof__a1pa;
 	}
 
 	sp2d->parity = parity;
diff --git a/fs/exofs/super.c b/fs/exofs/super.c
index 179cd5c2f52a..f3c29e9326f1 100644
--- a/fs/exofs/super.c
+++ b/fs/exofs/super.c
@@ -549,27 +549,26 @@ static int exofs_devs_2_odi(struct exofs_dt_device_info *dt_dev,
 static int __alloc_dev_table(struct exofs_sb_info *sbi, unsigned numdevs,
 		      struct exofs_dev **peds)
 {
-	struct __alloc_ore_devs_and_exofs_devs {
-		/* Twice bigger table: See exofs_init_comps() and comment at
-		 * exofs_read_lookup_dev_table()
-		 */
-		struct ore_dev *oreds[numdevs * 2 - 1];
-		struct exofs_dev eds[numdevs];
-	} *aoded;
+	/* Twice bigger table: See exofs_init_comps() and comment at
+	 * exofs_read_lookup_dev_table()
+	 */
+	size_t numores = numdevs * 2 - 1;
 	struct exofs_dev *eds;
 	unsigned i;
 
-	aoded = kzalloc(sizeof(*aoded), GFP_KERNEL);
-	if (unlikely(!aoded)) {
+	sbi->oc.ods = kzalloc(numores * sizeof(struct ore_dev *) +
+			      numdevs * sizeof(struct exofs_dev), GFP_KERNEL);
+	if (unlikely(!sbi->oc.ods)) {
 		EXOFS_ERR("ERROR: failed allocating Device array[%d]\n",
 			  numdevs);
 		return -ENOMEM;
 	}
 
-	sbi->oc.ods = aoded->oreds;
-	*peds = eds = aoded->eds;
+	/* Start of allocated struct exofs_dev entries */
+	*peds = eds = (void *)sbi->oc.ods[numores];
+	/* Initialize pointers into struct exofs_dev */
 	for (i = 0; i < numdevs; ++i)
-		aoded->oreds[i] = &eds[i].ored;
+		sbi->oc.ods[i] = &eds[i].ored;
 	return 0;
 }
 
-- 
2.7.4


-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] exofs: Avoid VLA in structures
  2018-03-27 20:39 [PATCH] exofs: Avoid VLA in structures Kees Cook
@ 2018-03-27 22:10 ` Andrew Morton
  2018-03-29 21:32 ` Nick Desaulniers
  1 sibling, 0 replies; 4+ messages in thread
From: Andrew Morton @ 2018-03-27 22:10 UTC (permalink / raw)
  To: Kees Cook; +Cc: Boaz Harrosh, linux-kernel, Nick Desaulniers, kernel-hardening

On Tue, 27 Mar 2018 13:39:04 -0700 Kees Cook <keescook@chromium.org> wrote:

> On the quest to remove all VLAs from the kernel[1] this adjusts several
> cases where allocation is made after an array of structures that points
> back into the allocation. The allocations are changed to perform explicit
> calculations instead of using a Variable Length Array in a structure.
> Additionally, this lets Clang compile this code now, since Clang does not
> support VLAIS[2].
> 
> [1] https://lkml.org/lkml/2018/3/7/621
> [2] https://lkml.org/lkml/2013/9/23/500
> 
> ...
>
> I not sure the best way to test this. Kconfig implies I need special hardware?

Yeah, I was wondering about that.  It's a tricky-looking patch.

Boaz, are you able to give it a spin?

Thanks.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] exofs: Avoid VLA in structures
  2018-03-27 20:39 [PATCH] exofs: Avoid VLA in structures Kees Cook
  2018-03-27 22:10 ` Andrew Morton
@ 2018-03-29 21:32 ` Nick Desaulniers
  2018-03-30  1:26   ` Kees Cook
  1 sibling, 1 reply; 4+ messages in thread
From: Nick Desaulniers @ 2018-03-29 21:32 UTC (permalink / raw)
  To: Kees Cook; +Cc: Andrew Morton, ooo, LKML, kernel-hardening

On Tue, Mar 27, 2018 at 1:39 PM Kees Cook <keescook@chromium.org> wrote:

> On the quest to remove all VLAs from the kernel[1] this adjusts several
> cases where allocation is made after an array of structures that points
> back into the allocation. The allocations are changed to perform explicit
> calculations instead of using a Variable Length Array in a structure.
> Additionally, this lets Clang compile this code now, since Clang does not
> support VLAIS[2].

> [1] https://lkml.org/lkml/2018/3/7/621
> [2] https://lkml.org/lkml/2013/9/23/500

> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> I not sure the best way to test this. Kconfig implies I need special
hardware?
> ---
>   fs/exofs/ore.c      | 84
+++++++++++++++++++++++++++++++----------------------
>   fs/exofs/ore_raid.c | 73 ++++++++++++++++++++++++++++++++++------------
>   fs/exofs/super.c    | 23 +++++++--------
>   3 files changed, 114 insertions(+), 66 deletions(-)

> diff --git a/fs/exofs/ore.c b/fs/exofs/ore.c
> index 3c6a9c156b7a..cfa862ea19d2 100644
> --- a/fs/exofs/ore.c
> +++ b/fs/exofs/ore.c
> @@ -146,68 +146,82 @@ int  _ore_get_io_state(struct ore_layout *layout,
>                          struct ore_io_state **pios)
>   {
>          struct ore_io_state *ios;
> -       struct page **pages;
> -       struct osd_sg_entry *sgilist;
> +       size_t size_ios, size_extra, size_total;
> +       void *ios_extra;
> +
> +       /*
> +        * The desired layout looks like this, with the extra_allocation
> +        * items pointed at from fields within ios or per_dev:
> +
>          struct __alloc_all_io_state {
>                  struct ore_io_state ios;
>                  struct ore_per_dev_state per_dev[numdevs];
>                  union {
>                          struct osd_sg_entry sglist[sgs_per_dev * numdevs];
>                          struct page *pages[num_par_pages];
> -               };
> -       } *_aios;
> -
> -       if (likely(sizeof(*_aios) <= PAGE_SIZE)) {
> -               _aios = kzalloc(sizeof(*_aios), GFP_KERNEL);
> -               if (unlikely(!_aios)) {
> -                       ORE_DBGMSG("Failed kzalloc bytes=%zd\n",
> -                                  sizeof(*_aios));
> +               } extra_allocation;
> +       } whole_allocation;
> +
> +       */
> +
> +       /* This should never happen, so abort early if it ever does. */
> +       if (sgs_per_dev && num_par_pages) {
> +               ORE_DBGMSG("Tried to use both pages and sglist\n");
> +               *pios = NULL;
> +               return -EINVAL;
> +       }
> +
> +       if (numdevs > (INT_MAX - sizeof(*ios)) /
> +                      sizeof(struct ore_per_dev_state))
> +               return -ENOMEM;
> +       size_ios = sizeof(*ios) + sizeof(struct ore_per_dev_state) *
numdevs;

Can all these invariant checks that return -ENOMEM be grouped together, as
it seems we have ...

> +
> +       if (sgs_per_dev * numdevs > INT_MAX / sizeof(struct osd_sg_entry))
> +               return -ENOMEM;
> +       if (num_par_pages > INT_MAX / sizeof(struct page *))
> +               return -ENOMEM;

...a whole bunch of single conditions with the same body, can be combined
with one:

if (a)
   return d;
if (b)
   return d;
if (c)
   return d;

->

if (a || b || c)
   return d;

> +       size_extra = max(sizeof(struct osd_sg_entry) * (sgs_per_dev *
numdevs),

Do you need parens around the sub-expression `(sgs_per_dev * numdevs)`?

> +                        sizeof(struct page *) * num_par_pages);
> +
> +       size_total = size_ios + size_extra;
> +
> +       if (likely(size_total <= PAGE_SIZE)) {
> +               ios = kzalloc(size_total, GFP_KERNEL);
> +               if (unlikely(!ios)) {
> +                       ORE_DBGMSG("Failed kzalloc bytes=%zd\n",
size_total);
>                          *pios = NULL;
>                          return -ENOMEM;
>                  }
> -               pages = num_par_pages ? _aios->pages : NULL;
> -               sgilist = sgs_per_dev ? _aios->sglist : NULL;
> -               ios = &_aios->ios;
> +               ios_extra = (char *)ios + size_ios;
>          } else {
> -               struct __alloc_small_io_state {
> -                       struct ore_io_state ios;
> -                       struct ore_per_dev_state per_dev[numdevs];
> -               } *_aio_small;
> -               union __extra_part {
> -                       struct osd_sg_entry sglist[sgs_per_dev * numdevs];
> -                       struct page *pages[num_par_pages];
> -               } *extra_part;
> -
> -               _aio_small = kzalloc(sizeof(*_aio_small), GFP_KERNEL);
> -               if (unlikely(!_aio_small)) {
> +               ios = kzalloc(size_ios, GFP_KERNEL);
> +               if (unlikely(!ios)) {
>                          ORE_DBGMSG("Failed alloc first part bytes=%zd\n",
> -                                  sizeof(*_aio_small));
> +                                  size_ios);
>                          *pios = NULL;
>                          return -ENOMEM;
>                  }
> -               extra_part = kzalloc(sizeof(*extra_part), GFP_KERNEL);
> -               if (unlikely(!extra_part)) {
> +               ios_extra = kzalloc(size_extra, GFP_KERNEL);
> +               if (unlikely(!ios_extra)) {
>                          ORE_DBGMSG("Failed alloc second part bytes=%zd\n",
> -                                  sizeof(*extra_part));
> -                       kfree(_aio_small);
> +                                  size_extra);
> +                       kfree(ios);
>                          *pios = NULL;
>                          return -ENOMEM;
>                  }

> -               pages = num_par_pages ? extra_part->pages : NULL;
> -               sgilist = sgs_per_dev ? extra_part->sglist : NULL;
>                  /* In this case the per_dev[0].sgilist holds the pointer
to
>                   * be freed
>                   */
> -               ios = &_aio_small->ios;
>                  ios->extra_part_alloc = true;
>          }

> -       if (pages) {
> -               ios->parity_pages = pages;
> +       if (num_par_pages) {
> +               ios->parity_pages = ios_extra;
>                  ios->max_par_pages = num_par_pages;
>          }
> -       if (sgilist) {
> +       if (sgs_per_dev) {
> +               struct osd_sg_entry *sgilist = ios_extra;
>                  unsigned d;

>                  for (d = 0; d < numdevs; ++d) {
> diff --git a/fs/exofs/ore_raid.c b/fs/exofs/ore_raid.c
> index 27cbdb697649..659129d5e9f7 100644
> --- a/fs/exofs/ore_raid.c
> +++ b/fs/exofs/ore_raid.c
> @@ -71,6 +71,11 @@ static int _sp2d_alloc(unsigned pages_in_unit,
unsigned group_width,
>   {
>          struct __stripe_pages_2d *sp2d;
>          unsigned data_devs = group_width - parity;
> +
> +       /*
> +        * Desired allocation layout is, though when larger than
PAGE_SIZE,
> +        * each struct __alloc_1p_arrays is separately allocated:
> +
>          struct _alloc_all_bytes {
>                  struct __alloc_stripe_pages_2d {
>                          struct __stripe_pages_2d sp2d;
> @@ -82,55 +87,85 @@ static int _sp2d_alloc(unsigned pages_in_unit,
unsigned group_width,
>                          char page_is_read[data_devs];
>                  } __a1pa[pages_in_unit];
>          } *_aab;
> +
>          struct __alloc_1p_arrays *__a1pa;
>          struct __alloc_1p_arrays *__a1pa_end;
> -       const unsigned sizeof__a1pa = sizeof(_aab->__a1pa[0]);
> +
> +       */
> +
> +       char *__a1pa;
> +       char *__a1pa_end;
> +

Just my notes, since this block could use another eye for review:

> +       const size_t sizeof_stripe_pages_2d =
> +               sizeof(struct __stripe_pages_2d) +
> +               sizeof(struct __1_page_stripe) * pages_in_unit;

Ok, so this replaces sizeof(_aab->__asp2d).

> +       const size_t sizeof__a1pa =
> +               ALIGN(sizeof(struct page *) * (2 * group_width) +
data_devs,
> +                     sizeof(void *));

And this replaces sizeof(_aab->__a1pa[0]);

> +       const size_t sizeof__a1pa_arrays = sizeof__a1pa * pages_in_unit;

This replaces sizeof(_aab->__a1pa[pages_in_unit]);

> +       const size_t alloc_total = sizeof_stripe_pages_2d +
> +                                  sizeof__a1pa_arrays;

Replaces sizeof(*_aab);

This is the trickiest part of this patch IMO, I think it needs careful
review, but looks correct to me.

> +
>          unsigned num_a1pa, alloc_size, i;

>          /* FIXME: check these numbers in ore_verify_layout */
> -       BUG_ON(sizeof(_aab->__asp2d) > PAGE_SIZE);
> +       BUG_ON(sizeof_stripe_pages_2d > PAGE_SIZE);
>          BUG_ON(sizeof__a1pa > PAGE_SIZE);

> -       if (sizeof(*_aab) > PAGE_SIZE) {
> -               num_a1pa = (PAGE_SIZE - sizeof(_aab->__asp2d)) /
sizeof__a1pa;
> -               alloc_size = sizeof(_aab->__asp2d) + sizeof__a1pa *
num_a1pa;
> +       /*
> +        * If alloc_total would be larger than PAGE_SIZE, only allocate
> +        * as many a1pa items as would fill the rest of the page, instead
> +        * of the full pages_in_unit count.
> +        */
> +       if (alloc_total > PAGE_SIZE) {
> +               num_a1pa = (PAGE_SIZE - sizeof_stripe_pages_2d) /
sizeof__a1pa;
> +               alloc_size = sizeof_stripe_pages_2d + sizeof__a1pa *
num_a1pa;
>          } else {
>                  num_a1pa = pages_in_unit;
> -               alloc_size = sizeof(*_aab);
> +               alloc_size = alloc_total;
>          }

> -       _aab = kzalloc(alloc_size, GFP_KERNEL);
> -       if (unlikely(!_aab)) {
> +       *psp2d = sp2d = kzalloc(alloc_size, GFP_KERNEL);
> +       if (unlikely(!sp2d)) {
>                  ORE_DBGMSG("!! Failed to alloc sp2d size=%d\n",
alloc_size);
>                  return -ENOMEM;
>          }
> +       /* From here Just call _sp2d_free */

> -       sp2d = &_aab->__asp2d.sp2d;
> -       *psp2d = sp2d; /* From here Just call _sp2d_free */
> -
> -       __a1pa = _aab->__a1pa;
> -       __a1pa_end = __a1pa + num_a1pa;
> +       /* Find start of a1pa area. */
> +       __a1pa = (char *)sp2d + sizeof_stripe_pages_2d;
> +       /* Find end of the _allocated_ a1pa area. */
> +       __a1pa_end = __a1pa + alloc_size;

> +       /* Allocate additionally needed a1pa items in PAGE_SIZE chunks. */
>          for (i = 0; i < pages_in_unit; ++i) {
>                  if (unlikely(__a1pa >= __a1pa_end)) {
>                          num_a1pa = min_t(unsigned, PAGE_SIZE /
sizeof__a1pa,
>                                                          pages_in_unit -
i);
> +                       alloc_size = sizeof__a1pa * num_a1pa;

> -                       __a1pa = kcalloc(num_a1pa, sizeof__a1pa,
GFP_KERNEL);
> +                       __a1pa = kzalloc(alloc_size, GFP_KERNEL);
>                          if (unlikely(!__a1pa)) {
>                                  ORE_DBGMSG("!! Failed to
_alloc_1p_arrays=%d\n",
>                                             num_a1pa);
>                                  return -ENOMEM;
>                          }
> -                       __a1pa_end = __a1pa + num_a1pa;
> +                       __a1pa_end = __a1pa + alloc_size;
>                          /* First *pages is marked for kfree of the buffer
*/
>                          sp2d->_1p_stripes[i].alloc = true;
>                  }

> -               sp2d->_1p_stripes[i].pages = __a1pa->pages;
> -               sp2d->_1p_stripes[i].scribble = __a1pa->scribble ;
> -               sp2d->_1p_stripes[i].page_is_read = __a1pa->page_is_read;
> -               ++__a1pa;
> +               /*
> +                * Attach all _lp_stripes pointers to the allocation for
> +                * it which was either part of the original PAGE_SIZE
> +                * allocation or the subsequent allocation in this loop.
> +                */
> +               sp2d->_1p_stripes[i].pages = (void *)__a1pa;
> +               sp2d->_1p_stripes[i].scribble =
> +                       sp2d->_1p_stripes[i].pages + group_width;
> +               sp2d->_1p_stripes[i].page_is_read =
> +                       (char *)(sp2d->_1p_stripes[i].scribble +
group_width);

Can you DRY up `sp2d->_1p_stripes[i]`? ex.

struct _1p_stripes* stripe;

for (i = 0; i < pages_in_unit; ...
   ...
   stripe = &sp2d->_1p_stripes[i];
   stripe->pages = (void*)__a1pa;
   stripe->scribble = stripe->pages + group_width;
   stripe->page_is_read = (char*)stripe->scribble + group_width;

> +               __a1pa += sizeof__a1pa;
>          }

>          sp2d->parity = parity;
> diff --git a/fs/exofs/super.c b/fs/exofs/super.c
> index 179cd5c2f52a..f3c29e9326f1 100644
> --- a/fs/exofs/super.c
> +++ b/fs/exofs/super.c
> @@ -549,27 +549,26 @@ static int exofs_devs_2_odi(struct
exofs_dt_device_info *dt_dev,
>   static int __alloc_dev_table(struct exofs_sb_info *sbi, unsigned numdevs,
>                        struct exofs_dev **peds)
>   {
> -       struct __alloc_ore_devs_and_exofs_devs {
> -               /* Twice bigger table: See exofs_init_comps() and comment
at
> -                * exofs_read_lookup_dev_table()
> -                */
> -               struct ore_dev *oreds[numdevs * 2 - 1];
> -               struct exofs_dev eds[numdevs];
> -       } *aoded;
> +       /* Twice bigger table: See exofs_init_comps() and comment at
> +        * exofs_read_lookup_dev_table()
> +        */
> +       size_t numores = numdevs * 2 - 1;

const size_t

>          struct exofs_dev *eds;
>          unsigned i;

> -       aoded = kzalloc(sizeof(*aoded), GFP_KERNEL);
> -       if (unlikely(!aoded)) {
> +       sbi->oc.ods = kzalloc(numores * sizeof(struct ore_dev *) +
> +                             numdevs * sizeof(struct exofs_dev),
GFP_KERNEL);
> +       if (unlikely(!sbi->oc.ods)) {
>                  EXOFS_ERR("ERROR: failed allocating Device array[%d]\n",
>                            numdevs);
>                  return -ENOMEM;
>          }

> -       sbi->oc.ods = aoded->oreds;
> -       *peds = eds = aoded->eds;
> +       /* Start of allocated struct exofs_dev entries */
> +       *peds = eds = (void *)sbi->oc.ods[numores];
> +       /* Initialize pointers into struct exofs_dev */
>          for (i = 0; i < numdevs; ++i)
> -               aoded->oreds[i] = &eds[i].ored;
> +               sbi->oc.ods[i] = &eds[i].ored;
>          return 0;
>   }

> --
> 2.7.4


> --
> Kees Cook
> Pixel Security

The sizeof calculations replacing the VLAIS I pointed out are pretty tricky
to follow, but I feel pretty confident about this patch.  With the changes
I recommend, feel free to add my Reviewed-by tag.  It would be good to get
additional review and testing from the maintainer.

Thanks for taking the time to tackle this.

--
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] exofs: Avoid VLA in structures
  2018-03-29 21:32 ` Nick Desaulniers
@ 2018-03-30  1:26   ` Kees Cook
  0 siblings, 0 replies; 4+ messages in thread
From: Kees Cook @ 2018-03-30  1:26 UTC (permalink / raw)
  To: Nick Desaulniers; +Cc: Andrew Morton, Boaz Harrosh, LKML, Kernel Hardening

On Thu, Mar 29, 2018 at 2:32 PM, Nick Desaulniers
<ndesaulniers@google.com> wrote:
> On Tue, Mar 27, 2018 at 1:39 PM Kees Cook <keescook@chromium.org> wrote:
>>[...]
>> +       if (numdevs > (INT_MAX - sizeof(*ios)) /
>> +                      sizeof(struct ore_per_dev_state))
>> +               return -ENOMEM;
>> +       size_ios = sizeof(*ios) + sizeof(struct ore_per_dev_state) *
> numdevs;
>
> Can all these invariant checks that return -ENOMEM be grouped together, as
> it seems we have ...
>
>> +
>> +       if (sgs_per_dev * numdevs > INT_MAX / sizeof(struct osd_sg_entry))
>> +               return -ENOMEM;
>> +       if (num_par_pages > INT_MAX / sizeof(struct page *))
>> +               return -ENOMEM;
>
> ...a whole bunch of single conditions with the same body, can be combined
> with one:
>
> if (a)
>    return d;
> if (b)
>    return d;
> if (c)
>    return d;
>
> ->
>
> if (a || b || c)
>    return d;

I find that harder to read, so I let the compiler do the grouping for
me. As each "if" maps to a portion of the assignment that follows it,
I like it how it is. If there's agreement that they should be grouped,
that's fine by me, of course. :)

>> +       size_extra = max(sizeof(struct osd_sg_entry) * (sgs_per_dev *
> numdevs),
>
> Do you need parens around the sub-expression `(sgs_per_dev * numdevs)`?

No, max() correctly protects those.

>> diff --git a/fs/exofs/ore_raid.c b/fs/exofs/ore_raid.c
>> index 27cbdb697649..659129d5e9f7 100644
>> --- a/fs/exofs/ore_raid.c
>> +++ b/fs/exofs/ore_raid.c
>> @@ -71,6 +71,11 @@ static int _sp2d_alloc(unsigned pages_in_unit,
> unsigned group_width,
>>   {
>>          struct __stripe_pages_2d *sp2d;
>>          unsigned data_devs = group_width - parity;
>> +
>> +       /*
>> +        * Desired allocation layout is, though when larger than
> PAGE_SIZE,
>> +        * each struct __alloc_1p_arrays is separately allocated:
>> +
>>          struct _alloc_all_bytes {
>>                  struct __alloc_stripe_pages_2d {
>>                          struct __stripe_pages_2d sp2d;
>> @@ -82,55 +87,85 @@ static int _sp2d_alloc(unsigned pages_in_unit,
> unsigned group_width,
>>                          char page_is_read[data_devs];
>>                  } __a1pa[pages_in_unit];
>>          } *_aab;
>> +
>>          struct __alloc_1p_arrays *__a1pa;
>>          struct __alloc_1p_arrays *__a1pa_end;
>> -       const unsigned sizeof__a1pa = sizeof(_aab->__a1pa[0]);
>> +
>> +       */
>> +
>> +       char *__a1pa;
>> +       char *__a1pa_end;
>> +
>
> Just my notes, since this block could use another eye for review:

Tell me about it. :P These could may be named better, but I'm not
familiar with the naming conventions of this code...

>
>> +       const size_t sizeof_stripe_pages_2d =
>> +               sizeof(struct __stripe_pages_2d) +
>> +               sizeof(struct __1_page_stripe) * pages_in_unit;
>
> Ok, so this replaces sizeof(_aab->__asp2d).
>
>> +       const size_t sizeof__a1pa =
>> +               ALIGN(sizeof(struct page *) * (2 * group_width) +
> data_devs,
>> +                     sizeof(void *));
>
> And this replaces sizeof(_aab->__a1pa[0]);
>
>> +       const size_t sizeof__a1pa_arrays = sizeof__a1pa * pages_in_unit;
>
> This replaces sizeof(_aab->__a1pa[pages_in_unit]);

Technically what you've written is still a single element of the array
(same as _aab->__a1pa[0] above). This replaces sizeof(_aab->__a1pa)
(the entire array size in bytes).

>
>> +       const size_t alloc_total = sizeof_stripe_pages_2d +
>> +                                  sizeof__a1pa_arrays;
>
> Replaces sizeof(*_aab);
>
> This is the trickiest part of this patch IMO, I think it needs careful
> review, but looks correct to me.

Thanks! That's why I added a bunch of comments. It was not obvious to
me what was happening.

>> -               sp2d->_1p_stripes[i].pages = __a1pa->pages;
>> -               sp2d->_1p_stripes[i].scribble = __a1pa->scribble ;
>> -               sp2d->_1p_stripes[i].page_is_read = __a1pa->page_is_read;
>> -               ++__a1pa;
>> +               /*
>> +                * Attach all _lp_stripes pointers to the allocation for
>> +                * it which was either part of the original PAGE_SIZE
>> +                * allocation or the subsequent allocation in this loop.
>> +                */
>> +               sp2d->_1p_stripes[i].pages = (void *)__a1pa;
>> +               sp2d->_1p_stripes[i].scribble =
>> +                       sp2d->_1p_stripes[i].pages + group_width;
>> +               sp2d->_1p_stripes[i].page_is_read =
>> +                       (char *)(sp2d->_1p_stripes[i].scribble +
> group_width);
>
> Can you DRY up `sp2d->_1p_stripes[i]`? ex.
>
> struct _1p_stripes* stripe;
>
> for (i = 0; i < pages_in_unit; ...
>    ...
>    stripe = &sp2d->_1p_stripes[i];
>    stripe->pages = (void*)__a1pa;
>    stripe->scribble = stripe->pages + group_width;
>    stripe->page_is_read = (char*)stripe->scribble + group_width;

Yeah, that could make it more readable.

>
>> +               __a1pa += sizeof__a1pa;
>>          }
>
>>          sp2d->parity = parity;
>> diff --git a/fs/exofs/super.c b/fs/exofs/super.c
>> index 179cd5c2f52a..f3c29e9326f1 100644
>> --- a/fs/exofs/super.c
>> +++ b/fs/exofs/super.c
>> @@ -549,27 +549,26 @@ static int exofs_devs_2_odi(struct
> exofs_dt_device_info *dt_dev,
>>   static int __alloc_dev_table(struct exofs_sb_info *sbi, unsigned numdevs,
>>                        struct exofs_dev **peds)
>>   {
>> -       struct __alloc_ore_devs_and_exofs_devs {
>> -               /* Twice bigger table: See exofs_init_comps() and comment
> at
>> -                * exofs_read_lookup_dev_table()
>> -                */
>> -               struct ore_dev *oreds[numdevs * 2 - 1];
>> -               struct exofs_dev eds[numdevs];
>> -       } *aoded;
>> +       /* Twice bigger table: See exofs_init_comps() and comment at
>> +        * exofs_read_lookup_dev_table()
>> +        */
>> +       size_t numores = numdevs * 2 - 1;
>
> const size_t

Good call.

> The sizeof calculations replacing the VLAIS I pointed out are pretty tricky
> to follow, but I feel pretty confident about this patch.  With the changes
> I recommend, feel free to add my Reviewed-by tag.  It would be good to get
> additional review and testing from the maintainer.

Yes, agreed. I'll send a v2 with your suggestions.

> Thanks for taking the time to tackle this.

Thanks for the review!

-Kees

-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2018-03-30  1:26 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-27 20:39 [PATCH] exofs: Avoid VLA in structures Kees Cook
2018-03-27 22:10 ` Andrew Morton
2018-03-29 21:32 ` Nick Desaulniers
2018-03-30  1:26   ` Kees Cook

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.