| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAGXu5j+O+P+tzgSpW7Uj90hZZPtW70KSNxfAHAJ5o0U0Kskt=Q@mail.gmail.com>
Date: Thu, 29 Mar 2018 18:26:25 -0700
From: Kees Cook <keescook@...omium.org>
To: Nick Desaulniers <ndesaulniers@...gle.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
Boaz Harrosh <ooo@...ctrozaur.com>,
LKML <linux-kernel@...r.kernel.org>,
Kernel Hardening <kernel-hardening@...ts.openwall.com>
Subject: Re: [PATCH] exofs: Avoid VLA in structures
On Thu, Mar 29, 2018 at 2:32 PM, Nick Desaulniers
<ndesaulniers@...gle.com> wrote:
> On Tue, Mar 27, 2018 at 1:39 PM Kees Cook <keescook@...omium.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
Powered by blists - more mailing lists