linux-kernel - Re: [dm-devel] [PATCH v3 9/9] crypto: shash: Remove VLA usage in unaligned hashing

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 2 Jul 2018 10:34:46 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Eric Biggers <ebiggers3@...il.com>
Cc:     Herbert Xu <herbert@...dor.apana.org.au>,
        Giovanni Cabiddu <giovanni.cabiddu@...el.com>,
        Arnd Bergmann <arnd@...db.de>,
        Eric Biggers <ebiggers@...gle.com>,
        Mike Snitzer <snitzer@...hat.com>,
        "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
        qat-linux@...el.com, LKML <linux-kernel@...r.kernel.org>,
        dm-devel@...hat.com, linux-crypto <linux-crypto@...r.kernel.org>,
        Lars Persson <larper@...s.com>,
        Tim Chen <tim.c.chen@...ux.intel.com>,
        "David S. Miller" <davem@...emloft.net>,
        Alasdair Kergon <agk@...hat.com>,
        Rabin Vincent <rabinv@...s.com>
Subject: Re: [dm-devel] [PATCH v3 9/9] crypto: shash: Remove VLA usage in
 unaligned hashing

On Sun, Jul 1, 2018 at 10:20 AM, Eric Biggers <ebiggers3@...il.com> wrote:
> On Sun, Jul 01, 2018 at 10:04:59AM -0700, Kees Cook wrote:
>> On Sat, Jun 30, 2018 at 12:03 AM, Eric Biggers <ebiggers3@...il.com> wrote:
>> > On Thu, Jun 28, 2018 at 05:28:43PM -0700, Kees Cook wrote:
>> >> @@ -88,11 +81,13 @@ static int shash_update_unaligned(struct shash_desc *desc, const u8 *data,
>> >>       unsigned long alignmask = crypto_shash_alignmask(tfm);
>> >>       unsigned int unaligned_len = alignmask + 1 -
>> >>                                    ((unsigned long)data & alignmask);
>> >> -     u8 ubuf[shash_align_buffer_size(unaligned_len, alignmask)]
>> >> -             __aligned_largest;
>> >> +     u8 ubuf[MAX_ALGAPI_ALIGNMASK + 1];
>> >>       u8 *buf = PTR_ALIGN(&ubuf[0], alignmask + 1);
>> >>       int err;
>> >>
>> >> +     if (WARN_ON(buf + unaligned_len > ubuf + sizeof(ubuf)))
>> >> +             return -EINVAL;
>> >> +
>> >
>> > How is 'ubuf' guaranteed to be large enough?  You removed the __aligned
>> > attribute, so 'ubuf' can have any alignment.  So the aligned pointer 'buf' may
>> > be as high as '&ubuf[alignmask]'.  Then, up to 'alignmask' bytes of data will be
>> > copied into 'buf'... resulting in up to '2 * alignmask' bytes needed in 'ubuf'.
>> > But you've only guaranteed 'alignmask + 1' bytes.
>>
>> Hm, good point. Adding __aligned(MAX_ALGAPI_ALIGNMASK + 1) looks to
>> fix this, yes?
>>
>> Also, if __aligned() is used here, can't PTR_ALIGN() be dropped? (I
>> think you pointed this out earlier.)
>
> Sure, I'm just not sure whether __aligned() with such a large alignment is
> guaranteed to work on stack variables on all architectures.  See e.g.
> https://patchwork.kernel.org/patch/9507697/.

That's terrible. :( That seems like a compiler bug, but okay.

>> Also, is "unaligned_len" being calculated correctly? Let's say
>> alignmask is 63. If data is binary ...111111, then unaligned_len will
>> be 64 - 63 == 1, which is fine: we copy 1 byte out, bump the address
>> by 1, and we're happily aligned to ...000000. If data is ...000000,
>> then unaligned_len will be 64. But it should be 0. Shouldn't this be:
>>
>> unsigned int unaligned_len;
>>
>> unaligned_len = (unsigned long)data & alignmask;
>> if (unaligned_len)
>>     unaligned_len = alignmask + 1 - unaligned_len;
>>
>> And then ubuf only needs to be MAX_ALGAPI_ALIGNMASK, without the +1?
>
> shash_update_unaligned() is only called when 'data & alignmask'.
> Similarly with shash_final_unaligned().

Ah! I see that now.

> Though, calculating 'unaligned_len' could be simplified to
>
>         unsigned int unaligned_len = -(unsigned long)data & alignmask;
>
> which works either way.

So, since we can't depend on __aligned() working, I'll just keep the
PTR_ALIGN and add MAX_ALGAPI_ALIGNMASK to each array. That'll be less
memory-efficient, but it'll actually get aligned correctly.

-Kees

-- 
Kees Cook
Pixel Security