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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cec469f4-2fd0-479a-8919-0d5578687fb2@linux.dev>
Date: Thu, 25 Jan 2024 14:34:49 -0800
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Vadim Fedorenko <vadim.fedorenko@...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 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. A noob question, what secret is in the siv len?

btw, unrelated, based on the selftest in patch 3, is it supporting any siv_len > 
0 for now?

> 
>>
>>> +        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;
>>> +}
>>
>