Re: [PATCH bpf-next v8 1/3] bpf: make common crypto API for TC/XDP programs

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

On 26/01/2024 10:30, Vadim Fedorenko wrote:
> On 25/01/2024 22:34, Martin KaFai Lau wrote:
>> On 1/25/24 3:19 AM, Vadim Fedorenko wrote:
>>> On 25/01/2024 01:10, Martin KaFai Lau wrote:
>>>> On 1/15/24 2:08 PM, Vadim Fedorenko wrote:
>>>>> +static int bpf_crypto_crypt(const struct bpf_crypto_ctx *ctx,
>>>>> +                const struct bpf_dynptr_kern *src,
>>>>> +                struct bpf_dynptr_kern *dst,
>>>>> +                const struct bpf_dynptr_kern *siv,
>>>>> +                bool decrypt)
>>>>> +{
>>>>> +    u32 src_len, dst_len, siv_len;
>>>>> +    const u8 *psrc;
>>>>> +    u8 *pdst, *piv;
>>>>> +    int err;
>>>>> +
>>>>> +    if (ctx->type->get_flags(ctx->tfm) & CRYPTO_TFM_NEED_KEY)
>>>>
>>>> nit. Does the indirect call get_flags() return different values?
>>>> Should it be rejected earlier, e.g. in bpf_crypto_ctx_create()?
>>>
>>> Well, that is the common pattern in crypto subsys to check flags.
>>> But after looking at it second time, I think I have to refactor this
>>> part. CRYPTO_TFM_NEED_KEY is set during tfm creation if algo requires
>>> the key. And it's freed when the key setup is successful. As there is no
>>> way bpf programs can modify tfm directly we can move this check to
>>> bpf_crypto_ctx_create() to key setup part and avoid indirect call in 
>>> this place.
>>>>
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    if (__bpf_dynptr_is_rdonly(dst))
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    siv_len = __bpf_dynptr_size(siv);
>>>>> +    src_len = __bpf_dynptr_size(src);
>>>>> +    dst_len = __bpf_dynptr_size(dst);
>>>>> +    if (!src_len || !dst_len)
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    if (siv_len != (ctx->type->ivsize(ctx->tfm) + 
>>>>> ctx->type->statesize(ctx->tfm)))
>>>>
>>>> Same here, two indirect calls per en/decrypt kfunc call. Does the 
>>>> return value change?
>>>
>>> I have to check the size of IV provided by the caller, and then to avoid
>>> indirect calls I have to store these values somewhere in ctx. It gives a
>>> direct access to these values to bpf programs, which can potentially
>>> abuse them. Not sure if it's good to open such opportunity.
>>
>> I don't think it makes any difference considering tfm has already been 
>> accessible in ctx->tfm.
> 
> Fair. I'll do it then.
> 
>> A noob question, what secret is in the siv len?
> 
> No secrets in the values themself. The problem I see is that user (bpf
> program) can adjust them to avoid proper validation and then pass
> smaller buffer and trigger read/write out-of-bounds.

I've done more tests, and looks like verifier will block programs that
are trying to write directly to the struct. In this case no abuse is
possible and it's safe to export the value into ctx and avoid indirect
calls.

> 
>> btw, unrelated, based on the selftest in patch 3, is it supporting any 
>> siv_len > 0 for now?
> 
> Well, it should. I see no reasons not to support it. But to test it
> properly another cipher should be used. I'll think about extending tests
> 
>>
>>>
>>>>
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    psrc = __bpf_dynptr_data(src, src_len);
>>>>> +    if (!psrc)
>>>>> +        return -EINVAL;
>>>>> +    pdst = __bpf_dynptr_data_rw(dst, dst_len);
>>>>> +    if (!pdst)
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    piv = siv_len ? __bpf_dynptr_data_rw(siv, siv_len) : NULL;
>>>>> +    if (siv_len && !piv)
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    err = decrypt ? ctx->type->decrypt(ctx->tfm, psrc, pdst, 
>>>>> src_len, piv)
>>>>> +              : ctx->type->encrypt(ctx->tfm, psrc, pdst, src_len, 
>>>>> piv);
>>>>> +
>>>>> +    return err;
>>>>> +}
>>>>
>>>
>>
>