Re: [PATCH] crypto: Fix missing initialisation affecting gcm-aes-s390

[Ard Biesheuvel]

On Wed, 26 Jul 2023 at 23:54, David Howells <dhowells@xxxxxxxxxx> wrote:
>
>
> Fix af_alg_alloc_areq() to initialise areq->first_rsgl.sgl.sgt.sgl to point
> to the scatterlist array in areq->first_rsgl.sgl.sgl.
>
> Without this, the gcm-aes-s390 driver will oops when it tries to do
> gcm_walk_start() on req->dst because req->dst is set to the value of
> areq->first_rsgl.sgl.sgl by _aead_recvmsg() calling
> aead_request_set_crypt().
>
> The problem comes if an empty ciphertext is passed: the loop in
> af_alg_get_rsgl() just passes straight out and doesn't set areq->first_rsgl
> up.
>
> This isn't a problem on x86_64 using gcmaes_crypt_by_sg() because, as far
> as I can tell, that ignores req->dst and only uses req->src[*].
>
> [*] Is this a bug in aesni-intel_glue.c?
>

It uses req->src directly only for processing the additional
authenticated data (AAD) which contributes to the MAC but not to the
ciphertext. Conceptually, there is no dst only src for this part, and
only the IPsec specific encapsulations of GCM and CCM etc do a plain
memcpy of src to dst (if src and dst do not refer to the same
scatterlist already). Otherwise, the AAD is not considered to be part
of the output.

The actual encryption logic does use both src and dst, but under the
hood (inside the skcipher walk helpers)



> The s390x oops looks something like:
>
>  Unable to handle kernel pointer dereference in virtual kernel address space
>  Failing address: 0000000a00000000 TEID: 0000000a00000803
>  Fault in home space mode while using kernel ASCE.
>  AS:00000000a43a0007 R3:0000000000000024
>  Oops: 003b ilc:2 [#1] SMP
>  ...
>  Call Trace:
>   [<000003ff7fc3d47e>] gcm_walk_start+0x16/0x28 [aes_s390]
>   [<00000000a2a342f2>] crypto_aead_decrypt+0x9a/0xb8
>   [<00000000a2a60888>] aead_recvmsg+0x478/0x698
>   [<00000000a2e519a0>] sock_recvmsg+0x70/0xb0
>   [<00000000a2e51a56>] sock_read_iter+0x76/0xa0
>   [<00000000a273e066>] vfs_read+0x26e/0x2a8
>   [<00000000a273e8c4>] ksys_read+0xbc/0x100
>   [<00000000a311d808>] __do_syscall+0x1d0/0x1f8
>   [<00000000a312ff30>] system_call+0x70/0x98
>  Last Breaking-Event-Address:
>   [<000003ff7fc3e6b4>] gcm_aes_crypt+0x104/0xa68 [aes_s390]
>
> Fixes: c1abe6f570af ("crypto: af_alg: Use extract_iter_to_sg() to create scatterlists")
> Reported-by: Ondrej Mosnáček <omosnacek@xxxxxxxxx>
> Link: https://lore.kernel.org/r/CAAUqJDuRkHE8fPgZJGaKjUjd3QfGwzfumuJBmStPqBhubxyk_A@xxxxxxxxxxxxxx/
> Signed-off-by: David Howells <dhowells@xxxxxxxxxx>
> cc: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
> cc: Sven Schnelle <svens@xxxxxxxxxxxxx>
> cc: Harald Freudenberger <freude@xxxxxxxxxxxxxxxxxx>
> cc: "David S. Miller" <davem@xxxxxxxxxxxxx>
> cc: Paolo Abeni <pabeni@xxxxxxxxxx>
> cc: linux-crypto@xxxxxxxxxxxxxxx
> cc: linux-s390@xxxxxxxxxxxxxxx
> cc: regressions@xxxxxxxxxxxxxxx
> ---
>  crypto/af_alg.c |    1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/crypto/af_alg.c b/crypto/af_alg.c
> index 06b15b9f661c..9ee8575d3b1a 100644
> --- a/crypto/af_alg.c
> +++ b/crypto/af_alg.c
> @@ -1192,6 +1192,7 @@ struct af_alg_async_req *af_alg_alloc_areq(struct sock *sk,
>
>         areq->areqlen = areqlen;
>         areq->sk = sk;
> +       areq->first_rsgl.sgl.sgt.sgl = areq->first_rsgl.sgl.sgl;
>         areq->last_rsgl = NULL;
>         INIT_LIST_HEAD(&areq->rsgl_list);
>         areq->tsgl = NULL;
>