[PATCH] crypto: qat - fix out-of-bounds read

[PATCH] crypto: qat - fix out-of-bounds read

[Giovanni Cabiddu]

When preparing an AER-CTR request, the driver copies the key provided by
the user into a data structure that is accessible by the firmware.
If the target device is QAT GEN4, the key size is rounded up by 16 since
a rounded up size is expected by the device.
If the key size is rounded up before the copy, the size used for copying
the key might be bigger than the size of the region containing the key,
causing an out-of-bounds read.

Fix by doing the copy first and then update the keylen.

This is to fix the following warning reported by KASAN:

	[  138.150574] BUG: KASAN: global-out-of-bounds in qat_alg_skcipher_init_com.isra.0+0x197/0x250 [intel_qat]
	[  138.150641] Read of size 32 at addr ffffffff88c402c0 by task cryptomgr_test/2340

	[  138.150651] CPU: 15 PID: 2340 Comm: cryptomgr_test Not tainted 6.2.0-rc1+ #45
	[  138.150659] Hardware name: Intel Corporation ArcherCity/ArcherCity, BIOS EGSDCRB1.86B.0087.D13.2208261706 08/26/2022
	[  138.150663] Call Trace:
	[  138.150668]  <TASK>
	[  138.150922]  kasan_check_range+0x13a/0x1c0
	[  138.150931]  memcpy+0x1f/0x60
	[  138.150940]  qat_alg_skcipher_init_com.isra.0+0x197/0x250 [intel_qat]
	[  138.151006]  qat_alg_skcipher_init_sessions+0xc1/0x240 [intel_qat]
	[  138.151073]  crypto_skcipher_setkey+0x82/0x160
	[  138.151085]  ? prepare_keybuf+0xa2/0xd0
	[  138.151095]  test_skcipher_vec_cfg+0x2b8/0x800

Fixes: 67916c951689 ("crypto: qat - add AES-CTR support for QAT GEN4 devices")
Cc: <stable@vger.kernel.org>
Reported-by: Vladis Dronov <vdronov@redhat.com>
Signed-off-by: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
Reviewed-by: Fiona Trahe <fiona.trahe@intel.com>
---
 drivers/crypto/qat/qat_common/qat_algs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/qat/qat_common/qat_algs.c b/drivers/crypto/qat/qat_common/qat_algs.c
index b4b9f0aa59b9..b61ada559158 100644
--- a/drivers/crypto/qat/qat_common/qat_algs.c
+++ b/drivers/crypto/qat/qat_common/qat_algs.c
@@ -435,8 +435,8 @@ static void qat_alg_skcipher_init_com(struct qat_alg_skcipher_ctx *ctx,
 	} else if (aes_v2_capable && mode == ICP_QAT_HW_CIPHER_CTR_MODE) {
 		ICP_QAT_FW_LA_SLICE_TYPE_SET(header->serv_specif_flags,
 					     ICP_QAT_FW_LA_USE_UCS_SLICE_TYPE);
-		keylen = round_up(keylen, 16);
 		memcpy(cd->ucs_aes.key, key, keylen);
+		keylen = round_up(keylen, 16);
 	} else {
 		memcpy(cd->aes.key, key, keylen);
 	}
-- 
2.39.1

Re: [PATCH] crypto: qat - fix out-of-bounds read

[Vladis Dronov]

 Hi, Giovanni, all,

