Hi Stephan, On Tue, Dec 12, 2017 at 07:09:08AM +0100, Stephan Müller wrote: > Hi Herbert, > > you see the reported problem by simply using > > sa.salg_mask = 0xffffffff; > > Note, I am not fully sure about whether CRYPTO_AF_ALG_ALLOWED_MASK and > CRYPTO_AF_ALG_ALLOWED_TYPE have the correct value. But I think that all > that user space should reach is potentially the ASYNC flag and the > cipher types flags. > > ---8<--- > > The user space interface allows specifying the type and the mask field > used to allocate the cipher. Only a subset of the type and mask is > considered relevant to be set by user space if needed at all. > > This fixes a bug where user space is able to cause one cipher to be > registered multiple times potentially exhausting kernel memory. > > Reported-by: syzbot <syzkaller@xxxxxxxxxxxxxxxx> > Cc: <stable@xxxxxxxxxxxxxxx> > Signed-off-by: Stephan Mueller <smueller@xxxxxxxxxx> The syzkaller reproducer triggered a crash in crypto_remove_spawns(). Is it possible the bug is still there somewhere, while this patch just makes it inaccessible through AF_ALG? Anyway, we definitely should expose as few algorithm flags to AF_ALG as possible. There are just way too many things that can go wrong with exposing arbitrary flags. However, why do the check in every af_alg_type.bind() method instead of just once in alg_bind()? If it can be done without breaking users, it also would be nice if we would actually validate the flags and return -EINVAL if unknown flags are specified. Otherwise users cannot test for whether specific flags are supported. Also, note that even after this fix there are still ways to register an arbitrarily large number of algorithms. There are two classes of problems. First, it can happen that a template gets instantiated for a request but the resulting algorithm does not exactly match the original request, so making the same request again will instantiate the template again. This could happen by specifically requesting an untested algorithm (type=0, mask=CRYPTO_ALG_TESTED), which your patch fixes. However this can also happen in cases where neither the final ->cra_name nor the final ->cra_driver_name matches what was requested. For example asking for "cryptd(sha1)" results in .cra_name = "sha1" and .cra_driver_name = "cryptd(sha1-avx2)", or asking for "xts(ecb(aes))" results in .cra_name = "xts(aes)" and .cra_driver_name = "xts(ecb-aes-aesni)". Probably the crypto API needs to be taught how to find the instantiated templates correctly. Second, you can just keep choosing different combinations of algorithms when instantiating templates, taking advantage of the fact that templates can be nested and some take multiple parameters, so the number of possible combinations grows exponentially. I don't know how to easily solve this. Perhaps crypto_free_skcipher(), crypto_free_ahash(), etc. should unregister the algorithm if it was created from a template and nothing else is using it; then the number of algorithms someone could instantiate via AF_ALG at a given time would be limited by their number of file descriptors. Eric
