[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f70e2d1e-b17d-44c2-9077-51afa9f4f05e@linux.dev>
Date: Fri, 26 Jan 2024 10:30:50 +0000
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Martin KaFai Lau <martin.lau@...ux.dev>, Vadim Fedorenko <vadfed@...a.com>
Cc: netdev@...r.kernel.org, linux-crypto@...r.kernel.org,
bpf@...r.kernel.org, Victor Stewart <v@...etag.social>,
Jakub Kicinski <kuba@...nel.org>, Andrii Nakryiko <andrii@...nel.org>,
Alexei Starovoitov <ast@...nel.org>, Mykola Lysenko <mykolal@...com>,
Herbert Xu <herbert@...dor.apana.org.au>
Subject: Re: [PATCH bpf-next v8 1/3] bpf: make common crypto API for TC/XDP
programs
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.
> 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;
>>>> +}
>>>
>>
>
Powered by blists - more mailing lists