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@xxxxxxxxx> 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/. > > 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(). Though, calculating 'unaligned_len' could be simplified to unsigned int unaligned_len = -(unsigned long)data & alignmask; which works either way. - Eric
