[PATCH bpf 1/2] bpf: Fix out-of-bounds dynptr write in bpf_crypto_crypt

[PATCH bpf 1/2] bpf: Fix out-of-bounds dynptr write in bpf_crypto_crypt

[Daniel Borkmann]

Stanislav reported that in bpf_crypto_crypt() the destination dynptr's
size is not validated to be at least as large as the source dynptr's
size before calling into the crypto backend with 'len = src_len'. This
can result in an OOB write when the destination is smaller than the
source.

Concretely, in mentioned function, psrc and pdst are both linear
buffers fetched from each dynptr:

  psrc = __bpf_dynptr_data(src, src_len);
  [...]
  pdst = __bpf_dynptr_data_rw(dst, dst_len);
  [...]
  err = decrypt ?
        ctx->type->decrypt(ctx->tfm, psrc, pdst, src_len, piv) :
        ctx->type->encrypt(ctx->tfm, psrc, pdst, src_len, piv);

The crypto backend expects pdst to be large enough with a src_len length
that can be written. Add an additional src_len > dst_len check and bail
out if it's the case. Note that these kfuncs are accessible under root
privileges only.

Fixes: 3e1c6f35409f ("bpf: make common crypto API for TC/XDP programs")
Reported-by: Stanislav Fort <disclosure@aisle.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Cc: Vadim Fedorenko <vadim.fedorenko@linux.dev>
---
 kernel/bpf/crypto.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/bpf/crypto.c b/kernel/bpf/crypto.c
index 94854cd9c4cc..83c4d9943084 100644
--- a/kernel/bpf/crypto.c
+++ b/kernel/bpf/crypto.c
@@ -278,7 +278,7 @@ static int bpf_crypto_crypt(const struct bpf_crypto_ctx *ctx,
 	siv_len = siv ? __bpf_dynptr_size(siv) : 0;
 	src_len = __bpf_dynptr_size(src);
 	dst_len = __bpf_dynptr_size(dst);
-	if (!src_len || !dst_len)
+	if (!src_len || !dst_len || src_len > dst_len)
 		return -EINVAL;
 
 	if (siv_len != ctx->siv_len)
-- 
2.43.0

[PATCH bpf 2/2] selftests/bpf: Extend crypto_sanity selftest with invalid dst buffer

[Daniel Borkmann]

Small cleanup and test extension to probe the bpf_crypto_{encrypt,decrypt}()
kfunc when a bad dst buffer is passed in to assert that an error is returned.

Also, encrypt_sanity() and skb_crypto_setup() were explicit to set the global
status variable to zero before any test, so do the same for decrypt_sanity().
Do not explicitly zero the on-stack err before bpf_crypto_ctx_create() given
the kfunc is expected to do it internally for the success case.

Before kernel fix:

  # ./vmtest.sh -- ./test_progs -t crypto
  [...]
  [    1.531200] bpf_testmod: loading out-of-tree module taints kernel.
  [    1.533388] bpf_testmod: module verification failed: signature and/or required key missing - tainting kernel
  #87/1    crypto_basic/crypto_release:OK
  #87/2    crypto_basic/crypto_acquire:OK
  #87      crypto_basic:OK
  test_crypto_sanity:PASS:skel open 0 nsec
  test_crypto_sanity:PASS:ip netns add crypto_sanity_ns 0 nsec
  test_crypto_sanity:PASS:ip -net crypto_sanity_ns -6 addr add face::1/128 dev lo nodad 0 nsec
  test_crypto_sanity:PASS:ip -net crypto_sanity_ns link set dev lo up 0 nsec
  test_crypto_sanity:PASS:open_netns 0 nsec
  test_crypto_sanity:PASS:AF_ALG init fail 0 nsec
  test_crypto_sanity:PASS:if_nametoindex lo 0 nsec
  test_crypto_sanity:PASS:skb_crypto_setup fd 0 nsec
  test_crypto_sanity:PASS:skb_crypto_setup 0 nsec
  test_crypto_sanity:PASS:skb_crypto_setup retval 0 nsec
  test_crypto_sanity:PASS:skb_crypto_setup status 0 nsec
  test_crypto_sanity:PASS:create qdisc hook 0 nsec
  test_crypto_sanity:PASS:make_sockaddr 0 nsec
  test_crypto_sanity:PASS:attach encrypt filter 0 nsec
  test_crypto_sanity:PASS:encrypt socket 0 nsec
  test_crypto_sanity:PASS:encrypt send 0 nsec
  test_crypto_sanity:FAIL:encrypt status unexpected error: -5 (errno 95)
  #88      crypto_sanity:FAIL
  Summary: 1/2 PASSED, 0 SKIPPED, 1 FAILED

After kernel fix:

  # ./vmtest.sh -- ./test_progs -t crypto
  [...]
  [    1.540963] bpf_testmod: loading out-of-tree module taints kernel.
  [    1.542404] bpf_testmod: module verification failed: signature and/or required key missing - tainting kernel
  #87/1    crypto_basic/crypto_release:OK
  #87/2    crypto_basic/crypto_acquire:OK
  #87      crypto_basic:OK
  #88      crypto_sanity:OK
  Summary: 2/2 PASSED, 0 SKIPPED, 0 FAILED

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Cc: Vadim Fedorenko <vadim.fedorenko@linux.dev>
---
 .../selftests/bpf/progs/crypto_sanity.c       | 46 +++++++++++++------
 1 file changed, 32 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/crypto_sanity.c b/tools/testing/selftests/bpf/progs/crypto_sanity.c
index 645be6cddf36..dfd8a258f14a 100644
--- a/tools/testing/selftests/bpf/progs/crypto_sanity.c
+++ b/tools/testing/selftests/bpf/progs/crypto_sanity.c
@@ -14,7 +14,7 @@ unsigned char key[256] = {};
 u16 udp_test_port = 7777;
 u32 authsize, key_len;
 char algo[128] = {};
-char dst[16] = {};
+char dst[16] = {}, dst_bad[8] = {};
 int status;
 
 static int skb_dynptr_validate(struct __sk_buff *skb, struct bpf_dynptr *psrc)
@@ -59,10 +59,9 @@ int skb_crypto_setup(void *ctx)
 		.authsize = authsize,
 	};
 	struct bpf_crypto_ctx *cctx;
