| Message ID | 20180310232231.19191-1-ebiggers3@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
On Sat, Mar 10, 2018 at 03:22:31PM -0800, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > If the pcrypt template is used multiple times in an algorithm, then a > deadlock occurs because all pcrypt instances share the same > padata_instance, which completes requests in the order submitted. That > is, the inner pcrypt request waits for the outer pcrypt request while > the outer request is already waiting for the inner. > > Fix this by making pcrypt forbid instantiation if pcrypt appears in the > underlying ->cra_driver_name. This is somewhat of a hack, but it's a > simple fix that should be sufficient to prevent the deadlock. > > Reproducer: > > #include <linux/if_alg.h> > #include <sys/socket.h> > #include <unistd.h> > > int main() > { > struct sockaddr_alg addr = { > .salg_type = "aead", > .salg_name = "pcrypt(pcrypt(rfc4106-gcm-aesni))" > }; > int algfd, reqfd; > char buf[32] = { 0 }; > > algfd = socket(AF_ALG, SOCK_SEQPACKET, 0); > bind(algfd, (void *)&addr, sizeof(addr)); > setsockopt(algfd, SOL_ALG, ALG_SET_KEY, buf, 20); > reqfd = accept(algfd, 0, 0); > write(reqfd, buf, 32); > read(reqfd, buf, 16); > } > > Reported-by: syzbot+56c7151cad94eec37c521f0e47d2eee53f9361c4@syzkaller.appspotmail.com > Fixes: 5068c7a883d1 ("crypto: pcrypt - Add pcrypt crypto parallelization wrapper") > Cc: <stable@vger.kernel.org> # v2.6.34+ > Signed-off-by: Eric Biggers <ebiggers@google.com> > --- > crypto/pcrypt.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/crypto/pcrypt.c b/crypto/pcrypt.c > index f8ec3d4ba4a80..3ec64604f6a56 100644 > --- a/crypto/pcrypt.c > +++ b/crypto/pcrypt.c > @@ -265,6 +265,12 @@ static void pcrypt_free(struct aead_instance *inst) > static int pcrypt_init_instance(struct crypto_instance *inst, > struct crypto_alg *alg) > { > + /* Recursive pcrypt deadlocks due to the shared padata_instance */ > + if (!strncmp(alg->cra_driver_name, "pcrypt(", 7) || > + strstr(alg->cra_driver_name, "(pcrypt(") || > + strstr(alg->cra_driver_name, ",pcrypt(")) > + return -EINVAL; > + This looks ok to me. Acked-by: Steffen Klassert <steffen.klassert@secunet.com>
On Sat, Mar 10, 2018 at 03:22:31PM -0800, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > If the pcrypt template is used multiple times in an algorithm, then a > deadlock occurs because all pcrypt instances share the same > padata_instance, which completes requests in the order submitted. That > is, the inner pcrypt request waits for the outer pcrypt request while > the outer request is already waiting for the inner. > > Fix this by making pcrypt forbid instantiation if pcrypt appears in the > underlying ->cra_driver_name. This is somewhat of a hack, but it's a > simple fix that should be sufficient to prevent the deadlock. I'm a bit uneasy with this fix. What if pcrypt is used in the underlying algorithm as a fallback? Wouldn't it still dead-lock without triggering this check? Thanks,
On Fri, Mar 23, 2018 at 08:21:52AM +0800, Herbert Xu wrote: > On Sat, Mar 10, 2018 at 03:22:31PM -0800, Eric Biggers wrote: > > From: Eric Biggers <ebiggers@google.com> > > > > If the pcrypt template is used multiple times in an algorithm, then a > > deadlock occurs because all pcrypt instances share the same > > padata_instance, which completes requests in the order submitted. That > > is, the inner pcrypt request waits for the outer pcrypt request while > > the outer request is already waiting for the inner. > > > > Fix this by making pcrypt forbid instantiation if pcrypt appears in the > > underlying ->cra_driver_name. This is somewhat of a hack, but it's a > > simple fix that should be sufficient to prevent the deadlock. > > I'm a bit uneasy with this fix. What if pcrypt is used in the > underlying algorithm as a fallback? Wouldn't it still dead-lock > without triggering this check? > Yep, I think you're right. Steffen, how feasible do you think would it be to make each pcrypt instance use its own padata instance, so different pcrypt instances aren't ordered with respect to each other? Or do you think there is a better solution? This really needs to be fixed; syzbot has hit it over 11000 times already. Eric
On Sun, Apr 08, 2018 at 03:55:28PM -0700, Eric Biggers wrote: > On Fri, Mar 23, 2018 at 08:21:52AM +0800, Herbert Xu wrote: > > On Sat, Mar 10, 2018 at 03:22:31PM -0800, Eric Biggers wrote: > > > From: Eric Biggers <ebiggers@google.com> > > > > > > If the pcrypt template is used multiple times in an algorithm, then a > > > deadlock occurs because all pcrypt instances share the same > > > padata_instance, which completes requests in the order submitted. That > > > is, the inner pcrypt request waits for the outer pcrypt request while > > > the outer request is already waiting for the inner. > > > > > > Fix this by making pcrypt forbid instantiation if pcrypt appears in the > > > underlying ->cra_driver_name. This is somewhat of a hack, but it's a > > > simple fix that should be sufficient to prevent the deadlock. > > > > I'm a bit uneasy with this fix. What if pcrypt is used in the > > underlying algorithm as a fallback? Wouldn't it still dead-lock > > without triggering this check? > > > > Yep, I think you're right. > > Steffen, how feasible do you think would it be to make each pcrypt instance use > its own padata instance, so different pcrypt instances aren't ordered with > respect to each other? It should be possible at least, but requires some work. I already had patches to get multiple independent pcrypt insatance to better support pcrypt for different workloads and NUMA mchines. I never got finalized with is because of the lack of NUMA hardware to test but the code is still availabe, maybe it can help: https://git.kernel.org/pub/scm/linux/kernel/git/klassert/linux-stk.git/log/?h=net-next-pcrypt > Or do you think there is a better solution? Not really. Btw. is there a valid usecase where a certain template is used twice in one algorithm instance?
On Mon, Apr 09, 2018 at 10:58:08AM +0200, Steffen Klassert wrote: > On Sun, Apr 08, 2018 at 03:55:28PM -0700, Eric Biggers wrote: > > On Fri, Mar 23, 2018 at 08:21:52AM +0800, Herbert Xu wrote: > > > On Sat, Mar 10, 2018 at 03:22:31PM -0800, Eric Biggers wrote: > > > > From: Eric Biggers <ebiggers@google.com> > > > > > > > > If the pcrypt template is used multiple times in an algorithm, then a > > > > deadlock occurs because all pcrypt instances share the same > > > > padata_instance, which completes requests in the order submitted. That > > > > is, the inner pcrypt request waits for the outer pcrypt request while > > > > the outer request is already waiting for the inner. > > > > > > > > Fix this by making pcrypt forbid instantiation if pcrypt appears in the > > > > underlying ->cra_driver_name. This is somewhat of a hack, but it's a > > > > simple fix that should be sufficient to prevent the deadlock. > > > > > > I'm a bit uneasy with this fix. What if pcrypt is used in the > > > underlying algorithm as a fallback? Wouldn't it still dead-lock > > > without triggering this check? > > > > > > > Yep, I think you're right. > > > > Steffen, how feasible do you think would it be to make each pcrypt instance use > > its own padata instance, so different pcrypt instances aren't ordered with > > respect to each other? > > It should be possible at least, but requires some work. > I already had patches to get multiple independent pcrypt > insatance to better support pcrypt for different workloads > and NUMA mchines. I never got finalized with is because > of the lack of NUMA hardware to test but the code is > still availabe, maybe it can help: > > https://git.kernel.org/pub/scm/linux/kernel/git/klassert/linux-stk.git/log/?h=net-next-pcrypt > > > Or do you think there is a better solution? > > Not really. > > Btw. is there a valid usecase where a certain template > is used twice in one algorithm instance? > None that I can think of right now. The problem is that it's hard to prevent template recursion when users can specify arbitrary algorithms using AF_ALG, and some algorithms even use fallbacks which may use templates not explicitly listed in the driver name. Remember, a kernel deadlock is a bug even if it's "not a valid use case"... In this case it can even be triggered by an unprivileged user via AF_ALG. Eric
On Mon, Apr 09, 2018 at 12:07:39PM -0700, Eric Biggers wrote: > On Mon, Apr 09, 2018 at 10:58:08AM +0200, Steffen Klassert wrote: > > On Sun, Apr 08, 2018 at 03:55:28PM -0700, Eric Biggers wrote: > > > > > > Steffen, how feasible do you think would it be to make each pcrypt instance use > > > its own padata instance, so different pcrypt instances aren't ordered with > > > respect to each other? > > > > It should be possible at least, but requires some work. > > I already had patches to get multiple independent pcrypt > > insatance to better support pcrypt for different workloads > > and NUMA mchines. I never got finalized with is because > > of the lack of NUMA hardware to test but the code is > > still availabe, maybe it can help: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/klassert/linux-stk.git/log/?h=net-next-pcrypt > > > > > Or do you think there is a better solution? > > > > Not really. > > > > Btw. is there a valid usecase where a certain template > > is used twice in one algorithm instance? > > > > None that I can think of right now. The problem is that it's hard to prevent > template recursion when users can specify arbitrary algorithms using AF_ALG, and > some algorithms even use fallbacks which may use templates not explicitly listed > in the driver name. > > Remember, a kernel deadlock is a bug even if it's "not a valid use case"... > In this case it can even be triggered by an unprivileged user via AF_ALG. Yes sure, I just wanted to know if it is worth to think about preventing template recursions. If there is a valid usecase, then we don't even need to think in this direction. While I think each pcrypt instance should have it's own padata instance on the long run, it would be good to have a not so intrusive fix that can be backported to the stable trees.
On Wed, Apr 18, 2018 at 07:35:33AM +0200, Steffen Klassert wrote: > > Yes sure, I just wanted to know if it is worth to think about > preventing template recursions. If there is a valid usecase, > then we don't even need to think in this direction. > > While I think each pcrypt instance should have it's own > padata instance on the long run, it would be good to have > a not so intrusive fix that can be backported to the stable > trees. Steffen, has there been any progress on this work? We need to fix this soon or we'll have to disable pcrypt because it is a security issue. It's not just about nested templates either. You can trigger the same issue where a pcrypt instance over an AEAD algorithm that uses a fallback which also happens to be pcrypt. Cheers,
On Wed, Feb 20, 2019 at 12:06:28PM +0800, Herbert Xu wrote: > On Wed, Apr 18, 2018 at 07:35:33AM +0200, Steffen Klassert wrote: > > > > Yes sure, I just wanted to know if it is worth to think about > > preventing template recursions. If there is a valid usecase, > > then we don't even need to think in this direction. > > > > While I think each pcrypt instance should have it's own > > padata instance on the long run, it would be good to have > > a not so intrusive fix that can be backported to the stable > > trees. > > Steffen, has there been any progress on this work? I had a look on what we need to use separate padata instances for each pcrypt instance. But that's comlicated and will create incompatibilities on the sysfs cpuset configuration options. So that's not really a thing that could be a fix. > > We need to fix this soon or we'll have to disable pcrypt because > it is a security issue. > > It's not just about nested templates either. You can trigger > the same issue where a pcrypt instance over an AEAD algorithm > that uses a fallback which also happens to be pcrypt. Would it be possible to forbid pcrypt for algorithms that needs a fallback? If I see this correct, only crypto algorithms used by HW crypto accelerators need a fallback to software crypto. HW crypto can't benefit from pcrypt anyway, so it would be no loss to disable pcrypt in that case. We could use the initial fix from Eric in combination with disableing pcrypt for algorithms that need a fallback.
On Wed, Feb 20, 2019 at 12:42:08PM +0100, Steffen Klassert wrote: > > I had a look on what we need to use separate padata > instances for each pcrypt instance. But that's comlicated > and will create incompatibilities on the sysfs cpuset > configuration options. So that's not really a thing that > could be a fix. I don't see why it'll create incompatibilities for the cpu mask configuration. You can just maintain two global configuration masks as you do now, and read them from each instance. What's the problem with that? > > We need to fix this soon or we'll have to disable pcrypt because > > it is a security issue. > > > > It's not just about nested templates either. You can trigger > > the same issue where a pcrypt instance over an AEAD algorithm > > that uses a fallback which also happens to be pcrypt. > > Would it be possible to forbid pcrypt for algorithms > that needs a fallback? This is complicated and not fool-proof. pcrypt itself might be embedded into some other algorithm directly rather than as a fallback. Basically anyone who does a crypto_alloc_aead could end up with a pcrypt instance unwittingly. If they were then used as part of another AEAD algorithm's encrypt/decrypt path then we'd have a dead-lock as we do now. Granted we may not have this situation right now but relying on it to never occur is not a good choice IMHO. Cheers,
diff --git a/crypto/pcrypt.c b/crypto/pcrypt.c index f8ec3d4ba4a80..3ec64604f6a56 100644 --- a/crypto/pcrypt.c +++ b/crypto/pcrypt.c @@ -265,6 +265,12 @@ static void pcrypt_free(struct aead_instance *inst) static int pcrypt_init_instance(struct crypto_instance *inst, struct crypto_alg *alg) { + /* Recursive pcrypt deadlocks due to the shared padata_instance */ + if (!strncmp(alg->cra_driver_name, "pcrypt(", 7) || + strstr(alg->cra_driver_name, "(pcrypt(") || + strstr(alg->cra_driver_name, ",pcrypt(")) + return -EINVAL; + if (snprintf(inst->alg.cra_driver_name, CRYPTO_MAX_ALG_NAME, "pcrypt(%s)", alg->cra_driver_name) >= CRYPTO_MAX_ALG_NAME) return -ENAMETOOLONG;