Am Montag, 9. Januar 2017, 14:08:23 CET schrieb Gilad Ben-Yossef:
Hi Gilad,
> Hello Stephen,
>
> Before getting to business I wish to offer my thanks for hosting the
> kernel crypto documentation on your web site at chronox.de. It has
> proven very useful to me :-)
I am glad that it is helpful.
>
> On Mon, Jan 9, 2017 at 12:23 PM, Stephan Müller <smueller@xxxxxxxxxx> wrote:
> > Am Sonntag, 8. Januar 2017, 15:45:37 CET schrieb Gilad Ben-Yossef:
> >
> > Hi Gilad,
> >
> >> ahash_request_set_callback(req, 0, NULL, NULL);
> >>
> >> Would anyone be kind enough to enlighten me?
> >
> > The documentation got out of sync with the real world. I will file a patch
> > for that shortly.
> >
> > The ahash API works identically to the async skcipher API for which you
> > find an example in the api-samples.rst. There you see that with the
> > set_callback, a function is registered that is triggered upon completion
> > of the operation.
> >
> > Thus, use the callback example you find for skcipher for your ahash
> > operation and you get an async operation.
>
> Thank you very much for pointing out the skcipher example. It is very
> helpful.
>
> However, I suspect there is something a miss here beyond documentation:
>
> There is (quite a lot of) kernel code calling
> ahash_request_set_callback() with a NULL callback and consequently not
> performing any synchronization in the completion of the hashing
> operations.
I think this is legacy, although I cannot say for sure. As of now, there is
hardly any real ahash implementation out there. The only one I am aware of is
the sha1-mb/sha256-mb/sha512-mb for x86. There, the kernel would even crash if
there is a NULL as callback, if I read the code correctly.
For all other implementations, they are synchronous even though you use the
ahash API. I.e. all of those implementations would not trigger the callback
function.
>
> See for example crypt_iv_essiv_init() in drivers/md/dm-crypt.c
> Other similar call sites can be found at block/drbd/drbd_worker.c,
> net/ppp/ppp_mppe.c, net/wireless/intersil/orinoco/mic.c,
> scsi/iscsi_tcp.c, target/iscsi/iscsi_target_login.c
>
> As far as I could tell the code in crypto/ahash.c does not take any
> special consideration of the case where a NULL call back function has
> been set and at least one of the underlying ahash algorithm provider
> will crash if used like this.
I do not think that the ahash.c code crashes, but look into the sha1-mb
implementation:
...
req = cast_mcryptd_ctx_to_req(req_ctx);
if (irqs_disabled())
req_ctx->complete(&req->base, ret);
else {
local_bh_disable();
req_ctx->complete(&req->base, ret);
local_bh_enable();
...
Here you see the invocation of complete without a check.
>
> This seems broken to me. I would be very happy to offer to fix the
> broken call sites, if you can only confirm my understanding that
> indeed cases which register a NULL callback are broken and it is not
> just a misunderstanding on my part.
IMHO, the use of a NULL callback works, but should definitely converted to a
real callback function.
>
> Many thanks,
> Gilad
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