-	int err = 0;
+	int err;
 
 	status = 0;
-
 	if (key_len > 256) {
 		status = -EINVAL;
 		return 0;
@@ -70,8 +69,8 @@ int skb_crypto_setup(void *ctx)
 
 	__builtin_memcpy(&params.algo, algo, sizeof(algo));
 	__builtin_memcpy(&params.key, key, sizeof(key));
-	cctx = bpf_crypto_ctx_create(&params, sizeof(params), &err);
 
+	cctx = bpf_crypto_ctx_create(&params, sizeof(params), &err);
 	if (!cctx) {
 		status = err;
 		return 0;
@@ -80,7 +79,6 @@ int skb_crypto_setup(void *ctx)
 	err = crypto_ctx_insert(cctx);
 	if (err && err != -EEXIST)
 		status = err;
-
 	return 0;
 }
 
@@ -92,6 +90,7 @@ int decrypt_sanity(struct __sk_buff *skb)
 	struct bpf_dynptr psrc, pdst;
 	int err;
 
+	status = 0;
 	err = skb_dynptr_validate(skb, &psrc);
 	if (err < 0) {
 		status = err;
@@ -110,13 +109,23 @@ int decrypt_sanity(struct __sk_buff *skb)
 		return TC_ACT_SHOT;
 	}
 
-	/* dst is a global variable to make testing part easier to check. In real
-	 * production code, a percpu map should be used to store the result.
+	/* Check also bad case where the dst buffer is smaller than the
+	 * skb's linear section.
+	 */
+	bpf_dynptr_from_mem(dst_bad, sizeof(dst_bad), 0, &pdst);
+	status = bpf_crypto_decrypt(ctx, &psrc, &pdst, NULL);
+	if (!status)
+		status = -EIO;
+	if (status != -EINVAL)
+		goto err;
+
+	/* dst is a global variable to make testing part easier to check.
+	 * In real production code, a percpu map should be used to store
+	 * the result.
 	 */
 	bpf_dynptr_from_mem(dst, sizeof(dst), 0, &pdst);
-
 	status = bpf_crypto_decrypt(ctx, &psrc, &pdst, NULL);
-
+err:
 	return TC_ACT_SHOT;
 }
 
@@ -129,7 +138,6 @@ int encrypt_sanity(struct __sk_buff *skb)
 	int err;
 
 	status = 0;
-
 	err = skb_dynptr_validate(skb, &psrc);
 	if (err < 0) {
 		status = err;
@@ -148,13 +156,23 @@ int encrypt_sanity(struct __sk_buff *skb)
 		return TC_ACT_SHOT;
 	}
 
