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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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  }






[Index of Archives]     [Kernel]     [Gnu Classpath]     [Gnu Crypto]     [DM Crypt]     [Netfilter]     [Bugtraq]
  Powered by Linux