[PATCH] crypto: api - fix unexpectedly getting generic implementation

[PATCH] crypto: api - fix unexpectedly getting generic implementation

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

When CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y, the first lookup of an
algorithm that needs to be instantiated using a template will always get
the generic implementation, even when an accelerated one is available.

This happens because the extra self-tests for the accelerated
implementation allocate the generic implementation for comparison
purposes, and then crypto_alg_tested() for the generic implementation
"fulfills" the original request (i.e. sets crypto_larval::adult).

Fix this by making crypto_alg_tested() replace an already-set
crypto_larval::adult when it has lower priority and the larval hasn't
already been complete()d (by cryptomgr_probe()).

This also required adding crypto_alg_sem protection to completing the
larval in cryptomgr_probe().

Also add some comments to crypto_alg_tested() to make it easier to
understand what's going on.

Fixes: 9a8a6b3f0950 ("crypto: testmgr - fuzz hashes against their generic implementation")
Fixes: d435e10e67be ("crypto: testmgr - fuzz skciphers against their generic implementation")
Fixes: 40153b10d91c ("crypto: testmgr - fuzz AEADs against their generic implementation")
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/algapi.c   | 46 +++++++++++++++++++++++++++++++++++++++++-----
 crypto/algboss.c  |  4 ++++
 crypto/api.c      |  5 -----
 crypto/internal.h |  5 +++++
 4 files changed, 50 insertions(+), 10 deletions(-)

diff --git a/crypto/algapi.c b/crypto/algapi.c
index b052f38edba621..6c5406d342e2b7 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -251,6 +251,17 @@ static struct crypto_larval *__crypto_register_alg(struct crypto_alg *alg)
 	goto out;
 }
 
