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

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

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>