On Wed, Feb 1, 2023 at 4:59 PM Giovanni Cabiddu
<giovanni.cabiddu@intel.com> wrote:
>
> When preparing an AER-CTR request, the driver copies the key provided by
> the user into a data structure that is accessible by the firmware.
> If the target device is QAT GEN4, the key size is rounded up by 16 since
> a rounded up size is expected by the device.
> If the key size is rounded up before the copy, the size used for copying
> the key might be bigger than the size of the region containing the key,
> causing an out-of-bounds read.
>
> Fix by doing the copy first and then update the keylen.
>
> This is to fix the following warning reported by KASAN:
>
>         [  138.150574] BUG: KASAN: global-out-of-bounds in qat_alg_skcipher_init_com.isra.0+0x197/0x250 [intel_qat]
>         [  138.150641] Read of size 32 at addr ffffffff88c402c0 by task cryptomgr_test/2340
>
>         [  138.150651] CPU: 15 PID: 2340 Comm: cryptomgr_test Not tainted 6.2.0-rc1+ #45
>         [  138.150659] Hardware name: Intel Corporation ArcherCity/ArcherCity, BIOS EGSDCRB1.86B.0087.D13.2208261706 08/26/2022
>         [  138.150663] Call Trace:
>         [  138.150668]  <TASK>
>         [  138.150922]  kasan_check_range+0x13a/0x1c0
>         [  138.150931]  memcpy+0x1f/0x60
>         [  138.150940]  qat_alg_skcipher_init_com.isra.0+0x197/0x250 [intel_qat]
>         [  138.151006]  qat_alg_skcipher_init_sessions+0xc1/0x240 [intel_qat]
>         [  138.151073]  crypto_skcipher_setkey+0x82/0x160
>         [  138.151085]  ? prepare_keybuf+0xa2/0xd0
>         [  138.151095]  test_skcipher_vec_cfg+0x2b8/0x800
>
> Fixes: 67916c951689 ("crypto: qat - add AES-CTR support for QAT GEN4 devices")
> Cc: <stable@vger.kernel.org>
> Reported-by: Vladis Dronov <vdronov@redhat.com>
> Signed-off-by: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
> Reviewed-by: Fiona Trahe <fiona.trahe@intel.com>
> ---
>  drivers/crypto/qat/qat_common/qat_algs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/crypto/qat/qat_common/qat_algs.c b/drivers/crypto/qat/qat_common/qat_algs.c
> index b4b9f0aa59b9..b61ada559158 100644
> --- a/drivers/crypto/qat/qat_common/qat_algs.c
> +++ b/drivers/crypto/qat/qat_common/qat_algs.c
> @@ -435,8 +435,8 @@ static void qat_alg_skcipher_init_com(struct qat_alg_skcipher_ctx *ctx,
>         } else if (aes_v2_capable && mode == ICP_QAT_HW_CIPHER_CTR_MODE) {
>                 ICP_QAT_FW_LA_SLICE_TYPE_SET(header->serv_specif_flags,
>                                              ICP_QAT_FW_LA_USE_UCS_SLICE_TYPE);
> -               keylen = round_up(keylen, 16);
>                 memcpy(cd->ucs_aes.key, key, keylen);
> +               keylen = round_up(keylen, 16);
>         } else {
>                 memcpy(cd->aes.key, key, keylen);
>         }
> --
> 2.39.1
>

Thanks, the fix seems to be working. It was tested on "Intel(R) Xeon(R)
Platinum 8468H / Sapphire Rapids 4 skt (SPR) XCC-SP, Qual E-3
stepping" machine with 8086:4940 (rev 40) QAT device:

Without the fix:

# dmesg | grep KASAN
[  142.612847] BUG: KASAN: global-out-of-bounds in
qat_alg_skcipher_init_com.isra.0+0x2fe/0x440 [intel_qat]

With the fix:

# dmesg | grep KASAN
<no output>

So,

Reviewed-by: Vladis Dronov <vdronov@redhat.com>
Tested-by: Vladis Dronov <vdronov@redhat.com>

Best regards,
Vladis Dronov | Red Hat, Inc. | The Core Kernel | Senior Software Engineer

Re: [PATCH] crypto: qat - fix out-of-bounds read

[Herbert Xu]