+/**
+ * crypto_alg_tested() - handle a self-test result
+ * @name: the driver name of the algorithm that was tested
+ * @err: 0 if testing passed, nonzero if testing failed
+ *
+ * If testing passed, mark the algorithm as tested and try to use it to fulfill
+ * any outstanding template instantiation requests.  Also remove any template
+ * instances that use a lower-priority implementation of the same algorithm.
+ *
+ * In any case, also wake up anyone waiting for the algorithm to be tested.
+ */
 void crypto_alg_tested(const char *name, int err)
 {
 	struct crypto_larval *test;
@@ -258,6 +269,7 @@ void crypto_alg_tested(const char *name, int err)
 	struct crypto_alg *q;
 	LIST_HEAD(list);
 
+	/* Find the algorithm's test larval */
 	down_write(&crypto_alg_sem);
 	list_for_each_entry(q, &crypto_alg_list, cra_list) {
 		if (crypto_is_moribund(q) || !crypto_is_larval(q))
@@ -290,26 +302,50 @@ void crypto_alg_tested(const char *name, int err)
 		if (crypto_is_larval(q)) {
 			struct crypto_larval *larval = (void *)q;
 
+			if (crypto_is_test_larval(larval))
+				continue;
+
 			/*
-			 * Check to see if either our generic name or
-			 * specific name can satisfy the name requested
-			 * by the larval entry q.
+			 * Fulfill the request larval 'q' (set larval->adult) if
+			 * the tested algorithm is compatible with it, i.e. if
+			 * the request is for the same generic or driver name
+			 * and for compatible flags.
+			 *
+			 * If larval->adult is already set, replace it if the
+			 * tested algorithm is higher priority and the larval
+			 * hasn't been completed()d yet.  This is needed to
+			 * avoid users always getting the generic implementation
+			 * on first use when the extra self-tests are enabled.
 			 */
+
 			if (strcmp(alg->cra_name, q->cra_name) &&
 			    strcmp(alg->cra_driver_name, q->cra_name))
 				continue;
 
-			if (larval->adult)
-				continue;
 			if ((q->cra_flags ^ alg->cra_flags) & larval->mask)
 				continue;
+
+			if (larval->adult &&
+			    larval->adult->cra_priority >= alg->cra_priority)
+				continue;
+
+			if (completion_done(&larval->completion))
+				continue;
+
 			if (!crypto_mod_get(alg))
 				continue;
 
+			if (larval->adult)
+				crypto_mod_put(larval->adult);
 			larval->adult = alg;
 			continue;
 		}
 
+		/*
+		 * Remove any template instances that use a lower-priority
+		 * implementation of the same algorithm.
+		 */
+
 		if (strcmp(alg->cra_name, q->cra_name))
 			continue;
 
diff --git a/crypto/algboss.c b/crypto/algboss.c
index a62149d6c839f5..f2b3b3ab008334 100644
--- a/crypto/algboss.c
+++ b/crypto/algboss.c
@@ -81,7 +81,11 @@ static int cryptomgr_probe(void *data)
 	crypto_tmpl_put(tmpl);
 
 out:
+	/* crypto_alg_sem is needed to synchronize with crypto_alg_tested() */
+	down_write(&crypto_alg_sem);
 	complete_all(&param->larval->completion);
+	up_write(&crypto_alg_sem);
+
 	crypto_alg_put(&param->larval->alg);
 	kfree(param);
 	module_put_and_exit(0);
diff --git a/crypto/api.c b/crypto/api.c
index ef96142ceca746..855004cb0b4d59 100644
--- a/crypto/api.c
+++ b/crypto/api.c
@@ -47,11 +47,6 @@ void crypto_mod_put(struct crypto_alg *alg)
 }
 EXPORT_SYMBOL_GPL(crypto_mod_put);
 
-static inline int crypto_is_test_larval(struct crypto_larval *larval)
-{
-	return larval->alg.cra_driver_name[0];
-}
-
 static struct crypto_alg *__crypto_alg_lookup(const char *name, u32 type,
 					      u32 mask)
 {
diff --git a/crypto/internal.h b/crypto/internal.h
index ff06a3bd1ca10c..ba744ac2ee1f09 100644
--- a/crypto/internal.h
+++ b/crypto/internal.h
@@ -110,6 +110,11 @@ static inline int crypto_is_larval(struct crypto_alg *alg)
 	return alg->cra_flags & CRYPTO_ALG_LARVAL;
 }
 
+static inline int crypto_is_test_larval(struct crypto_larval *larval)
+{
+	return larval->alg.cra_driver_name[0];
+}
+
 static inline int crypto_is_dead(struct crypto_alg *alg)
 {
 	return alg->cra_flags & CRYPTO_ALG_DEAD;
-- 
2.24.0.393.g34dc348eaf-goog

Re: [PATCH] crypto: api - fix unexpectedly getting generic implementation

[Ard Biesheuvel]

On Mon, 2 Dec 2019 at 22:13, Eric Biggers <ebiggers@kernel.org> wrote:
>
> From: Eric Biggers <ebiggers@google.com>
>
> When CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y, the first lookup of an
> algorithm that needs to be instantiated using a template will always get
> the generic implementation, even when an accelerated one is available.
>
> This happens because the extra self-tests for the accelerated
> implementation allocate the generic implementation for comparison
> purposes, and then crypto_alg_tested() for the generic implementation
> "fulfills" the original request (i.e. sets crypto_larval::adult).
>
> Fix this by making crypto_alg_tested() replace an already-set
> crypto_larval::adult when it has lower priority and the larval hasn't
> already been complete()d (by cryptomgr_probe()).
>
> This also required adding crypto_alg_sem protection to completing the
> larval in cryptomgr_probe().
>
> Also add some comments to crypto_alg_tested() to make it easier to
> understand what's going on.
>
> Fixes: 9a8a6b3f0950 ("crypto: testmgr - fuzz hashes against their generic implementation")
> Fixes: d435e10e67be ("crypto: testmgr - fuzz skciphers against their generic implementation")
> Fixes: 40153b10d91c ("crypto: testmgr - fuzz AEADs against their generic implementation")
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Thanks for fixing this.

Acked-by: Ard Biesheuvel <ardb@kernel.org>

> ---
>  crypto/algapi.c   | 46 +++++++++++++++++++++++++++++++++++++++++-----
>  crypto/algboss.c  |  4 ++++
>  crypto/api.c      |  5 -----
>  crypto/internal.h |  5 +++++
>  4 files changed, 50 insertions(+), 10 deletions(-)
>
> diff --git a/crypto/algapi.c b/crypto/algapi.c
> index b052f38edba621..6c5406d342e2b7 100644
> --- a/crypto/algapi.c
> +++ b/crypto/algapi.c
> @@ -251,6 +251,17 @@ static struct crypto_larval *__crypto_register_alg(struct crypto_alg *alg)
>         goto out;
>  }
>
> +/**
> + * crypto_alg_tested() - handle a self-test result
> + * @name: the driver name of the algorithm that was tested
> + * @err: 0 if testing passed, nonzero if testing failed
> + *
> + * If testing passed, mark the algorithm as tested and try to use it to fulfill
> + * any outstanding template instantiation requests.  Also remove any template
> + * instances that use a lower-priority implementation of the same algorithm.
> + *
> + * In any case, also wake up anyone waiting for the algorithm to be tested.
> + */
>  void crypto_alg_tested(const char *name, int err)
>  {
>         struct crypto_larval *test;
> @@ -258,6 +269,7 @@ void crypto_alg_tested(const char *name, int err)
>         struct crypto_alg *q;
>         LIST_HEAD(list);
>
> +       /* Find the algorithm's test larval */
>         down_write(&crypto_alg_sem);
>         list_for_each_entry(q, &crypto_alg_list, cra_list) {
>                 if (crypto_is_moribund(q) || !crypto_is_larval(q))
> @@ -290,26 +302,50 @@ void crypto_alg_tested(const char *name, int err)
>                 if (crypto_is_larval(q)) {
>                         struct crypto_larval *larval = (void *)q;
>
> +                       if (crypto_is_test_larval(larval))
> +                               continue;
> +
>                         /*
> -                        * Check to see if either our generic name or
> -                        * specific name can satisfy the name requested
> -                        * by the larval entry q.
> +                        * Fulfill the request larval 'q' (set larval->adult) if
> +                        * the tested algorithm is compatible with it, i.e. if
> +                        * the request is for the same generic or driver name
> +                        * and for compatible flags.
> +                        *
> +                        * If larval->adult is already set, replace it if the
> +                        * tested algorithm is higher priority and the larval
> +                        * hasn't been completed()d yet.  This is needed to
> +                        * avoid users always getting the generic implementation
> +                        * on first use when the extra self-tests are enabled.
>                          */
> +
>                         if (strcmp(alg->cra_name, q->cra_name) &&
>                             strcmp(alg->cra_driver_name, q->cra_name))
>                                 continue;
>
> -                       if (larval->adult)
> -                               continue;
>                         if ((q->cra_flags ^ alg->cra_flags) & larval->mask)
>                                 continue;
> +
> +                       if (larval->adult &&
> +                           larval->adult->cra_priority >= alg->cra_priority)
> +                               continue;
> +
> +                       if (completion_done(&larval->completion))
> +                               continue;
> +
>                         if (!crypto_mod_get(alg))
>                                 continue;
>
> +                       if (larval->adult)
> +                               crypto_mod_put(larval->adult);
>                         larval->adult = alg;
>                         continue;
>                 }
>
> +               /*
> +                * Remove any template instances that use a lower-priority
> +                * implementation of the same algorithm.
> +                */
> +
>                 if (strcmp(alg->cra_name, q->cra_name))
>                         continue;
>
> diff --git a/crypto/algboss.c b/crypto/algboss.c
> index a62149d6c839f5..f2b3b3ab008334 100644
> --- a/crypto/algboss.c
> +++ b/crypto/algboss.c
> @@ -81,7 +81,11 @@ static int cryptomgr_probe(void *data)
>         crypto_tmpl_put(tmpl);
>
>  out:
> +       /* crypto_alg_sem is needed to synchronize with crypto_alg_tested() */
> +       down_write(&crypto_alg_sem);
>         complete_all(&param->larval->completion);
> +       up_write(&crypto_alg_sem);
> +
>         crypto_alg_put(&param->larval->alg);
>         kfree(param);
>         module_put_and_exit(0);
> diff --git a/crypto/api.c b/crypto/api.c
> index ef96142ceca746..855004cb0b4d59 100644
> --- a/crypto/api.c
> +++ b/crypto/api.c
> @@ -47,11 +47,6 @@ void crypto_mod_put(struct crypto_alg *alg)
>  }
>  EXPORT_SYMBOL_GPL(crypto_mod_put);
>
> -static inline int crypto_is_test_larval(struct crypto_larval *larval)
> -{
> -       return larval->alg.cra_driver_name[0];
> -}
> -
>  static struct crypto_alg *__crypto_alg_lookup(const char *name, u32 type,
>                                               u32 mask)
>  {
> diff --git a/crypto/internal.h b/crypto/internal.h
> index ff06a3bd1ca10c..ba744ac2ee1f09 100644
> --- a/crypto/internal.h
> +++ b/crypto/internal.h
> @@ -110,6 +110,11 @@ static inline int crypto_is_larval(struct crypto_alg *alg)
>         return alg->cra_flags & CRYPTO_ALG_LARVAL;
>  }
>
> +static inline int crypto_is_test_larval(struct crypto_larval *larval)
> +{
> +       return larval->alg.cra_driver_name[0];
> +}
> +
>  static inline int crypto_is_dead(struct crypto_alg *alg)
>  {
>         return alg->cra_flags & CRYPTO_ALG_DEAD;
> --
> 2.24.0.393.g34dc348eaf-goog
>

[v2 PATCH] crypto: api - fix unexpectedly getting generic implementation

[Herbert Xu]

Eric Biggers <ebiggers@kernel.org> wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> When CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y, the first lookup of an
> algorithm that needs to be instantiated using a template will always get
> the generic implementation, even when an accelerated one is available.
> 
> This happens because the extra self-tests for the accelerated
> implementation allocate the generic implementation for comparison
> purposes, and then crypto_alg_tested() for the generic implementation
> "fulfills" the original request (i.e. sets crypto_larval::adult).
> 
> Fix this by making crypto_alg_tested() replace an already-set
> crypto_larval::adult when it has lower priority and the larval hasn't
> already been complete()d (by cryptomgr_probe()).
> 
> This also required adding crypto_alg_sem protection to completing the
> larval in cryptomgr_probe().
> 
> Also add some comments to crypto_alg_tested() to make it easier to
> understand what's going on.
> 
> Fixes: 9a8a6b3f0950 ("crypto: testmgr - fuzz hashes against their generic implementation")
> Fixes: d435e10e67be ("crypto: testmgr - fuzz skciphers against their generic implementation")
> Fixes: 40153b10d91c ("crypto: testmgr - fuzz AEADs against their generic implementation")
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
> crypto/algapi.c   | 46 +++++++++++++++++++++++++++++++++++++++++-----
> crypto/algboss.c  |  4 ++++
> crypto/api.c      |  5 -----
> crypto/internal.h |  5 +++++
> 4 files changed, 50 insertions(+), 10 deletions(-)

Thanks for the patch! I had forgotten about this problem :)

> @@ -290,26 +302,50 @@ void crypto_alg_tested(const char *name, int err)
>                if (crypto_is_larval(q)) {
>                        struct crypto_larval *larval = (void *)q;
> 
> +                       if (crypto_is_test_larval(larval))
> +                               continue;
> +
>                        /*
> -                        * Check to see if either our generic name or
> -                        * specific name can satisfy the name requested
> -                        * by the larval entry q.
> +                        * Fulfill the request larval 'q' (set larval->adult) if
> +                        * the tested algorithm is compatible with it, i.e. if
> +                        * the request is for the same generic or driver name
> +                        * and for compatible flags.
> +                        *
> +                        * If larval->adult is already set, replace it if the
> +                        * tested algorithm is higher priority and the larval
> +                        * hasn't been completed()d yet.  This is needed to
> +                        * avoid users always getting the generic implementation
> +                        * on first use when the extra self-tests are enabled.
>                         */
> +
>                        if (strcmp(alg->cra_name, q->cra_name) &&
>                            strcmp(alg->cra_driver_name, q->cra_name))
>                                continue;
> 
> -                       if (larval->adult)
> -                               continue;
>                        if ((q->cra_flags ^ alg->cra_flags) & larval->mask)
>                                continue;
> +
> +                       if (larval->adult &&
> +                           larval->adult->cra_priority >= alg->cra_priority)
> +                               continue;
> +
> +                       if (completion_done(&larval->completion))
> +                               continue;
> +
>                        if (!crypto_mod_get(alg))
>                                continue;
> 
> +                       if (larval->adult)
> +                               crypto_mod_put(larval->adult);
>                        larval->adult = alg;
>                        continue;
>                }

I think this is a tad over-complicated.  All we really need to do
is avoid changing larval->adult if we are not the best larval.
Something like this (totally untested!):

---8<---
When CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y, the first lookup of an
algorithm that needs to be instantiated using a template will always get
the generic implementation, even when an accelerated one is available.

This happens because the extra self-tests for the accelerated
implementation allocate the generic implementation for comparison
purposes, and then crypto_alg_tested() for the generic implementation
"fulfills" the original request (i.e. sets crypto_larval::adult).

This patch fixes this by only fulfilling the original request if
we are currently the best outstanding larval as judged by the
priority.
 
Fixes: 9a8a6b3f0950 ("crypto: testmgr - fuzz hashes against their generic implementation")
Fixes: d435e10e67be ("crypto: testmgr - fuzz skciphers against their generic implementation")
Fixes: 40153b10d91c ("crypto: testmgr - fuzz AEADs against their generic implementation")
Reported-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/crypto/algapi.c b/crypto/algapi.c
index b052f38edba6..3e65653735f4 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -280,6 +280,18 @@ void crypto_alg_tested(const char *name, int err)
 
 	alg->cra_flags |= CRYPTO_ALG_TESTED;
 
+	/* Only satisfy larval waiters if we are the best. */
+	list_for_each_entry(q, &crypto_alg_list, cra_list) {
+		if (!crypto_is_larval(q))
+			continue;
+
+		if (strcmp(alg->cra_name, q->cra_name))
+			continue;
+
+		if (q->cra_priority > alg->cra_priority)
+			goto complete;
+	}
+
 	list_for_each_entry(q, &crypto_alg_list, cra_list) {
 		if (q == alg)
 			continue;
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [v2 PATCH] crypto: api - fix unexpectedly getting generic implementation

[Eric Biggers]

On Wed, Dec 04, 2019 at 05:19:10PM +0800, Herbert Xu wrote:
> I think this is a tad over-complicated.  All we really need to do
> is avoid changing larval->adult if we are not the best larval.
> Something like this (totally untested!):
> 
> ---8<---
> When CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y, the first lookup of an
> algorithm that needs to be instantiated using a template will always get
> the generic implementation, even when an accelerated one is available.
> 
> This happens because the extra self-tests for the accelerated
> implementation allocate the generic implementation for comparison
> purposes, and then crypto_alg_tested() for the generic implementation
> "fulfills" the original request (i.e. sets crypto_larval::adult).
> 
> This patch fixes this by only fulfilling the original request if
> we are currently the best outstanding larval as judged by the
> priority.
>  
> Fixes: 9a8a6b3f0950 ("crypto: testmgr - fuzz hashes against their generic implementation")
> Fixes: d435e10e67be ("crypto: testmgr - fuzz skciphers against their generic implementation")
> Fixes: 40153b10d91c ("crypto: testmgr - fuzz AEADs against their generic implementation")
> Reported-by: Eric Biggers <ebiggers@google.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> diff --git a/crypto/algapi.c b/crypto/algapi.c
> index b052f38edba6..3e65653735f4 100644
> --- a/crypto/algapi.c
> +++ b/crypto/algapi.c
> @@ -280,6 +280,18 @@ void crypto_alg_tested(const char *name, int err)
>  
>  	alg->cra_flags |= CRYPTO_ALG_TESTED;
>  
> +	/* Only satisfy larval waiters if we are the best. */
> +	list_for_each_entry(q, &crypto_alg_list, cra_list) {
> +		if (!crypto_is_larval(q))
> +			continue;
> +
> +		if (strcmp(alg->cra_name, q->cra_name))
> +			continue;
> +
> +		if (q->cra_priority > alg->cra_priority)
> +			goto complete;
> +	}
> +

I was going to do something like this originally (but also checking that 'q' is
not "moribund", is a test larval, and has compatible cra_flags).  But I don't
think it will work because a higher priority implementation could be registered
while a lower priority one is being instantiated and tested.  Based on this
logic, when the lower priority implementation finishes being tested,
larval->adult wouldn't be set since a higher priority implementation is still
being tested.  But then cryptomgr_probe() will complete() the larval anyway and
for the user crypto_alloc_foo() will fail with ENOENT.

With my patch the user would get the lower priority implementation in this case,
since it would be the best one ready at the time cryptomgr_probe() finished.

- Eric

[v3 PATCH] crypto: api - fix unexpectedly getting generic implementation

[Herbert Xu]

On Wed, Dec 04, 2019 at 09:22:44AM -0800, Eric Biggers wrote:
>
> I was going to do something like this originally (but also checking that 'q' is
> not "moribund", is a test larval, and has compatible cra_flags).  But I don't

You are right.  I'll add these tests to the patch.

> think it will work because a higher priority implementation could be registered
> while a lower priority one is being instantiated and tested.  Based on this
> logic, when the lower priority implementation finishes being tested,
> larval->adult wouldn't be set since a higher priority implementation is still
> being tested.  But then cryptomgr_probe() will complete() the larval anyway and
> for the user crypto_alloc_foo() will fail with ENOENT.

I think this is a different problem, one which we probably should
address but it already exists regardless of what we do here.  For
example, assuming that tmpl(X) does not currently exist, and I
request tmpl(X-generic) then tmpl(X-generic) along with X-generic
will be created in the system.  If someone then comes along and
asks for tmpl(X) then we'll simply give them tmpl(X-generic) even
if there exists an accelerated version of X.

The problem you describe is simply a racy version of the above
scenario where the requests for tmpl(X) and tmpl(X-generic) occur
about the same time.

We should certainly fix this, as otherwise anyone could force
the whole system to use the generic (or some other non-optimal
variant).  One possible solution is to mark any instance created
by a request like tmpl(X-generic) as non-optimal so that a subsequent
request for tmpl(X) has to go through the whole process again before
being fulfilled.

The hardest part of this is actually ensuring that X-generic does
not fulfil a subsequent request for X.  This is because X-generic
is probably going to be created by a module-load and the crypto
API is not driving the registration of X-generic.  We could
probably use some form of larvals that get created at registration
time to resolve this.

Cheers,

---8<---
When CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y, the first lookup of an
algorithm that needs to be instantiated using a template will always get
the generic implementation, even when an accelerated one is available.

This happens because the extra self-tests for the accelerated
implementation allocate the generic implementation for comparison
purposes, and then crypto_alg_tested() for the generic implementation
"fulfills" the original request (i.e. sets crypto_larval::adult).

This patch fixes this by only fulfilling the original request if
we are currently the best outstanding larval as judged by the
priority.
 
Fixes: 9a8a6b3f0950 ("crypto: testmgr - fuzz hashes against...")
Fixes: d435e10e67be ("crypto: testmgr - fuzz skciphers against...")
Fixes: 40153b10d91c ("crypto: testmgr - fuzz AEADs against...")
Reported-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/crypto/algapi.c b/crypto/algapi.c
index b052f38edba6..390bdc874b61 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -280,6 +280,24 @@ void crypto_alg_tested(const char *name, int err)
 
 	alg->cra_flags |= CRYPTO_ALG_TESTED;
 
+	/* Only satisfy larval waiters if we are the best. */
+	list_for_each_entry(q, &crypto_alg_list, cra_list) {
+		struct crypto_larval *larval;
+
+		if (crypto_is_moribund(q) || !crypto_is_larval(q))
+			continue;
+
+		if (strcmp(alg->cra_name, q->cra_name))
+			continue;
+
+		larval = (void *)q;
+		if ((q->cra_flags ^ alg->cra_flags) & larval->mask)
+			continue;
+
+		if (q->cra_priority > alg->cra_priority)
+			goto complete;
+	}
+
 	list_for_each_entry(q, &crypto_alg_list, cra_list) {
 		if (q == alg)
 			continue;
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [v3 PATCH] crypto: api - fix unexpectedly getting generic implementation

[Eric Biggers]

On Thu, Dec 05, 2019 at 09:58:11AM +0800, Herbert Xu wrote:
> On Wed, Dec 04, 2019 at 09:22:44AM -0800, Eric Biggers wrote:
> >
> > I was going to do something like this originally (but also checking that 'q' is
> > not "moribund", is a test larval, and has compatible cra_flags).  But I don't
> 
> You are right.  I'll add these tests to the patch.
> 
> > think it will work because a higher priority implementation could be registered
> > while a lower priority one is being instantiated and tested.  Based on this
> > logic, when the lower priority implementation finishes being tested,
> > larval->adult wouldn't be set since a higher priority implementation is still
> > being tested.  But then cryptomgr_probe() will complete() the larval anyway and
> > for the user crypto_alloc_foo() will fail with ENOENT.
> 
> I think this is a different problem, one which we probably should
> address but it already exists regardless of what we do here.  For
> example, assuming that tmpl(X) does not currently exist, and I
> request tmpl(X-generic) then tmpl(X-generic) along with X-generic
> will be created in the system.  If someone then comes along and
> asks for tmpl(X) then we'll simply give them tmpl(X-generic) even
> if there exists an accelerated version of X.
> 
> The problem you describe is simply a racy version of the above
> scenario where the requests for tmpl(X) and tmpl(X-generic) occur
> about the same time.
> 

No, the problem I'm talking about is different and is new to your patch.  If
tmpl(X-accelerated) is registered while someone is doing crypto_alg_mod_lookup()
that triggered instantiation of tmpl(X-generic), then crypto_alg_mod_lookup()
could fail with ENOENT, instead of returning tmpl(X-generic) as it does
currently.  This is because the proposed new logic will not fulfill the request
larval if a better implementation of tmpl(X) is still being tested.  But there's
no guarantee that tmpl(X) will finish being tested by the time cryptomgr_probe()
thinks it is done and complete()s the request larval with 'adult' still NULL.

(I think; I haven't actually tested this, this analysis is just based on my
reading of the code...)

- Eric

[v4 PATCH] crypto: api - fix unexpectedly getting generic implementation

[Herbert Xu]

On Wed, Dec 04, 2019 at 07:43:01PM -0800, Eric Biggers wrote:
>
> No, the problem I'm talking about is different and is new to your patch.  If
> tmpl(X-accelerated) is registered while someone is doing crypto_alg_mod_lookup()
> that triggered instantiation of tmpl(X-generic), then crypto_alg_mod_lookup()
> could fail with ENOENT, instead of returning tmpl(X-generic) as it does
> currently.  This is because the proposed new logic will not fulfill the request
> larval if a better implementation of tmpl(X) is still being tested.  But there's
> no guarantee that tmpl(X) will finish being tested by the time cryptomgr_probe()
> thinks it is done and complete()s the request larval with 'adult' still NULL.
> 
> (I think; I haven't actually tested this, this analysis is just based on my
> reading of the code...)

Right.  This is indeed a regression.  How about this patch then?
We can simply punt and retry the lookup if we encounter a better
larval.

---8<---
When CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y, the first lookup of an
algorithm that needs to be instantiated using a template will always get
the generic implementation, even when an accelerated one is available.

This happens because the extra self-tests for the accelerated
implementation allocate the generic implementation for comparison
purposes, and then crypto_alg_tested() for the generic implementation
"fulfills" the original request (i.e. sets crypto_larval::adult).

This patch fixes this by only fulfilling the original request if
we are currently the best outstanding larval as judged by the
priority.  If we're not the best then we will ask all waiters on
that larval request to retry the lookup.

Note that this patch introduces a behaviour change when the module
providing the new algorithm is unregistered during the process.
Previously we would have failed with ENOENT, after the patch we
will instead redo the lookup.
 
Fixes: 9a8a6b3f0950 ("crypto: testmgr - fuzz hashes against...")
Fixes: d435e10e67be ("crypto: testmgr - fuzz skciphers against...")
Fixes: 40153b10d91c ("crypto: testmgr - fuzz AEADs against...")
Reported-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/crypto/algapi.c b/crypto/algapi.c
index b052f38edba6..c7527ac614af 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -257,6 +257,7 @@ void crypto_alg_tested(const char *name, int err)
 	struct crypto_alg *alg;
 	struct crypto_alg *q;
 	LIST_HEAD(list);
+	bool best;
 
 	down_write(&crypto_alg_sem);
 	list_for_each_entry(q, &crypto_alg_list, cra_list) {
@@ -280,6 +281,21 @@ void crypto_alg_tested(const char *name, int err)
 
 	alg->cra_flags |= CRYPTO_ALG_TESTED;
 
+	/* Only satisfy larval waiters if we are the best. */
+	best = true;
+	list_for_each_entry(q, &crypto_alg_list, cra_list) {
+		if (crypto_is_moribund(q) || !crypto_is_larval(q))
+			continue;
+
+		if (strcmp(alg->cra_name, q->cra_name))
+			continue;
+
+		if (q->cra_priority > alg->cra_priority) {
+			best = false;
+			break;
+		}
+	}
+
 	list_for_each_entry(q, &crypto_alg_list, cra_list) {
 		if (q == alg)
 			continue;
@@ -289,6 +305,7 @@ void crypto_alg_tested(const char *name, int err)
 
 		if (crypto_is_larval(q)) {
 			struct crypto_larval *larval = (void *)q;
+			struct crypto_alg *r;
 
 			/*
 			 * Check to see if either our generic name or
@@ -303,8 +320,10 @@ void crypto_alg_tested(const char *name, int err)
 				continue;
 			if ((q->cra_flags ^ alg->cra_flags) & larval->mask)
 				continue;
-			if (!crypto_mod_get(alg))
-				continue;
+
+			r = ERR_PTR(-EAGAIN);
+			if (best && crypto_mod_get(alg))
+				r = alg;
 
 			larval->adult = alg;
 			continue;
diff --git a/crypto/api.c b/crypto/api.c
index 55bca28df92d..b5ad4cc1198a 100644
--- a/crypto/api.c
+++ b/crypto/api.c
@@ -97,7 +97,7 @@ static void crypto_larval_destroy(struct crypto_alg *alg)
 	struct crypto_larval *larval = (void *)alg;
 
 	BUG_ON(!crypto_is_larval(alg));
-	if (larval->adult)
+	if (!IS_ERR_OR_NULL(larval->adult))
 		crypto_mod_put(larval->adult);
 	kfree(larval);
 }
@@ -178,6 +178,8 @@ static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg)
 		alg = ERR_PTR(-ETIMEDOUT);
 	else if (!alg)
 		alg = ERR_PTR(-ENOENT);
+	else if (IS_ERR(alg))
+		;
 	else if (crypto_is_test_larval(larval) &&
 		 !(alg->cra_flags & CRYPTO_ALG_TESTED))
 		alg = ERR_PTR(-EAGAIN);
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [v4 PATCH] crypto: api - fix unexpectedly getting generic implementation

[Eric Biggers]

On Thu, Dec 05, 2019 at 12:55:45PM +0800, Herbert Xu wrote:
> On Wed, Dec 04, 2019 at 07:43:01PM -0800, Eric Biggers wrote:
> >
> > No, the problem I'm talking about is different and is new to your patch.  If
> > tmpl(X-accelerated) is registered while someone is doing crypto_alg_mod_lookup()
> > that triggered instantiation of tmpl(X-generic), then crypto_alg_mod_lookup()
> > could fail with ENOENT, instead of returning tmpl(X-generic) as it does
> > currently.  This is because the proposed new logic will not fulfill the request
> > larval if a better implementation of tmpl(X) is still being tested.  But there's
> > no guarantee that tmpl(X) will finish being tested by the time cryptomgr_probe()
> > thinks it is done and complete()s the request larval with 'adult' still NULL.
> > 
> > (I think; I haven't actually tested this, this analysis is just based on my
> > reading of the code...)
> 
> Right.  This is indeed a regression.  How about this patch then?
> We can simply punt and retry the lookup if we encounter a better
> larval.
> 
> ---8<---
> When CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y, the first lookup of an
> algorithm that needs to be instantiated using a template will always get
> the generic implementation, even when an accelerated one is available.
> 
> This happens because the extra self-tests for the accelerated
> implementation allocate the generic implementation for comparison
> purposes, and then crypto_alg_tested() for the generic implementation
> "fulfills" the original request (i.e. sets crypto_larval::adult).
> 
> This patch fixes this by only fulfilling the original request if
> we are currently the best outstanding larval as judged by the
> priority.  If we're not the best then we will ask all waiters on
> that larval request to retry the lookup.
> 
> Note that this patch introduces a behaviour change when the module
> providing the new algorithm is unregistered during the process.
> Previously we would have failed with ENOENT, after the patch we
> will instead redo the lookup.
>  
> Fixes: 9a8a6b3f0950 ("crypto: testmgr - fuzz hashes against...")
> Fixes: d435e10e67be ("crypto: testmgr - fuzz skciphers against...")
> Fixes: 40153b10d91c ("crypto: testmgr - fuzz AEADs against...")
> Reported-by: Eric Biggers <ebiggers@google.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> diff --git a/crypto/algapi.c b/crypto/algapi.c
> index b052f38edba6..c7527ac614af 100644
> --- a/crypto/algapi.c
> +++ b/crypto/algapi.c
> @@ -257,6 +257,7 @@ void crypto_alg_tested(const char *name, int err)
>  	struct crypto_alg *alg;
>  	struct crypto_alg *q;
>  	LIST_HEAD(list);
> +	bool best;
>  
>  	down_write(&crypto_alg_sem);
>  	list_for_each_entry(q, &crypto_alg_list, cra_list) {
> @@ -280,6 +281,21 @@ void crypto_alg_tested(const char *name, int err)
>  
>  	alg->cra_flags |= CRYPTO_ALG_TESTED;
>  
> +	/* Only satisfy larval waiters if we are the best. */
> +	best = true;
> +	list_for_each_entry(q, &crypto_alg_list, cra_list) {
> +		if (crypto_is_moribund(q) || !crypto_is_larval(q))
> +			continue;
> +
> +		if (strcmp(alg->cra_name, q->cra_name))
> +			continue;
> +
> +		if (q->cra_priority > alg->cra_priority) {
> +			best = false;
> +			break;
> +		}
> +	}
> +
>  	list_for_each_entry(q, &crypto_alg_list, cra_list) {
>  		if (q == alg)
>  			continue;
> @@ -289,6 +305,7 @@ void crypto_alg_tested(const char *name, int err)
>  
>  		if (crypto_is_larval(q)) {
>  			struct crypto_larval *larval = (void *)q;
> +			struct crypto_alg *r;
>  
>  			/*
>  			 * Check to see if either our generic name or
> @@ -303,8 +320,10 @@ void crypto_alg_tested(const char *name, int err)
>  				continue;
>  			if ((q->cra_flags ^ alg->cra_flags) & larval->mask)
>  				continue;
> -			if (!crypto_mod_get(alg))
> -				continue;
> +
> +			r = ERR_PTR(-EAGAIN);
> +			if (best && crypto_mod_get(alg))
> +				r = alg;
>  
>  			larval->adult = alg;
>  			continue;
> diff --git a/crypto/api.c b/crypto/api.c
> index 55bca28df92d..b5ad4cc1198a 100644
> --- a/crypto/api.c
> +++ b/crypto/api.c
> @@ -97,7 +97,7 @@ static void crypto_larval_destroy(struct crypto_alg *alg)
>  	struct crypto_larval *larval = (void *)alg;
>  
>  	BUG_ON(!crypto_is_larval(alg));
> -	if (larval->adult)
> +	if (!IS_ERR_OR_NULL(larval->adult))
>  		crypto_mod_put(larval->adult);
>  	kfree(larval);
>  }
> @@ -178,6 +178,8 @@ static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg)
>  		alg = ERR_PTR(-ETIMEDOUT);
>  	else if (!alg)
>  		alg = ERR_PTR(-ENOENT);
> +	else if (IS_ERR(alg))
> +		;
>  	else if (crypto_is_test_larval(larval) &&
>  		 !(alg->cra_flags & CRYPTO_ALG_TESTED))
>  		alg = ERR_PTR(-EAGAIN);

Sorry, I didn't notice you had already sent another patch for this.  I think
this patch is okay, except that it's broken because it doesn't actually do
anything with the 'r' variable in crypto_alg_tested().  I suggest just removing
that variable and doing:

		if (best && crypto_mod_get(alg))
			larval->adult = alg;
		else
			larval->adult = ERR_PTR(-EAGAIN);

Also, it would be nice to also add a function comment for crypto_alg_tested(),
like I had in my original patch.  It's hard to understand this code.

- Eric

[v5 PATCH] crypto: api - fix unexpectedly getting generic implementation

[Herbert Xu]

On Tue, Dec 10, 2019 at 06:26:13PM -0800, Eric Biggers wrote:
>
> Sorry, I didn't notice you had already sent another patch for this.  I think
> this patch is okay, except that it's broken because it doesn't actually do
> anything with the 'r' variable in crypto_alg_tested().  I suggest just removing

Oops.

> that variable and doing:
> 
> 		if (best && crypto_mod_get(alg))
> 			larval->adult = alg;
> 		else
> 			larval->adult = ERR_PTR(-EAGAIN);

OK I have made this change.

> Also, it would be nice to also add a function comment for crypto_alg_tested(),
> like I had in my original patch.  It's hard to understand this code.

Your original comments no longer apply but if you wish to make
another patch to add more comments that would certainly be welcome.

Thanks,

---8<---
When CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y, the first lookup of an
algorithm that needs to be instantiated using a template will always get
the generic implementation, even when an accelerated one is available.

This happens because the extra self-tests for the accelerated
implementation allocate the generic implementation for comparison
purposes, and then crypto_alg_tested() for the generic implementation
"fulfills" the original request (i.e. sets crypto_larval::adult).

This patch fixes this by only fulfilling the original request if
we are currently the best outstanding larval as judged by the
priority.  If we're not the best then we will ask all waiters on
that larval request to retry the lookup.

Note that this patch introduces a behaviour change when the module
providing the new algorithm is unregistered during the process.
Previously we would have failed with ENOENT, after the patch we
will instead redo the lookup.
 
Fixes: 9a8a6b3f0950 ("crypto: testmgr - fuzz hashes against...")
Fixes: d435e10e67be ("crypto: testmgr - fuzz skciphers against...")
Fixes: 40153b10d91c ("crypto: testmgr - fuzz AEADs against...")
Reported-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/crypto/algapi.c b/crypto/algapi.c
index cd643e294664..9589b3f0041b 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -284,6 +284,7 @@ void crypto_alg_tested(const char *name, int err)
 	struct crypto_alg *alg;
 	struct crypto_alg *q;
 	LIST_HEAD(list);
+	bool best;
 
 	down_write(&crypto_alg_sem);
 	list_for_each_entry(q, &crypto_alg_list, cra_list) {
@@ -307,6 +308,21 @@ void crypto_alg_tested(const char *name, int err)
 
 	alg->cra_flags |= CRYPTO_ALG_TESTED;
 
+	/* Only satisfy larval waiters if we are the best. */
+	best = true;
+	list_for_each_entry(q, &crypto_alg_list, cra_list) {
+		if (crypto_is_moribund(q) || !crypto_is_larval(q))
+			continue;
+
+		if (strcmp(alg->cra_name, q->cra_name))
+			continue;
+
+		if (q->cra_priority > alg->cra_priority) {
+			best = false;
+			break;
+		}
+	}
+
 	list_for_each_entry(q, &crypto_alg_list, cra_list) {
 		if (q == alg)
 			continue;
@@ -330,10 +346,12 @@ void crypto_alg_tested(const char *name, int err)
 				continue;
 			if ((q->cra_flags ^ alg->cra_flags) & larval->mask)
 				continue;
-			if (!crypto_mod_get(alg))
-				continue;
 
-			larval->adult = alg;
+			if (best && crypto_mod_get(alg))
+				larval->adult = alg;
+			else
+				larval->adult = ERR_PTR(-EAGAIN);
+
 			continue;
 		}
 
diff --git a/crypto/api.c b/crypto/api.c
index 0ef9f2a37d3d..c00af5ad1b16 100644
--- a/crypto/api.c
+++ b/crypto/api.c
@@ -97,7 +97,7 @@ static void crypto_larval_destroy(struct crypto_alg *alg)
 	struct crypto_larval *larval = (void *)alg;
 
 	BUG_ON(!crypto_is_larval(alg));
-	if (larval->adult)
+	if (!IS_ERR_OR_NULL(larval->adult))
 		crypto_mod_put(larval->adult);
 	kfree(larval);
 }
@@ -178,6 +178,8 @@ static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg)
 		alg = ERR_PTR(-ETIMEDOUT);
 	else if (!alg)
 		alg = ERR_PTR(-ENOENT);
+	else if (IS_ERR(alg))
+		;
 	else if (crypto_is_test_larval(larval) &&
 		 !(alg->cra_flags & CRYPTO_ALG_TESTED))
 		alg = ERR_PTR(-EAGAIN);

-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [v5 PATCH] crypto: api - fix unexpectedly getting generic implementation

[Eric Biggers]

On Wed, Dec 11, 2019 at 10:50:11AM +0800, Herbert Xu wrote:
> On Tue, Dec 10, 2019 at 06:26:13PM -0800, Eric Biggers wrote:
> >
> > Sorry, I didn't notice you had already sent another patch for this.  I think
> > this patch is okay, except that it's broken because it doesn't actually do
> > anything with the 'r' variable in crypto_alg_tested().  I suggest just removing
> 
> Oops.
> 
> > that variable and doing:
> > 
> > 		if (best && crypto_mod_get(alg))
> > 			larval->adult = alg;
> > 		else
> > 			larval->adult = ERR_PTR(-EAGAIN);
> 
> OK I have made this change.
> 
> > Also, it would be nice to also add a function comment for crypto_alg_tested(),
> > like I had in my original patch.  It's hard to understand this code.
> 
> Your original comments no longer apply but if you wish to make
> another patch to add more comments that would certainly be welcome.
> 
> Thanks,
> 
> ---8<---
> When CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y, the first lookup of an
> algorithm that needs to be instantiated using a template will always get
> the generic implementation, even when an accelerated one is available.
> 
> This happens because the extra self-tests for the accelerated
> implementation allocate the generic implementation for comparison
> purposes, and then crypto_alg_tested() for the generic implementation
> "fulfills" the original request (i.e. sets crypto_larval::adult).
> 
> This patch fixes this by only fulfilling the original request if
> we are currently the best outstanding larval as judged by the
> priority.  If we're not the best then we will ask all waiters on
> that larval request to retry the lookup.
> 
> Note that this patch introduces a behaviour change when the module
> providing the new algorithm is unregistered during the process.
> Previously we would have failed with ENOENT, after the patch we
> will instead redo the lookup.
>  
> Fixes: 9a8a6b3f0950 ("crypto: testmgr - fuzz hashes against...")
> Fixes: d435e10e67be ("crypto: testmgr - fuzz skciphers against...")
> Fixes: 40153b10d91c ("crypto: testmgr - fuzz AEADs against...")
> Reported-by: Eric Biggers <ebiggers@google.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Looks good, thanks.

Reviewed-by: Eric Biggers <ebiggers@google.com>

- Eric

Re: [v3 PATCH] crypto: api - fix unexpectedly getting generic implementation

[Eric Biggers]

On Thu, Dec 05, 2019 at 09:58:11AM +0800, Herbert Xu wrote:
> +	/* Only satisfy larval waiters if we are the best. */
> +	list_for_each_entry(q, &crypto_alg_list, cra_list) {
> +		struct crypto_larval *larval;
> +
> +		if (crypto_is_moribund(q) || !crypto_is_larval(q))
> +			continue;
> +
> +		if (strcmp(alg->cra_name, q->cra_name))
> +			continue;
> +
> +		larval = (void *)q;
> +		if ((q->cra_flags ^ alg->cra_flags) & larval->mask)
> +			continue;
> +
> +		if (q->cra_priority > alg->cra_priority)
> +			goto complete;
> +	}
> +

This logic doesn't make sense to me either.  It's supposed to be looking for a
"test larval", not a "request larval", right?  But it seems that larval->mask is
always 0 for "test larvals", so the flags check will never do anything...

Also, different "request larvals" can use different flags and masks.  So I don't
think it's possible to know whether 'q' can fulfill every outstanding request
that 'alg' can without actually going through and looking at the requests.  So
that's another case where users can start incorrectly getting ENOENT.

If we don't try to skip setting larval->adult, but rather override the existing
value (as my patch does), the incorrect ENOENTs are prevented.

- Eric

Re: [v3 PATCH] crypto: api - fix unexpectedly getting generic implementation

[Herbert Xu]

On Wed, Dec 04, 2019 at 08:04:42PM -0800, Eric Biggers wrote:
>
> This logic doesn't make sense to me either.  It's supposed to be looking for a
> "test larval", not a "request larval", right?  But it seems that larval->mask is
> always 0 for "test larvals", so the flags check will never do anything...

No we only care about request larvals.  Test larvals always have
a non-null adult set so they are irrelevant.

> Also, different "request larvals" can use different flags and masks.  So I don't
> think it's possible to know whether 'q' can fulfill every outstanding request
> that 'alg' can without actually going through and looking at the requests.  So
> that's another case where users can start incorrectly getting ENOENT.

That's a good point.  Let me think about this a bit more.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [v3 PATCH] crypto: api - fix unexpectedly getting generic implementation

[Herbert Xu]

On Thu, Dec 05, 2019 at 12:23:57PM +0800, Herbert Xu wrote:
> On Wed, Dec 04, 2019 at 08:04:42PM -0800, Eric Biggers wrote:
> >
> > This logic doesn't make sense to me either.  It's supposed to be looking for a
> > "test larval", not a "request larval", right?  But it seems that larval->mask is
> > always 0 for "test larvals", so the flags check will never do anything...
> 
> No we only care about request larvals.  Test larvals always have
> a non-null adult set so they are irrelevant.

Scratch that.  You're right we only care about test larvals and
if we wanted to check the flags we need to drill down to the
actual tested algorithm using larval->adult.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt