Am Dienstag, 13. September 2016, 13:27:34 CET schrieb Stephan Mueller:
Hi Herbert,
> Am Dienstag, 13. September 2016, 18:08:16 CEST schrieb Herbert Xu:
>
> Hi Herbert,
>
> > This patch appears to be papering over a real bug.
> >
> > The async path should be exactly the same as the sync path, except
> > that we don't wait for completion. So the question is why are we
> > getting this crash here for async but not sync?
>
> At least one reason is found in skcipher_recvmsg_async with the following
> code path:
>
> if (txbufs == tx_nents) {
> struct scatterlist *tmp;
> int x;
> /* Ran out of tx slots in async request
> * need to expand */
> tmp = kcalloc(tx_nents * 2, sizeof(*tmp),
> GFP_KERNEL);
> if (!tmp)
> goto free;
>
> sg_init_table(tmp, tx_nents * 2);
> for (x = 0; x < tx_nents; x++)
> sg_set_page(&tmp[x], sg_page(&sreq->tsg[x]),
> sreq->tsg[x].length,
> sreq->tsg[x].offset);
> kfree(sreq->tsg);
> sreq->tsg = tmp;
> tx_nents *= 2;
> mark = true;
> }
>
>
> ==> the code allocates twice the amount of the previously existing memory,
> copies the existing SGs over, but does not set the remaining SGs to
> anything. If the caller provides less pages than the number of allocated
> SGs, some SGs are unset. Hence, the deallocation must not do anything with
> the yet uninitialized SGs.
I looked into the issue a bit deeper. In addition to the aforementioned code,
the following code seems to be a second culprit:
tx_nents = skcipher_all_sg_nents(ctx);
sreq->tsg = kcalloc(tx_nents, sizeof(*sg), GFP_KERNEL);
if (unlikely(!sreq->tsg))
goto unlock;
sg_init_table(sreq->tsg, tx_nents);
Here again, an SGL is initialized, but there are no pages mapped to the SGs.
May I ask you to reconsider this patch as well as the patch "[PATCH] crypto:
call put_page on used pages only" from September 10 since the current code of
libkcapi can easily trigger these bugs and lead to a kernel crash.
If you consider the patches papering over the heart of the problem, may I ask
for suggestions on how the mentioned code should be changed such that the
issues are removed? If the suggestion is to re-architect the memory handling
in the async part, may I ask to at least apply the patches for now with the
goal to have time for re-architecting the async code and yet have no open
holes that lead to crashes?
Thanks.
Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