On Wed, Feb 01, 2023 at 03:59:44PM +0000, Giovanni Cabiddu wrote:
.
> @@ -435,8 +435,8 @@ static void qat_alg_skcipher_init_com(struct qat_alg_skcipher_ctx *ctx,
>  	} else if (aes_v2_capable && mode == ICP_QAT_HW_CIPHER_CTR_MODE) {
>  		ICP_QAT_FW_LA_SLICE_TYPE_SET(header->serv_specif_flags,
>  					     ICP_QAT_FW_LA_USE_UCS_SLICE_TYPE);
> -		keylen = round_up(keylen, 16);
>  		memcpy(cd->ucs_aes.key, key, keylen);
> +		keylen = round_up(keylen, 16);

Now cd->ucs_aes.key contains potentially unitialised data, should
we zero them?

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: [PATCH] crypto: qat - fix out-of-bounds read

[Giovanni Cabiddu]

On Fri, Feb 03, 2023 at 09:04:59AM +0800, Herbert Xu wrote:
> On Wed, Feb 01, 2023 at 03:59:44PM +0000, Giovanni Cabiddu wrote:
> .
> > @@ -435,8 +435,8 @@ static void qat_alg_skcipher_init_com(struct qat_alg_skcipher_ctx *ctx,
> >  	} else if (aes_v2_capable && mode == ICP_QAT_HW_CIPHER_CTR_MODE) {
> >  		ICP_QAT_FW_LA_SLICE_TYPE_SET(header->serv_specif_flags,
> >  					     ICP_QAT_FW_LA_USE_UCS_SLICE_TYPE);
> > -		keylen = round_up(keylen, 16);
> >  		memcpy(cd->ucs_aes.key, key, keylen);
> > +		keylen = round_up(keylen, 16);
> 
> Now cd->ucs_aes.key contains potentially unitialised data, should
> we zero them?
The content descriptor structure (cd) is already initialized to zero
before the function qat_alg_skcipher_init_com() is called.
This is done in
  (1) qat_alg_skcipher_newkey() implicitly by dma_alloc_coherent(),
      the first time setkey() is called for a tfm or
  (2) qat_alg_skcipher_rekey() explicitly, for subsequent calls to
      sekey().

Regards,

-- 
Giovanni

Re: [PATCH] crypto: qat - fix out-of-bounds read

[Herbert Xu]

On Fri, Feb 03, 2023 at 06:36:22AM +0000, Giovanni Cabiddu wrote:
>
> The content descriptor structure (cd) is already initialized to zero
> before the function qat_alg_skcipher_init_com() is called.
> This is done in
>   (1) qat_alg_skcipher_newkey() implicitly by dma_alloc_coherent(),
>       the first time setkey() is called for a tfm or

Sorry but what zeroes the memory in this case? The only zeroing
I can find in newkey is when you free the memory.

>   (2) qat_alg_skcipher_rekey() explicitly, for subsequent calls to
>       sekey().

This looks fine.

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: [PATCH] crypto: qat - fix out-of-bounds read

[Giovanni Cabiddu]

On Fri, Feb 03, 2023 at 04:48:12PM +0800, Herbert Xu wrote:
> On Fri, Feb 03, 2023 at 06:36:22AM +0000, Giovanni Cabiddu wrote:
> >
> > The content descriptor structure (cd) is already initialized to zero
> > before the function qat_alg_skcipher_init_com() is called.
> > This is done in
> >   (1) qat_alg_skcipher_newkey() implicitly by dma_alloc_coherent(),
> >       the first time setkey() is called for a tfm or
> 
> Sorry but what zeroes the memory in this case? The only zeroing
> I can find in newkey is when you free the memory.
dma_alloc_coherent() returns zero'd memory.
When implemented originally that code used dma_zalloc_coherent(). This
was phased out in Kernel 5.0 by 750afb08ca71.

	commit 750afb08ca71310fcf0c4e2cb1565c63b8235b60
	Author: Luis Chamberlain <mcgrof@kernel.org>
	Date:   Fri Jan 4 09:23:09 2019 +0100

	cross-tree: phase out dma_zalloc_coherent()

	We already need to zero out memory for dma_alloc_coherent(), as such
	using dma_zalloc_coherent() is superflous. Phase it out.
	...

Regards,

-- 
Giovanni

Re: [PATCH] crypto: qat - fix out-of-bounds read

[Herbert Xu]

On Fri, Feb 03, 2023 at 09:10:55AM +0000, Giovanni Cabiddu wrote:
>
> dma_alloc_coherent() returns zero'd memory.
> When implemented originally that code used dma_zalloc_coherent(). This
> was phased out in Kernel 5.0 by 750afb08ca71.

OK thanks for digging this up.  It wasn't obvious.
-- 
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: [PATCH] crypto: qat - fix out-of-bounds read

[Herbert Xu]

On Wed, Feb 01, 2023 at 03:59:44PM +0000, Giovanni Cabiddu wrote:
> When preparing an AER-CTR request, the driver copies the key provided by
> the user into a data structure that is accessible by the firmware.
> If the target device is QAT GEN4, the key size is rounded up by 16 since
> a rounded up size is expected by the device.
> If the key size is rounded up before the copy, the size used for copying
> the key might be bigger than the size of the region containing the key,
> causing an out-of-bounds read.
> 
> Fix by doing the copy first and then update the keylen.
> 
> This is to fix the following warning reported by KASAN:
> 
> 	[  138.150574] BUG: KASAN: global-out-of-bounds in qat_alg_skcipher_init_com.isra.0+0x197/0x250 [intel_qat]
> 	[  138.150641] Read of size 32 at addr ffffffff88c402c0 by task cryptomgr_test/2340
> 
> 	[  138.150651] CPU: 15 PID: 2340 Comm: cryptomgr_test Not tainted 6.2.0-rc1+ #45
> 	[  138.150659] Hardware name: Intel Corporation ArcherCity/ArcherCity, BIOS EGSDCRB1.86B.0087.D13.2208261706 08/26/2022
> 	[  138.150663] Call Trace:
> 	[  138.150668]  <TASK>
> 	[  138.150922]  kasan_check_range+0x13a/0x1c0
> 	[  138.150931]  memcpy+0x1f/0x60
> 	[  138.150940]  qat_alg_skcipher_init_com.isra.0+0x197/0x250 [intel_qat]
> 	[  138.151006]  qat_alg_skcipher_init_sessions+0xc1/0x240 [intel_qat]
> 	[  138.151073]  crypto_skcipher_setkey+0x82/0x160
> 	[  138.151085]  ? prepare_keybuf+0xa2/0xd0
> 	[  138.151095]  test_skcipher_vec_cfg+0x2b8/0x800
> 
> Fixes: 67916c951689 ("crypto: qat - add AES-CTR support for QAT GEN4 devices")
> Cc: <stable@vger.kernel.org>
> Reported-by: Vladis Dronov <vdronov@redhat.com>
> Signed-off-by: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
> Reviewed-by: Fiona Trahe <fiona.trahe@intel.com>
> ---
>  drivers/crypto/qat/qat_common/qat_algs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Patch applied.  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