-	/* dst is a global variable to make testing part easier to check. In real
-	 * production code, a percpu map should be used to store the result.
+	/* Check also bad case where the dst buffer is smaller than the
+	 * skb's linear section.
+	 */
+	bpf_dynptr_from_mem(dst_bad, sizeof(dst_bad), 0, &pdst);
+	status = bpf_crypto_encrypt(ctx, &psrc, &pdst, NULL);
+	if (!status)
+		status = -EIO;
+	if (status != -EINVAL)
+		goto err;
+
+	/* dst is a global variable to make testing part easier to check.
+	 * In real production code, a percpu map should be used to store
+	 * the result.
 	 */
 	bpf_dynptr_from_mem(dst, sizeof(dst), 0, &pdst);
-
 	status = bpf_crypto_encrypt(ctx, &psrc, &pdst, NULL);
-
+err:
 	return TC_ACT_SHOT;
 }
 
-- 
2.43.0

Re: [PATCH bpf 1/2] bpf: Fix out-of-bounds dynptr write in bpf_crypto_crypt

[Vadim Fedorenko]

On 29/08/2025 15:36, Daniel Borkmann wrote:
> Stanislav reported that in bpf_crypto_crypt() the destination dynptr's
> size is not validated to be at least as large as the source dynptr's
> size before calling into the crypto backend with 'len = src_len'. This
> can result in an OOB write when the destination is smaller than the
> source.
> 
> Concretely, in mentioned function, psrc and pdst are both linear
> buffers fetched from each dynptr:
> 
>    psrc = __bpf_dynptr_data(src, src_len);
>    [...]
>    pdst = __bpf_dynptr_data_rw(dst, dst_len);
>    [...]
>    err = decrypt ?
>          ctx->type->decrypt(ctx->tfm, psrc, pdst, src_len, piv) :
>          ctx->type->encrypt(ctx->tfm, psrc, pdst, src_len, piv);
> 
> The crypto backend expects pdst to be large enough with a src_len length
> that can be written. Add an additional src_len > dst_len check and bail
> out if it's the case. Note that these kfuncs are accessible under root
> privileges only.
> 
> Fixes: 3e1c6f35409f ("bpf: make common crypto API for TC/XDP programs")
> Reported-by: Stanislav Fort <disclosure@aisle.com>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Vadim Fedorenko <vadim.fedorenko@linux.dev>
> ---
>   kernel/bpf/crypto.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/bpf/crypto.c b/kernel/bpf/crypto.c
> index 94854cd9c4cc..83c4d9943084 100644
> --- a/kernel/bpf/crypto.c
> +++ b/kernel/bpf/crypto.c
> @@ -278,7 +278,7 @@ static int bpf_crypto_crypt(const struct bpf_crypto_ctx *ctx,
>   	siv_len = siv ? __bpf_dynptr_size(siv) : 0;
>   	src_len = __bpf_dynptr_size(src);
>   	dst_len = __bpf_dynptr_size(dst);
> -	if (!src_len || !dst_len)
> +	if (!src_len || !dst_len || src_len > dst_len)

I think it would make sense to have less restrictive check. I mean it's
ok to have dst_len equal to src_len.

>   		return -EINVAL;
>   
>   	if (siv_len != ctx->siv_len)

Re: [PATCH bpf 1/2] bpf: Fix out-of-bounds dynptr write in bpf_crypto_crypt

[Daniel Borkmann]

On 8/29/25 4:50 PM, Vadim Fedorenko wrote:
> On 29/08/2025 15:36, Daniel Borkmann wrote:
>> Stanislav reported that in bpf_crypto_crypt() the destination dynptr's
>> size is not validated to be at least as large as the source dynptr's
>> size before calling into the crypto backend with 'len = src_len'. This
>> can result in an OOB write when the destination is smaller than the
>> source.
>>
>> Concretely, in mentioned function, psrc and pdst are both linear
>> buffers fetched from each dynptr:
>>
>>    psrc = __bpf_dynptr_data(src, src_len);
>>    [...]
>>    pdst = __bpf_dynptr_data_rw(dst, dst_len);
>>    [...]
>>    err = decrypt ?
>>          ctx->type->decrypt(ctx->tfm, psrc, pdst, src_len, piv) :
>>          ctx->type->encrypt(ctx->tfm, psrc, pdst, src_len, piv);
>>
>> The crypto backend expects pdst to be large enough with a src_len length
>> that can be written. Add an additional src_len > dst_len check and bail
>> out if it's the case. Note that these kfuncs are accessible under root
>> privileges only.
>>
>> Fixes: 3e1c6f35409f ("bpf: make common crypto API for TC/XDP programs")
>> Reported-by: Stanislav Fort <disclosure@aisle.com>
>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>> Cc: Vadim Fedorenko <vadim.fedorenko@linux.dev>
>> ---
>>   kernel/bpf/crypto.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/bpf/crypto.c b/kernel/bpf/crypto.c
>> index 94854cd9c4cc..83c4d9943084 100644
>> --- a/kernel/bpf/crypto.c
>> +++ b/kernel/bpf/crypto.c
>> @@ -278,7 +278,7 @@ static int bpf_crypto_crypt(const struct bpf_crypto_ctx *ctx,
>>       siv_len = siv ? __bpf_dynptr_size(siv) : 0;
>>       src_len = __bpf_dynptr_size(src);
>>       dst_len = __bpf_dynptr_size(dst);
>> -    if (!src_len || !dst_len)
>> +    if (!src_len || !dst_len || src_len > dst_len)
> 
> I think it would make sense to have less restrictive check. I mean it's
> ok to have dst_len equal to src_len.

That scenario is/remains allowed and is also what the 'good' case is testing in
the BPF selftests (src_len 16 vs dst_len 16).

Thanks,
Daniel

Re: [PATCH bpf 1/2] bpf: Fix out-of-bounds dynptr write in bpf_crypto_crypt

[Vadim Fedorenko]

On 29/08/2025 16:30, Daniel Borkmann wrote:
> On 8/29/25 4:50 PM, Vadim Fedorenko wrote:
>> On 29/08/2025 15:36, Daniel Borkmann wrote:
>>> Stanislav reported that in bpf_crypto_crypt() the destination dynptr's
>>> size is not validated to be at least as large as the source dynptr's
>>> size before calling into the crypto backend with 'len = src_len'. This
>>> can result in an OOB write when the destination is smaller than the
>>> source.
>>>
>>> Concretely, in mentioned function, psrc and pdst are both linear
>>> buffers fetched from each dynptr:
>>>
>>>    psrc = __bpf_dynptr_data(src, src_len);
>>>    [...]
>>>    pdst = __bpf_dynptr_data_rw(dst, dst_len);
>>>    [...]
>>>    err = decrypt ?
>>>          ctx->type->decrypt(ctx->tfm, psrc, pdst, src_len, piv) :
>>>          ctx->type->encrypt(ctx->tfm, psrc, pdst, src_len, piv);
>>>
>>> The crypto backend expects pdst to be large enough with a src_len length
>>> that can be written. Add an additional src_len > dst_len check and bail
>>> out if it's the case. Note that these kfuncs are accessible under root
>>> privileges only.
>>>
>>> Fixes: 3e1c6f35409f ("bpf: make common crypto API for TC/XDP programs")
>>> Reported-by: Stanislav Fort <disclosure@aisle.com>
>>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>>> Cc: Vadim Fedorenko <vadim.fedorenko@linux.dev>
>>> ---
>>>   kernel/bpf/crypto.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/kernel/bpf/crypto.c b/kernel/bpf/crypto.c
>>> index 94854cd9c4cc..83c4d9943084 100644
>>> --- a/kernel/bpf/crypto.c
>>> +++ b/kernel/bpf/crypto.c
>>> @@ -278,7 +278,7 @@ static int bpf_crypto_crypt(const struct 
>>> bpf_crypto_ctx *ctx,
>>>       siv_len = siv ? __bpf_dynptr_size(siv) : 0;
>>>       src_len = __bpf_dynptr_size(src);
>>>       dst_len = __bpf_dynptr_size(dst);
>>> -    if (!src_len || !dst_len)
>>> +    if (!src_len || !dst_len || src_len > dst_len)
>>
>> I think it would make sense to have less restrictive check. I mean it's
>> ok to have dst_len equal to src_len.
> 
> That scenario is/remains allowed and is also what the 'good' case is 
> testing in
> the BPF selftests (src_len 16 vs dst_len 16).

Ah, sorry, misread the code. Yeah, it makes sense.

Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

Re: [PATCH bpf 1/2] bpf: Fix out-of-bounds dynptr write in bpf_crypto_crypt

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Fri, 29 Aug 2025 16:36:56 +0200 you wrote:
> Stanislav reported that in bpf_crypto_crypt() the destination dynptr's
> size is not validated to be at least as large as the source dynptr's
> size before calling into the crypto backend with 'len = src_len'. This
> can result in an OOB write when the destination is smaller than the
> source.
> 
> Concretely, in mentioned function, psrc and pdst are both linear
> buffers fetched from each dynptr:
> 
> [...]

Here is the summary with links:
  - [bpf,1/2] bpf: Fix out-of-bounds dynptr write in bpf_crypto_crypt
    https://git.kernel.org/bpf/bpf/c/51ae4ca30f11
  - [bpf,2/2] selftests/bpf: Extend crypto_sanity selftest with invalid dst buffer
    https://git.kernel.org/bpf/bpf/c/5aa00f0e9589

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html