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

[Vadim Fedorenko <vadim.fedorenko@xxxxxxxxx>]

On 05/12/2023 21:56, Dan Carpenter wrote:
> Hi Vadim,
>
> kernel test robot noticed the following build warnings:
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Vadim-Fedorenko/bpf-crypto-add-skcipher-to-bpf-crypto/20231202-091254
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
> patch link:    https://lore.kernel.org/r/20231202010604.1877561-1-vadfed%40meta.com
> patch subject: [PATCH bpf-next v7 1/3] bpf: make common crypto API for TC/XDP programs
> config: x86_64-randconfig-161-20231202 (https://download.01.org/0day-ci/archive/20231206/202312060647.2JfAE3rk-lkp@xxxxxxxxx/config)
> compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
> reproduce: (https://download.01.org/0day-ci/archive/20231206/202312060647.2JfAE3rk-lkp@xxxxxxxxx/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@xxxxxxxxx>
> | Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> | Closes: https://lore.kernel.org/r/202312060647.2JfAE3rk-lkp@xxxxxxxxx/
>
> smatch warnings:
> kernel/bpf/crypto.c:192 bpf_crypto_ctx_create() error: we previously assumed 'ctx' could be null (see line 165)
> kernel/bpf/crypto.c:192 bpf_crypto_ctx_create() error: potentially dereferencing uninitialized 'ctx'.
>
> vim +/ctx +192 kernel/bpf/crypto.c
>
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  122  __bpf_kfunc struct bpf_crypto_ctx *
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  123  bpf_crypto_ctx_create(const char *type__str, const char *algo__str,
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  124  		      const struct bpf_dynptr_kern *pkey,
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  125  		      unsigned int authsize, int *err)
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  126  {
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  127  	const struct bpf_crypto_type *type = bpf_crypto_get_type(type__str);
>
> Delete this assignment.  (Duplicated).

Ah, yeah, will remove it.

> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  128  	struct bpf_crypto_ctx *ctx;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  129  	const u8 *key;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  130  	u32 key_len;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  131
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  132  	type = bpf_crypto_get_type(type__str);
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  133  	if (IS_ERR(type)) {
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  134  		*err = PTR_ERR(type);
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  135  		return NULL;
>
> Why doesn't this function just return error pointers?

bpf_kfuncs cannot return error pointers, it makes BPF verifier very unhappy.

> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  136  	}
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  137
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  138  	if (!type->has_algo(algo__str)) {
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  139  		*err = -EOPNOTSUPP;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  140  		goto err;
>
> ctx is uninitialized on this path.
Yep, it was already highlighted in the feedback, thanks.

> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  141  	}
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  142
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  143  	if (!authsize && type->setauthsize) {
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  144  		*err = -EOPNOTSUPP;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  145  		goto err;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  146  	}
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  147
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  148  	if (authsize && !type->setauthsize) {
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  149  		*err = -EOPNOTSUPP;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  150  		goto err;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  151  	}
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  152
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  153  	key_len = __bpf_dynptr_size(pkey);
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  154  	if (!key_len) {
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  155  		*err = -EINVAL;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  156  		goto err;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  157  	}
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  158  	key = __bpf_dynptr_data(pkey, key_len);
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  159  	if (!key) {
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  160  		*err = -EINVAL;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  161  		goto err;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  162  	}
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  163
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  164  	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 @165  	if (!ctx) {
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  166  		*err = -ENOMEM;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  167  		goto err;
>
> ctx is NULL here.
>
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  168  	}
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  169
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  170  	ctx->type = type;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  171  	ctx->tfm = type->alloc_tfm(algo__str);
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  172  	if (IS_ERR(ctx->tfm)) {
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  173  		*err = PTR_ERR(ctx->tfm);
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  174  		ctx->tfm = NULL;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  175  		goto err;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  176  	}
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  177
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  178  	if (authsize) {
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  179  		*err = type->setauthsize(ctx->tfm, authsize);
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  180  		if (*err)
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  181  			goto err;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  182  	}
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  183
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  184  	*err = type->setkey(ctx->tfm, key, key_len);
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  185  	if (*err)
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  186  		goto err;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  187
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  188  	refcount_set(&ctx->usage, 1);
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  189
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  190  	return ctx;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  191  err:
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01 @192  	if (ctx->tfm)
>                                                              ^^^^^^^^
> NULL dereference.  These two error handling bugs in three lines of code
> are canonical One Err Label type bugs.  Better to do a ladder where each
> error label frees the last thing that was allocated.  Easier to review.
> Then you could delete the "ctx->tfm = NULL;" assignment on line 174.
>
> 	return ctx;
>
> err_free_tfm:
> 	type->free_tfm(ctx->tfm);
> err_free_ctx:
> 	kfree(ctx);
> err_module_put:
> 	module_put(type->owner);
>
> 	return NULL;
>
> I have written about this at length on my blog:
> https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/

Thanks, very good blog post, I'll follow this way in the next version.

> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  193  		type->free_tfm(ctx->tfm);
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  194  	kfree(ctx);
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  195  	module_put(type->owner);
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  196
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  197  	return NULL;
> 0c47cb96ac404e Vadim Fedorenko 2023-12-01  198  }